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 Fri, 15 Sep 2017 19:14:52 GMT
Lars Volker has posted comments on this change.

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


Patch Set 6:

(9 comments)

Thank you for working on this. I left some comments inline. Overall I feel that we're still
not completely decided on which new features of C++11 and new paradigms they allow we want
to use. I'd be happy to follow up on this on dev@.

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 you replace some
of these with C++11 equivalents (e.g. for_each))?


Line 39: constexpr auto i32_max = numeric_limits<int32_t>::max();
We currently use auto only for iterators and const& in loops.


Line 73: TEST(QueryOptions, SetByteOptions) {
Can you add a comment explaining what the test does overall? Same for the other tests.


Line 96:   auto process_case_set = [&](auto& case_set) {
I find this part hard to follow and its use of lambdas seems to deviate from our current best
practices somewhat. Can you try to refactor some parts using plain functions to generate context
objects instead?


PS6, Line 97: kase
Can you think of a better name?


PS6, Line 99: minus_1_to_0
Why is this needed? Is the added level of indirection worth it?


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 documentation
on how to define test cases?


Line 171:     fusion::for_each(case_set, [&options, accept_integer](auto& kase) {
Can you use C++11 for_each instead?


Line 263:     for (auto& key_def : {key_def_min, key_def_default}) {
const auto&?


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

Mime
View raw message