Qifan Chen has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16621 )

Change subject: IMPALA-3816 Codegen perf critical loops in Sorter
......................................................................


Patch Set 7:

(8 comments)

Thanks a lot Csaba and Daniel!

On the comment "if 'CodegenFnPtr<Sorter::SortHelperFn> 
codegend_sort_helper_fn_' could be owned by Sorter::TupleSorter instead of 
SortPlanNode and PartialSortPlanNode. It seems that Sorter::TupleSorter is 
responsible for both codegening and calling it."

There is a timing between the codegen and the use of the codegened method, 
where the former is done for the plan node in general throughout the code base, 
originated from FragmentInstanceState::Open() via InvokeCodegen().

http://gerrit.cloudera.org:8080/#/c/16621/7//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/16621/7//COMMIT_MSG@7
PS7, Line 7: IMPALA-3816 Codegen perf critical loops in Sorter
> nit: missing :
Done


http://gerrit.cloudera.org:8080/#/c/16621/7/be/src/exec/partial-sort-node.h
File be/src/exec/partial-sort-node.h:

http://gerrit.cloudera.org:8080/#/c/16621/7/be/src/exec/partial-sort-node.h@45
PS7, Line 45: TupelSorter
> Typo: TupleSorter.
Done


http://gerrit.cloudera.org:8080/#/c/16621/7/be/src/exec/partial-sort-node.cc
File be/src/exec/partial-sort-node.cc:

http://gerrit.cloudera.org:8080/#/c/16621/7/be/src/exec/partial-sort-node.cc@26
PS7, Line 26: #include "runtime/sorter-internal.h"
> Nit: This include should come before "util/runtime-profile-counters.h" beca
Done


http://gerrit.cloudera.org:8080/#/c/16621/7/be/src/exec/sort-node.cc
File be/src/exec/sort-node.cc:

http://gerrit.cloudera.org:8080/#/c/16621/7/be/src/exec/sort-node.cc@27
PS7, Line 27: #include "codegen/llvm-codegen.h"
            : #include "runtime/sorter-internal.h"
> Should be sorted alphabetically.
Done


http://gerrit.cloudera.org:8080/#/c/16621/7/be/src/runtime/sorter-internal.h
File be/src/runtime/sorter-internal.h:

http://gerrit.cloudera.org:8080/#/c/16621/7/be/src/runtime/sorter-internal.h@440
PS7, Line 440: attemp
> typo: attempt
Done


http://gerrit.cloudera.org:8080/#/c/16621/7/be/src/runtime/sorter-internal.h@442
PS7, Line 442: GENERAL
> The implementation doesn't return Status("GENERAL") but Status("Sorter::Tup
Done


http://gerrit.cloudera.org:8080/#/c/16621/7/be/src/runtime/sorter.h
File be/src/runtime/sorter.h:

http://gerrit.cloudera.org:8080/#/c/16621/7/be/src/runtime/sorter.h@106
PS7, Line 106: code-gen
> Nit: we usually write it as 'codegen'.
Done


http://gerrit.cloudera.org:8080/#/c/16621/7/be/src/runtime/sorter.cc
File be/src/runtime/sorter.cc:

http://gerrit.cloudera.org:8080/#/c/16621/7/be/src/runtime/sorter.cc@35
PS7, Line 35: #include "runtime/fragment-state.h"
> The included headers are generally in the block above, sorted alphabeticall
Good to know! Done.



--
To view, visit http://gerrit.cloudera.org:8080/16621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie08137449d4a7b554ca8b8650260f8bd72e0a81b
Gerrit-Change-Number: 16621
Gerrit-PatchSet: 7
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Comment-Date: Mon, 26 Oct 2020 14:02:00 +0000
Gerrit-HasComments: Yes

Reply via email to