libe8013 commented on code in PR #5043:
URL: https://github.com/apache/calcite/pull/5043#discussion_r3465323504
##########
core/src/main/java/org/apache/calcite/rel/rules/ProjectToWindowRule.java:
##########
@@ -193,7 +194,7 @@ public interface ProjectToLogicalProjectAndWindowRuleConfig
ProjectToLogicalProjectAndWindowRuleConfig DEFAULT =
ImmutableProjectToLogicalProjectAndWindowRuleConfig.of()
.withOperandSupplier(b ->
- b.operand(Project.class)
+ b.operand(LogicalProject.class)
Review Comment:
You're right — the fix itself is not JDBC-specific. This rule is a logical
plan rewrite that emits a LogicalWindow
of Convention.NONE, so narrowing the operand to LogicalProject means it no
longer matches any non-logical Project
(JdbcProject, EnumerableProject, …), not just the JDBC one. JDBC is simply
the path where the bug surfaces: when the
dialect reports supportsWindowFunctions() = true, JdbcProjectRule produces
a JdbcProject carrying the OVER
expression, the old Project.class operand let
ProjectToLogicalProjectAndWindowRule fire on it, and onMatch
propagated the JDBC traitSet into a Convention.NONE LogicalWindow that
doesn't implement JdbcRel → the
"calling-convention" AssertionError.
This is exactly what Danny Chen suggested on CALCITE-3352 ("limit this rule
to only match logical nodes because it
is a plan rewrite"). So I'll rephrase the Jira title to reflect that the
fix is about logical-only matching, e.g.:
[CALCITE-7616] ProjectToLogicalProjectAndWindowRule should only match
logical Project nodes
The test still uses a JDBC schema because that's the realistic way to
obtain a non-logical Project carrying an OVER
expression. Happy to use any wording the maintainers prefer.
--
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]
