Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/18531 )
Change subject: IMPALA-10453: Support file pruning via runtime filters on Iceberg ...................................................................... Patch Set 5: (7 comments) Looks good. http://gerrit.cloudera.org:8080/#/c/18531/5/be/src/exec/file-metadata-utils.h File be/src/exec/file-metadata-utils.h: http://gerrit.cloudera.org:8080/#/c/18531/5/be/src/exec/file-metadata-utils.h@40 PS5, Line 40: scanner context We no longer store a scanner context. Shouldn't it be "corresponding to 'partition_id'"? http://gerrit.cloudera.org:8080/#/c/18531/5/be/src/exec/hdfs-scan-node-base.h File be/src/exec/hdfs-scan-node-base.h: http://gerrit.cloudera.org:8080/#/c/18531/5/be/src/exec/hdfs-scan-node-base.h@645 PS5, Line 645: , Nit: a full stop would be better. http://gerrit.cloudera.org:8080/#/c/18531/5/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: http://gerrit.cloudera.org:8080/#/c/18531/5/be/src/exec/hdfs-scan-node-base.cc@724 PS5, Line 724: (int64_t) Do we need this cast here? ScanRangeMetadata::partition_id is already an int64_t: https://github.com/apache/impala/blob/1d16367afb1e520bae5992c1e6c92b4fb41d2655/be/src/exec/hdfs-scan-node-base.h#L108 http://gerrit.cloudera.org:8080/#/c/18531/5/be/src/exec/hdfs-scan-node-base.cc@949 PS5, Line 949: &f Nit: space http://gerrit.cloudera.org:8080/#/c/18531/5/be/src/exec/hdfs-scan-node-base.cc@955 PS5, Line 955: // Exprs guaranteed to be literals, so can safely be evaluated without a row. Does this mean that template_tuple can be NULL? If so, this could be clarified. http://gerrit.cloudera.org:8080/#/c/18531/5/be/src/exec/hdfs-scan-node-base.cc@955 PS5, Line 955: // Exprs guaranteed to be literals, so can safely be evaluated without a row. Nit: wouldn't this comment be better placed above the line containing ctx.Eval(row)? http://gerrit.cloudera.org:8080/#/c/18531/5/be/src/exec/hdfs-scan-node-base.cc@955 PS5, Line 955: // Exprs guaranteed to be literals, so can safely be evaluated without a row. Can we DCHECK that they are indeed literals? -- To view, visit http://gerrit.cloudera.org:8080/18531 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7762e1238bdf236b85d2728881a402a2bb41f36a Gerrit-Change-Number: 18531 Gerrit-PatchSet: 5 Gerrit-Owner: Tamas Mate <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gergely Fürnstáhl <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 21 Jul 2022 13:25:51 +0000 Gerrit-HasComments: Yes
