Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22094 )
Change subject: IMPALA-13533: Calcite CTE backend ...................................................................... Patch Set 54: (9 comments) http://gerrit.cloudera.org:8080/#/c/22094/52//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22094/52//COMMIT_MSG@36 PS52, Line 36: map eac > Nit: extra map Done http://gerrit.cloudera.org:8080/#/c/22094/52/be/src/exec/cte-consumer-node.cc File be/src/exec/cte-consumer-node.cc: http://gerrit.cloudera.org:8080/#/c/22094/52/be/src/exec/cte-consumer-node.cc@167 PS52, Line 167: LocalExchanger > Nit: Comment out of date Done http://gerrit.cloudera.org:8080/#/c/22094/52/be/src/exec/cte-consumer-node.cc@188 PS52, Line 188: // Ensure blocking operators make a deep copy of data if they need to retain it. : // Heap memory is re-used from inp > Nit: If I'm understanding this right, this is critical for having the right Ack http://gerrit.cloudera.org:8080/#/c/22094/52/be/src/exec/cte-producer-node.h File be/src/exec/cte-producer-node.h: http://gerrit.cloudera.org:8080/#/c/22094/52/be/src/exec/cte-producer-node.h@40 PS52, Line 40: /// Node that buffers results produced by a Common Table Expression into a : /// LocalExchanger. > Let's add some more detail here. Right now, everything accumulates in Open( Done http://gerrit.cloudera.org:8080/#/c/22094/52/be/src/exec/cte-producer-node.cc File be/src/exec/cte-producer-node.cc: http://gerrit.cloudera.org:8080/#/c/22094/52/be/src/exec/cte-producer-node.cc@97 PS52, Line 97: DCHECK(!child_batch->needs_deep_copy()); > I think either here or in the local exchanger or both, we should DCHECK tha I'm running into weird cases here. We get some batches back with needs_deep_copy() set, but in every case the next call to GetNext returns eos, and in some cases they have 0 rows. I tried to add a conservative DeepCopyTo for !eos && child_batch->needs_deep_copy() && child_batch->num_rows() > 0 but Q70 returns different results when I do that. Need to investigate further. http://gerrit.cloudera.org:8080/#/c/22094/52/be/src/exec/sequence-node.h File be/src/exec/sequence-node.h: http://gerrit.cloudera.org:8080/#/c/22094/52/be/src/exec/sequence-node.h@32 PS52, Line 32: /// Node that consumes (calls Open() but not GetNext()) terminal children in-order, before : /// passing thro > So, a SequenceNode runs Open() on the non-passthrough children (but never G Ack http://gerrit.cloudera.org:8080/#/c/22094/52/be/src/runtime/local-exchanger.h File be/src/runtime/local-exchanger.h: http://gerrit.cloudera.org:8080/#/c/22094/52/be/src/runtime/local-exchanger.h@36 PS52, Line 36: /// In-memory exchange. One producer pushes RowBatches which are consumed by multiple : /// consumers. Pull returns the original RowBatch; creating a copy is the responsibility : /// of the caller. Thread-safe. Inspired by StarRocks' multi_cast_local_exchange. > Let me check my understanding. Some upsides: Yup, those make sense as enhancements. http://gerrit.cloudera.org:8080/#/c/22094/52/be/src/runtime/local-exchanger.cc File be/src/runtime/local-exchanger.cc: http://gerrit.cloudera.org:8080/#/c/22094/52/be/src/runtime/local-exchanger.cc@39 PS52, Line 39: Status LocalExchanger::Push(std::unique_ptr<RowBatch> batch) { > I think we'll want a way to see how much memory is accumulated in this loca Do we have a straight-forward way to identify memory owned by the RowBatch? Maybe I should use SetMemTracker? Currently I think the CTEProducerNode would show retained memory in row batches. http://gerrit.cloudera.org:8080/#/c/22094/52/testdata/workloads/functional-query/queries/QueryTest/cte-distributed.test File testdata/workloads/functional-query/queries/QueryTest/cte-distributed.test: http://gerrit.cloudera.org:8080/#/c/22094/52/testdata/workloads/functional-query/queries/QueryTest/cte-distributed.test@52 PS52, Line 52: WITH v1 AS (SELECT n_name, n_nationkey FROM tpch.nation WHERE n_regionkey = 0) : SELECT v1.n_name, v1_next.n_name FROM v1, v1 v1_next WHERE v1.n_nationkey + 1 = v1_next.n_nationkey; > The test passes, but if I run this manually in impala-shell, it hits this D This is a very contrived case, but we shouldn't hit that. I'll look into it. -- 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: 54 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Anonymous Coward (816) Gerrit-Reviewer: Balazs Hevele <[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: Tue, 21 Jul 2026 22:03:34 +0000 Gerrit-HasComments: Yes
