Anonymous Coward #248 has posted comments on this change. ( http://gerrit.cloudera.org:8080/9693 )
Change subject: IMPALA-5842: Write page index in Parquet files ...................................................................... Patch Set 6: (2 comments) I have 2 minor findings only. Otherwise, LGTM. (I'm not an Impala nor a C/C++ expert, though.) http://gerrit.cloudera.org:8080/#/c/9693/6/be/src/exec/hdfs-parquet-table-writer.cc File be/src/exec/hdfs-parquet-table-writer.cc: http://gerrit.cloudera.org:8080/#/c/9693/6/be/src/exec/hdfs-parquet-table-writer.cc@727 PS6, Line 727: // max_values_ and mark the values as valid. In case min and max values are not It is a bit misleading to by talking about marking the min/max values valid/invalid. It is about the null pages which case the min/max values do not make sense so we store empty strings instead. http://gerrit.cloudera.org:8080/#/c/9693/6/be/src/exec/parquet-column-stats.inline.h File be/src/exec/parquet-column-stats.inline.h: http://gerrit.cloudera.org:8080/#/c/9693/6/be/src/exec/parquet-column-stats.inline.h@54 PS6, Line 54: if (MinMaxTrait<T>::Compare(prev_page_max_value_, cs->max_value_) == 1 || I know it is not Java but it might be readable to use a java-like contract for compare. It says compare(a, b) < 0 if a < b etc. It also might have performance benefit in some cases if you simply return a - b instead of if-else. -- To view, visit http://gerrit.cloudera.org:8080/9693 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icbacf7fe3b7672e3ce719261ecef445b16f8dec9 Gerrit-Change-Number: 9693 Gerrit-PatchSet: 6 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Anonymous Coward #248 Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 27 Mar 2018 12:32:29 +0000 Gerrit-HasComments: Yes
