Janaki Lahorani has posted comments on this change. ( http://gerrit.cloudera.org:8080/12113 )
Change subject: IMPALA-6533: Add min-max filter for decimal types on kudu tables. ...................................................................... Patch Set 13: (20 comments) Thanks Tim. All comments from Tim are addressed in Patch Set 13. http://gerrit.cloudera.org:8080/#/c/12113/11/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/12113/11/be/src/runtime/coordinator.cc@972 PS11, Line 972: sc_.src_e > I don't think this cast is necessary, is it? Done http://gerrit.cloudera.org:8080/#/c/12113/4/be/src/runtime/decimal-value.h File be/src/runtime/decimal-value.h: http://gerrit.cloudera.org:8080/#/c/12113/4/be/src/runtime/decimal-value.h@205 PS4, Line 205: } > We could simplify this by memcpy()ing the raw bytes into t_value->decimal_v Done http://gerrit.cloudera.org:8080/#/c/12113/4/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: http://gerrit.cloudera.org:8080/#/c/12113/4/be/src/runtime/decimal-value.inline.h@96 PS4, Line 96: } > We'd usually add a DCHECK(false) << sizeof(T) here or in the default of the Fixed the code to use the binary value directly. http://gerrit.cloudera.org:8080/#/c/12113/4/be/src/util/min-max-filter-ir.cc File be/src/util/min-max-filter-ir.cc: http://gerrit.cloudera.org:8080/#/c/12113/4/be/src/util/min-max-filter-ir.cc@104 PS4, Line 104: case 16: > Same comment about a DCHECK in the default: case Done http://gerrit.cloudera.org:8080/#/c/12113/11/be/src/util/min-max-filter.h File be/src/util/min-max-filter.h: http://gerrit.cloudera.org:8080/#/c/12113/11/be/src/util/min-max-filter.h@91 PS11, Line 91: > Maybe just pass in a const ColumnType&? That would be more generic. Feel fr Done http://gerrit.cloudera.org:8080/#/c/12113/11/be/src/util/min-max-filter.h@94 PS11, Line 94: /// Copies the contents of 'in' into 'out'. > This overload only exists as a convenience for test functions right? It's c Done http://gerrit.cloudera.org:8080/#/c/12113/4/be/src/util/min-max-filter.h File be/src/util/min-max-filter.h: http://gerrit.cloudera.org:8080/#/c/12113/4/be/src/util/min-max-filter.h@246 PS4, Line 246: switch (size_) { \ > M is a bit cryptic - maybe min_or_max? It's probably also worth adding a ve Done http://gerrit.cloudera.org:8080/#/c/12113/4/be/src/util/min-max-filter.h@273 PS4, Line 273: rtual ~Decima > Purely for consistently, we can do this assignments in the constructor init Done http://gerrit.cloudera.org:8080/#/c/12113/4/be/src/util/min-max-filter.h@282 PS4, Line 282: null > We've been standardising on C++11 nullptr instead of NULL. The codebase is Done http://gerrit.cloudera.org:8080/#/c/12113/4/be/src/util/min-max-filter.h@304 PS4, Line 304: > nit: GetSize() Done http://gerrit.cloudera.org:8080/#/c/12113/4/be/src/util/min-max-filter.h@314 PS4, Line 314: }; > Could make this const and initialise in constructor init list, e.g. Done http://gerrit.cloudera.org:8080/#/c/12113/11/be/src/util/min-max-filter.cc File be/src/util/min-max-filter.cc: http://gerrit.cloudera.org:8080/#/c/12113/11/be/src/util/min-max-filter.cc@492 PS11, Line 492: if (in.always_false) { > nit: our style is to use braces around the if body (except if the whole if Done http://gerrit.cloudera.org:8080/#/c/12113/11/testdata/data/README File testdata/data/README: http://gerrit.cloudera.org:8080/#/c/12113/11/testdata/data/README@293 PS11, Line 293: decimal values that covers the 3 storage formats with various precision and scale. > Can you mention how the data was generated, in case someone wants to reprod Done http://gerrit.cloudera.org:8080/#/c/12113/4/testdata/data/decimal_rtf_tbl.txt File testdata/data/decimal_rtf_tbl.txt: PS4: > Can you add a brief description of the new files to testdata/data/README. P Done http://gerrit.cloudera.org:8080/#/c/12113/4/testdata/datasets/functional/functional_schema_template.sql File testdata/datasets/functional/functional_schema_template.sql: http://gerrit.cloudera.org:8080/#/c/12113/4/testdata/datasets/functional/functional_schema_template.sql@1775 PS4, Line 1775: functional > Can you add a -- comment per file to explain the purpose? Done http://gerrit.cloudera.org:8080/#/c/12113/11/testdata/datasets/functional/functional_schema_template.sql File testdata/datasets/functional/functional_schema_template.sql: http://gerrit.cloudera.org:8080/#/c/12113/11/testdata/datasets/functional/functional_schema_template.sql@1854 PS11, Line 1854: functional > I guess the handling of comments in these files is a bit broken - it's incl Done http://gerrit.cloudera.org:8080/#/c/12113/4/testdata/workloads/functional-query/queries/QueryTest/min_max_filters.test File testdata/workloads/functional-query/queries/QueryTest/min_max_filters.test: http://gerrit.cloudera.org:8080/#/c/12113/4/testdata/workloads/functional-query/queries/QueryTest/min_max_filters.test@70 PS4, Line 70: select STRAIGHT_JOIN count(*) from decimal_rtf_tbl a > Can you add a linebreak before the "from" clauses. That'll get the queries Done http://gerrit.cloudera.org:8080/#/c/12113/11/testdata/workloads/functional-query/queries/QueryTest/min_max_filters.test File testdata/workloads/functional-query/queries/QueryTest/min_max_filters.test: http://gerrit.cloudera.org:8080/#/c/12113/11/testdata/workloads/functional-query/queries/QueryTest/min_max_filters.test@2789 PS11, Line 2789: > I think it would be good to break out this sections into separate .test fil Done http://gerrit.cloudera.org:8080/#/c/12113/4/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/12113/4/tests/common/impala_test_suite.py@515 PS4, Line 515: rt_profile = verify_runtime_profile(test_section[rt_profile_info], > nit: long line Done http://gerrit.cloudera.org:8080/#/c/12113/4/tests/common/test_result_verifier.py File tests/common/test_result_verifier.py: http://gerrit.cloudera.org:8080/#/c/12113/4/tests/common/test_result_verifier.py@605 PS4, Line 605: if update_section: > nit: long lines Done -- To view, visit http://gerrit.cloudera.org:8080/12113 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib7e7278e902160d7060f8097290bc172d9031f94 Gerrit-Change-Number: 12113 Gerrit-PatchSet: 13 Gerrit-Owner: Janaki Lahorani <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Janaki Lahorani <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 04 Jan 2019 21:41:50 +0000 Gerrit-HasComments: Yes
