Amogh Margoor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17706 )
Change subject: IMPALA-3430: Runtime filter : Extend runtime filter to support Min/Max values for HDFS scans ...................................................................... Patch Set 27: (6 comments) Change looks good. I just reviewed the NLJ support for Min/Max Filter and left few nits and questions. Will take a look into FE in second round tomorrow. http://gerrit.cloudera.org:8080/#/c/17706/27/be/src/exec/filter-context.cc File be/src/exec/filter-context.cc: http://gerrit.cloudera.org:8080/#/c/17706/27/be/src/exec/filter-context.cc@97 PS27, Line 97: void FilterContext::InsertPerCompareOp(TupleRow* row) const noexcept { Will this be executed for ever row on the build side of join ? If yes, I am worried about the switch statement. Do we remove it via codegen ? http://gerrit.cloudera.org:8080/#/c/17706/27/be/src/exec/join-builder.cc File be/src/exec/join-builder.cc: http://gerrit.cloudera.org:8080/#/c/17706/27/be/src/exec/join-builder.cc@148 PS27, Line 148: << ", fillter details=" << ctx.local_min_max_filter->DebugString() nit: filter instead of fillter http://gerrit.cloudera.org:8080/#/c/17706/27/be/src/exec/nested-loop-join-builder.cc File be/src/exec/nested-loop-join-builder.cc: http://gerrit.cloudera.org:8080/#/c/17706/27/be/src/exec/nested-loop-join-builder.cc@60 PS27, Line 60: const TPlanFragmentInstanceCtx& instance_ctx = state->instance_ctx(); nit: instance_ctx and filters_produced can be read only once, before the loop. http://gerrit.cloudera.org:8080/#/c/17706/27/be/src/exec/nested-loop-join-builder.cc@205 PS27, Line 205: copied_build_batches_.Reset(); Do we need to invoke MinMaxFilter::Close() for allocated filters ? http://gerrit.cloudera.org:8080/#/c/17706/27/be/src/runtime/string-value.h File be/src/runtime/string-value.h: http://gerrit.cloudera.org:8080/#/c/17706/27/be/src/runtime/string-value.h@131 PS27, Line 131: std::string LeastSmallerString(int max_lken) const; nit: 'max_len' http://gerrit.cloudera.org:8080/#/c/17706/27/be/src/runtime/string-value.h@136 PS27, Line 136: std::string LeastLargerString(int max_len) const; These two functions here seems like utility function and would be better placed in util/string-util.h -- To view, visit http://gerrit.cloudera.org:8080/17706 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7c2bb5baad622051d1002c9c162c672d428e5446 Gerrit-Change-Number: 17706 Gerrit-PatchSet: 27 Gerrit-Owner: Qifan Chen <[email protected]> Gerrit-Reviewer: Amogh Margoor <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 11 Aug 2021 22:16:03 +0000 Gerrit-HasComments: Yes
