Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/18393 )
Change subject: IMPALA-4530: Implement in-memory merge of quicksorted runs ...................................................................... Patch Set 17: (2 comments) http://gerrit.cloudera.org:8080/#/c/18393/17/be/src/runtime/sorter.cc File be/src/runtime/sorter.cc: http://gerrit.cloudera.org:8080/#/c/18393/17/be/src/runtime/sorter.cc@154 PS17, Line 154: DCHECK_GT(sorter_->max_num_of_pages_, 0); > DCHECK(!initialized) here for safety with RETURN_IF_ERROR calls below. RETURN_IF_ERROR won't set *initialized. Looks like the callers currently check the return status so it should be ok like it is. It's just a little hard to follow without checking how the caller is handling returns. http://gerrit.cloudera.org:8080/#/c/18393/17/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/18393/17/common/thrift/Query.thrift@625 PS17, Line 625: // Minimum is 2, recommended is 10 or more to avoid high fragmentation > also merge buffer requirement and merge cost I see not many options are documented in the Query.thrift. My reservation here is the description doesn't give enough context which could lead to improper usage. Maybe best to add more guidance in the commit message and not get into details here. -- To view, visit http://gerrit.cloudera.org:8080/18393 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58c0ae112e279b93426752895ded7b1a3791865c Gerrit-Change-Number: 18393 Gerrit-PatchSet: 17 Gerrit-Owner: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Comment-Date: Mon, 27 Mar 2023 14:31:28 +0000 Gerrit-HasComments: Yes
