Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22094 )
Change subject: IMPALA-13533: Calcite CTE backend ...................................................................... Patch Set 59: Code-Review+1 (2 comments) This is looking good to me, except that we still have bugs to shake out in memory ownership. One way out would be to have an option to deep copy rows in the local exchanger (maybe a startup flag?). If we default to deep copying until we sort out the memory ownership, that might help us keep things stable. I think we're better off with this merged than continuing development outside the repo. I'm looking at the other changes in this stack. http://gerrit.cloudera.org:8080/#/c/22094/59/be/src/exec/cte-producer-node.cc File be/src/exec/cte-producer-node.cc: http://gerrit.cloudera.org:8080/#/c/22094/59/be/src/exec/cte-producer-node.cc@96 PS59, Line 96: // It is safe to move child_batch into the exchanger even when needs_deep_copy is set > I think this is false and it causes an occasional crash. Looking into it mo For a union with passthrough, the union node can return rows marked as needing deep copy, then close the child in the next GetNext(). That seems hard for us to handle. Would it be useful to have a mode that always deep copies? That could be a safer (but slower) option until we have everything nailed down. 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) { > Do we have a straight-forward way to identify memory owned by the RowBatch? I think you're right that CTEProducerNode should know about the memory use. We pass in a single MemTracker for all the RowBatches that we construct, so that should track it. That's a reasonable place to track it, so we wouldn't need it in the LocalExchanger. -- 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: 59 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: Wed, 12 Aug 2026 00:59:10 +0000 Gerrit-HasComments: Yes
