Lars Volker has posted comments on this change. Change subject: IMPALA-2328: Read support for min/max Parquet statistics ......................................................................
Patch Set 11: (6 comments) Thank you Alex for the review. I submitted a change to address your comments here: https://gerrit.cloudera.org/#/c/6147/ Let's continue the review there. http://gerrit.cloudera.org:8080/#/c/6032/11/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: Line 194: // Allocate tuple buffer to evaluate conjuncts on parquet::Statistics. > Sorry to keep asking you about more JIRAs :), but you could file a JIRA to Sure thing, IMPALA-4986 Line 196: if (min_max_tuple_desc) { > != nullptr? Done, do we do that by convention? Line 496: int col_idx = slot_desc->col_pos() - scan_node_->num_partition_keys(); > I think we need to handle the name-based field solution policy here as well Done Line 497: DCHECK(col_idx < row_group.columns.size()); > I don't think this DCHECK is right. A Parquet file may legitimately have fe Using the schema resolver should catch this case now, so I kept the DCHECK. http://gerrit.cloudera.org:8080/#/c/6032/11/be/src/exec/parquet-metadata-utils.cc File be/src/exec/parquet-metadata-utils.cc: Line 223: DCHECK(col_idx < row_group.columns.size()); > I don't think this DCHECK is right. A Parquet file may legitimately not hav With the changes in the caller I think it is correct now. Within the scope of this method it seems reasonable to me to expect the caller to make this guarantee. http://gerrit.cloudera.org:8080/#/c/6032/11/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: Line 340: if (!(binaryPred.getChild(0) instanceof SlotRef)) continue; > Why not allow cast SlotRefs? Added support for implicit casts. -- To view, visit http://gerrit.cloudera.org:8080/6032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I39b836165756fcf929c801048d91c50c8fdcdae4 Gerrit-PatchSet: 11 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Matthew Mulder <mmul...@cloudera.com> Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-HasComments: Yes
