Tianyi Wang has posted comments on this change. Change subject: IMPALA-5425: Add test for validating input when setting query options ......................................................................
Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/7805/6/be/src/service/query-options-test.cc File be/src/service/query-options-test.cc: Line 20: #include <boost/fusion/algorithm/iteration/for_each.hpp> > We are generally trying to reduce our dependencies on Boost libraries. Can See comment on line 171. Line 39: constexpr auto i32_max = numeric_limits<int32_t>::max(); > We currently use auto only for iterators and const& in loops. Removed some autos. Line 73: TEST(QueryOptions, SetByteOptions) { > Can you add a comment explaining what the test does overall? Same for the o Done Line 96: auto process_case_set = [&](auto& case_set) { > I find this part hard to follow and its use of lambdas seems to deviate fro Flattened them into template functions. PS6, Line 97: kase > Can you think of a better name? Done PS6, Line 99: minus_1_to_0 > Why is this needed? Is the added level of indirection worth it? Removed. PS6, Line 137: // Expands to {"entry", prefix::entry}, : #define ENTRY(_, prefix, entry) {BOOST_PP_STRINGIZE(entry), prefix::entry}, : // ENTRIES(prefix, (a)(b)) expands to {"a", prefix::a}, {"b", prefix::b}, : #define ENTRIES(prefix, name) BOOST_PP_SEQ_FOR_EACH(ENTRY, prefix, name) : // A case is a pair: (keydef, [(enum_string, enum_value)]) : #define CASE(key, enumtype, enums) make_pair(key, \ : vector<pair<const char*, enumtype::type>>\ : {ENTRIES(enumtype, BOOST_PP_TUPLE_TO_SEQ(enums))}) > I find this very hard to understand. Can you have a look at the Google Test I'm not sure which part I should look at. The complication here is that I need a test case to include: 1. the string name of that option "prefetch_mode" 2. A reference to the member in TQueryOptions: &options.prefetch_mode 3. A list of enum value, and their string representation: vector<...>{make_pair("NONE", TPrefetchMode::NONE), make_pair("HT_BUCKET", TPrefetchMode::HT_BUCKET)} I added an example to show what expanded code looks like. If no macros are used here, this part of code would be really long and repetitive. I think though the macros are complicated, the test cases are clear and easy to modify. By looking at "CASE(KEY(prefetch_mode), TPrefetchMode, (NONE, HT_BUCKET))" it's easy to know how to add/modify a test case. Line 171: fusion::for_each(case_set, [&options, accept_integer](auto& kase) { > Can you use C++11 for_each instead? Not that easy. Let me explain: The complication here is that, each enum query option is of different type. For example, the type of options.prefetch_mode is TPrefetchMode::type. So the enum_and_int is actually a tuple of different types, and that's why I used make_tuple instead of a vector. What we need here is to iterate through the tuple, and that is not what std::for_each can do. Line 263: for (auto& key_def : {key_def_min, key_def_default}) { > const auto&? Done. -- 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: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tianyi Wang <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-HasComments: Yes
