impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options
Date Tue, 19 Sep 2017 18:49:33 GMT
Lars Volker has posted comments on this change.

Change subject: IMPALA-5425: Add test for validating input when setting query options
......................................................................


Patch Set 7:

(5 comments)

I'm still trying to see if there are ways to simplify the code with patterns we use more commonly.
If you disagree, let's catch up in person, since I'm not set on what's the right way forward
here.

http://gerrit.cloudera.org:8080/#/c/7805/6/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

PS6, Line 137:   };
             :   TestByteCaseSet(options, case_set_i64);
             :   TestByteCaseSet(options, case_set_i32);
             : }
             : 
             : // Test an enum testcase. May or may not accept integer
             : template<typename KeyType, typename EnumType>
             : void TestEnumCase(TQueryOptions& options,
> I'm not sure which part I should look at. The complication here is that I n
Can you use a templatiezed class to store the test cases instead of a tuple? Additionally
you could use a small factory method to hide the macros.


Line 171:   });
> Not that easy. Let me explain:
Could you use https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#value-parameterized-tests
with a templatized parameter class to achieve the same?


http://gerrit.cloudera.org:8080/#/c/7805/7/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

Line 79:     auto ok = MakeOk(options, test_case.first);
I think it would be helpful to see that these are functions here. Can you remove the auto?


Line 81:     auto lb = test_case.second.first;
Do these need to be auto?


Line 103:     for (auto& value : common_value) {
const? Please check elsewhere, too.


-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang <twang@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <twang@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message