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 <tw...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-HasComments: Yes
