Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/22094 )
Change subject: IMPALA-13533: Calcite CTE backend ...................................................................... Patch Set 26: (9 comments) Still trying to understand this, added a few comments about BE. http://gerrit.cloudera.org:8080/#/c/22094/25/be/src/exec/cte-consumer-node.cc File be/src/exec/cte-consumer-node.cc: http://gerrit.cloudera.org:8080/#/c/22094/25/be/src/exec/cte-consumer-node.cc@147 PS25, Line 147: VLOG_QUERY << "No CTE exchanger present for CTE consumer: " << name_; When is this possible? When no producer was scheduled to this frag instance? http://gerrit.cloudera.org:8080/#/c/22094/25/be/src/exec/cte-producer-node.cc File be/src/exec/cte-producer-node.cc: http://gerrit.cloudera.org:8080/#/c/22094/25/be/src/exec/cte-producer-node.cc@36 PS25, Line 36: // Register the exchanger here while plan setup is single-threaded. Consumers look for exchanges in Prepare - can you mention that this guarantees that producers are registered first? http://gerrit.cloudera.org:8080/#/c/22094/25/be/src/exec/cte-producer-node.cc@97 PS25, Line 97: SleepForMs(10); : *eos = exchanger_->ReadFinished(); Why doesn't CTEProducerNode wait for the exchanger to finish (or for cancellation)? It it possible to stop it in any other way? http://gerrit.cloudera.org:8080/#/c/22094/25/be/src/exec/sequence-node.h File be/src/exec/sequence-node.h: http://gerrit.cloudera.org:8080/#/c/22094/25/be/src/exec/sequence-node.h@35 PS25, Line 35: class SequenceNode : public ExecNode { : public: Should I see this node in some plan? I didn't spot it in any plans http://gerrit.cloudera.org:8080/#/c/22094/25/be/src/runtime/local-exchanger.h File be/src/runtime/local-exchanger.h: http://gerrit.cloudera.org:8080/#/c/22094/25/be/src/runtime/local-exchanger.h@32 PS25, Line 32: Each consumer gets its own copy of each RowBatch and tracks its own memory : /// ownership It could be noted that the copy is the responsibility of the caller. http://gerrit.cloudera.org:8080/#/c/22094/25/be/src/runtime/local-exchanger.cc File be/src/runtime/local-exchanger.cc: http://gerrit.cloudera.org:8080/#/c/22094/25/be/src/runtime/local-exchanger.cc@26 PS25, Line 26: consumer_count_ Is it possible for a consumer to close while the producer is still pushing, e.g because a node in the tree reaches its limit? If not, then a DCHECK could check consumers_done_ == 0. My concern is that consumers_left will be initialized to the full count, but the closed consumer will not decrease it. http://gerrit.cloudera.org:8080/#/c/22094/25/be/src/runtime/local-exchanger.cc@37 PS25, Line 37: dummy It doesn't look completely dummy to me, as in line 45 the currently processed cell replaces it, which still points to a living RowBatch. http://gerrit.cloudera.org:8080/#/c/22094/25/be/src/runtime/local-exchanger.cc@44 PS25, Line 44: cell->consumers_left--; I see that this works as a cell is cleaned up not when it's consumers_left reaches 0 but when the next one's - it is a bit counter-intuitive to me, would it be possible to decrease progress_[consumer_index]->consumers_left instead, and delete it when it reaches 0? http://gerrit.cloudera.org:8080/#/c/22094/25/be/src/runtime/local-exchanger.cc@70 PS25, Line 70: head_->consumers_left == 0 Can consumers_left go below 0? If not, then there could be a DCHECK for it. This looks possible to me if all consumers fetched the cell, decreasing it's consumers_left to 0, but it is still kept in queue due to line 73. If a consumer call close then it will decrease consumers_left even if it is 0. -- To view, visit http://gerrit.cloudera.org:8080/22094 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I48f16d495d4b37be97e6a913f0eb5b94d70e199a Gerrit-Change-Number: 22094 Gerrit-PatchSet: 26 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Wed, 17 Dec 2025 01:06:28 +0000 Gerrit-HasComments: Yes
