venkata91 commented on PR #4928: URL: https://github.com/apache/calcite/pull/4928#issuecomment-4410925235
> This looks fine. I wonder whether not handling TableFunctionScan was an omission or was intentional. Thanks for the review, @mihaibudiu! It looks like an omission, not intentional: - The sibling TableScan does have the override (TableScan.java:180), but TableFunctionScan doesn't — no semantic reason to differ. - AbstractRelNode.accept(RelShuttle)'s doc explicitly states the override is expected wherever a corresponding RelShuttle.visit(...) exists (and it does for TableFunctionScan). - LogicalWindow and LogicalSnapshot are missing it too. SqlHintsConverterTest.HintCollector had a visit(RelNode) + instanceof workaround covering exactly those three — looks like accumulated workarounds for the same gap. I encountered this while working adding support for `projection pushdown` with `UNNEST` in Flink. See https://github.com/apache/flink/pull/28127 Without this fix, any rule that walks a Correlate's right subtree with a RexShuttle to renumber $cor0.X references (e.g. ProjectCorrelateTransposeRule.RelNodesExprsHandler after pruning the left input) silently skips the TFS's rexCall — dispatch routes through visit(RelNode) instead of visit(TableFunctionScan). The stale field index survives the rewrite, and at runtime the TFS reads the wrong source column. This blocks correct projection pushdown through Correlate-over-TFS shapes such as Flink's UNNEST. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
