Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22091 )
Change subject: IMPALA-13531: Calcite CTE frontend ...................................................................... Patch Set 14: (1 comment) http://gerrit.cloudera.org:8080/#/c/22091/14/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSequence.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSequence.java: http://gerrit.cloudera.org:8080/#/c/22091/14/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaSequence.java@39 PS14, Line 39: private final List<RelNode> inputs_; > I don't follow. I implemented both replaceInput and copy so that callers ca The problem I think exists is that these aren't normal "inputs". It's a separate list maintained on the top layer tracking lower level inputs. So no one is gonna call "replaceInput" for the ImpalaSequence RelNode for these extra inputs. Also, as a sidenote, this is slightly different from runImpalaConvertProgram because that program is run at the very end. We know nothing else will change in the RelNode tree structure at that point. It's hard to be clear about this, so my apologies, but I'll try to step through what I'm thinking... Let's say a copy was done for ImpalaCTEConsumer right after the CTE Program was run. Why would a copy be needed? I don't think it will be done because the internals of Calcite know nothing about trying to re-create an ImpalaConsumer. So that's why I think we can throw an exception there. But let's say it did. Chance are this would happen with the parent of ImpalaCTEConsumer. Maybe through reflection? Geez, I hope they're not doing that, but they might be. But the parent might want to replace its child RelNode ImpalaCTEConsumer with a copy of it. But this is entirely separate from the ImpalaSequence copy of it. There's no way the internals of Calcite can know that the upper layer of ImpalaSequence is holding onto a lower level of ImpalaCTEConsumer. So they will now be pointing to different ImpalaCTEConsumers, and the ImpalaSequence one will be stale. So there are still rules that are run after the CTEProgram. So now let's say Calcite changes a Project immediately under a CTEProducer. Calcite would probably call RelNode.replaceInput() for the immediate parent of the Project, our ImpalaCTEProducer. But that wouldn't change the ImpalaSequence's copy of the ImpalaCTEProducer. Anyway, I don't really think your code is buggy unless Calcite is using reflection somewhere to call the copy method. So I think we should protect that and throw an exception there. I hope that made sense? -- To view, visit http://gerrit.cloudera.org:8080/22091 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id0840c0859d2fe25628d799a18d302cee1eb36e8 Gerrit-Change-Number: 22091 Gerrit-PatchSet: 14 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Anonymous Coward (816) 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: Sat, 13 Dec 2025 17:26:40 +0000 Gerrit-HasComments: Yes
