[CALCITE-6746] Optimization rule ProjectWindowTranspose is unsound #4113
Conversation
| List<RexNode> analyze = new ArrayList<>(); | ||
| if (group.lowerBound.getOffset() != null) { | ||
| analyze.add(group.lowerBound.getOffset()); | ||
| } | ||
| if (group.upperBound.getOffset() != null) { | ||
| analyze.add(group.upperBound.getOffset()); | ||
| } | ||
| referenceFinder.apply(analyze); | ||
|
|
There was a problem hiding this comment.
Why not simply:
group.lowerBound.accept(referenceFinder);
group.upperBound.accept(referenceFinder);
There was a problem hiding this comment.
It doesn't work, since the RexWindowBound does not derive from RexNode.
There was a problem hiding this comment.
RexWindowBound accepts RexVisitor. In your case, if its a bounded window bound, it should take care of visiting the offset (also offset is not nullable). See this: https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/rex/RexWindowBounds.java#L181
There was a problem hiding this comment.
This seems to work in this case.
The style of visitors I am used to requires always the entry point to be a visitor apply function, and not an accept function of the visited object. This is because the visitor may setup and teardown some data structures on apply.
A quick look through the codebase shows lots of accept calls, so I will use your suggestion.
There was a problem hiding this comment.
Because this is such a small change I have updated my previous commit instead of creating a new one.
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
mihaibudiu
force-pushed
the
issue6746
branch
from
6a5780a to
2bf0941
Compare
|
Where should I add the description? In the change or in the test? |
mihaibudiu
added
the
LGTM-will-merge-soon
You can update the description from github. Scroll to the top and you'll see: |
|
The visitor that keep track of the used fields did not account for the window bounds.