Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15087 )
Change subject: IMPALA-8110: Fix the Parquet stats filtering issue to correctly handle narrowed integer types ...................................................................... Patch Set 7: Code-Review+1 (4 comments) Thanks for the fixes so far! A few more nits, mainly about readability and compactness. http://gerrit.cloudera.org:8080/#/c/15087/7/be/src/exec/parquet/parquet-column-stats.cc File be/src/exec/parquet/parquet-column-stats.cc: http://gerrit.cloudera.org:8080/#/c/15087/7/be/src/exec/parquet/parquet-column-stats.cc@100 PS7, Line 100: return false; here + for SMALLINT: Fits to a one line if without braces. http://gerrit.cloudera.org:8080/#/c/15087/7/be/src/exec/parquet/parquet-column-stats.cc@102 PS7, Line 102: ret = ColumnStats<int32_t>::DecodePlainValue(*paired_stats_value, : &paired_stats_val, parquet::Type::INT32); here + for SMALLINT: doesn't need to be in an else block, as the "if" block returns. http://gerrit.cloudera.org:8080/#/c/15087/7/testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test File testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test: http://gerrit.cloudera.org:8080/#/c/15087/7/testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test@690 PS7, Line 690: i An idea to improve readability: i / j / k could be renamed to contain their type, e.g. i32_to_8, i16_to_8, i32_to_16. http://gerrit.cloudera.org:8080/#/c/15087/7/testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test@717 PS7, Line 717: 200 Here and in other = predicates: it seems more interesting to test for the overflown value instead of the out of range positive value (where the result of the count should be 1), as we are mainly hunting for false negatives in the tests. -- To view, visit http://gerrit.cloudera.org:8080/15087 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8bdaf4c4b2d0c6ea26d6e9bf013afca647e53a1 Gerrit-Change-Number: 15087 Gerrit-PatchSet: 7 Gerrit-Owner: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Fri, 07 Feb 2020 14:48:08 +0000 Gerrit-HasComments: Yes
