Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15281 )
Change subject: IMPALA-4080 [part 3]: Moved codegen code for TupleRowComparator into a Config object ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/15281/1/be/src/exec/exchange-node.h File be/src/exec/exchange-node.h: http://gerrit.cloudera.org:8080/#/c/15281/1/be/src/exec/exchange-node.h@49 PS1, Line 49: const TupleRowComparatorConfig* row_comparator_config_ = nullptr; > Can this be const? Done http://gerrit.cloudera.org:8080/#/c/15281/1/be/src/exec/topn-node.h File be/src/exec/topn-node.h: http://gerrit.cloudera.org:8080/#/c/15281/1/be/src/exec/topn-node.h@61 PS1, Line 61: InsertBatchFn codegend_insert_batch_fn_ = nullptr; > init to nullptr? Done http://gerrit.cloudera.org:8080/#/c/15281/1/be/src/exec/topn-node.cc File be/src/exec/topn-node.cc: http://gerrit.cloudera.org:8080/#/c/15281/1/be/src/exec/topn-node.cc@111 PS1, Line 111: Status codegen_status = row_comparator_config_->Codegen(state); > It this comment still valid? Done http://gerrit.cloudera.org:8080/#/c/15281/1/be/src/util/tuple-row-compare.h File be/src/util/tuple-row-compare.h: http://gerrit.cloudera.org:8080/#/c/15281/1/be/src/util/tuple-row-compare.h@70 PS1, Line 70: /// TODO: consider passing TSortInfo. > Would it make sense to pass in TSortInfo? Not sure how invasive that is. I agree it will be cleaner but that would add a few extra Init() and close() calls for managing expressions: - Add an Init methods to create the ordering expressions. - Create a close() method to close them - Also do the same for Sorter class by creating a config class for it and nesting TupleRowComparatorConfig inside it. - For SorterConfig we need to create both ordering_exprs and sort_tuple_slot_exprs inside a new Init() method. I've added a TODO and dont really feel strongly either way so let me know if you feel strongly about making this change. http://gerrit.cloudera.org:8080/#/c/15281/1/be/src/util/tuple-row-compare.h@82 PS1, Line 82: const std::vector<bool>& is_asc_; > Might be a good idea to document nulls_first while we're here - i.e. the -1 Done http://gerrit.cloudera.org:8080/#/c/15281/1/be/src/util/tuple-row-compare.h@87 PS1, Line 87: r<int8_t> nulls_firs > init to null? Done -- To view, visit http://gerrit.cloudera.org:8080/15281 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I265604ecbae2c80ff59e697fddfeea4976b25cd9 Gerrit-Change-Number: 15281 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 28 Feb 2020 19:09:33 +0000 Gerrit-HasComments: Yes
