Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16942 )
Change subject: IMPALA-10296: Fix analytic limit pushdown when predicates are present ...................................................................... Patch Set 13: (10 comments) http://gerrit.cloudera.org:8080/#/c/16942/11/be/src/exec/topn-node-ir.cc File be/src/exec/topn-node-ir.cc: http://gerrit.cloudera.org:8080/#/c/16942/11/be/src/exec/topn-node-ir.cc@44 PS11, Line 44: priority_queue_.Push(insert_tuple); > Might be useful to explicitly mention here that we don't have to worry abou Done http://gerrit.cloudera.org:8080/#/c/16942/11/be/src/exec/topn-node-ir.cc@91 PS11, Line 91: else { : // 'materialized_tuple' needs to be > I might move this above the DCHECK, to make it clearer its referring to the Done http://gerrit.cloudera.org:8080/#/c/16942/11/be/src/exec/topn-node-ir.cc@93 PS11, Line 93: from the heap. : DCHECK_LT(cmp_result > Not sure what this is supposed to mean, since my understanding would be tha I'm not sure exactly what I was getting at, I think I was just restating the invariant that everything in 'overflowed_ties_' was equal with the head of the queue, so it didn't add much. http://gerrit.cloudera.org:8080/#/c/16942/11/be/src/exec/topn-node-ir.cc@97 PS11, Line 97: > Might be worth explicitly saying that 'top_tuple' is what we just popped of Done http://gerrit.cloudera.org:8080/#/c/16942/11/be/src/exec/topn-node-ir.cc@100 PS11, Line 100: node->tuple_row_less_than_->Compare(top_tuple, priority_queue_.Top()) == 0) { > I find this comment a little confusing, since we've already done the one Po Thanks, that's clearer. http://gerrit.cloudera.org:8080/#/c/16942/11/be/src/exec/topn-node.h File be/src/exec/topn-node.h: http://gerrit.cloudera.org:8080/#/c/16942/11/be/src/exec/topn-node.h@86 PS11, Line 86: /// TODO: currently we only support a single top-n heap per operator. IMPALA-9979 will > nit: its a little confusing to call out 'unpartitioned mode' specifically w Yeah I had to pull this comment out of the partitioned mode patch and didn't want to create too many merge conflicts by editing comments. I added a TODO as suggested. http://gerrit.cloudera.org:8080/#/c/16942/11/be/src/exec/topn-node.h@180 PS11, Line 180: RuntimeProfile::Counter* tuple_pool_reclaim_counter_= nullptr; > The counters here and below aren't used in this patch Done http://gerrit.cloudera.org:8080/#/c/16942/11/be/src/exec/topn-node.h@200 PS11, Line 200: n data structure use > nit: not in this patch Done http://gerrit.cloudera.org:8080/#/c/16942/11/be/src/exec/topn-node.h@256 PS11, Line 256: const int64_t capacity_; > If I understand correctly, this function only gets called once the heap has Done. Updated comment and added DCHECK. http://gerrit.cloudera.org:8080/#/c/16942/11/be/src/exec/topn-node.cc File be/src/exec/topn-node.cc: http://gerrit.cloudera.org:8080/#/c/16942/11/be/src/exec/topn-node.cc@306 PS11, Line 306: heap is at > I think this is a typo? i.e. these two words should be removed Done -- To view, visit http://gerrit.cloudera.org:8080/16942 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I801d7799b0d649c73d2dd1703729a9b58a662509 Gerrit-Change-Number: 16942 Gerrit-PatchSet: 13 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Sat, 16 Jan 2021 01:48:13 +0000 Gerrit-HasComments: Yes
