Repository navigation
[CALCITE-4597] Allow RelNodes to have an empty row type (zero fields) - #5274
Conversation
|
|
||
| /** Adds a relational expression to the stack, checking the | ||
| * {@link Config#emptyRowTypePolicy()}. */ | ||
| private void stackPush(RelNode rel, |
There was a problem hiding this comment.
Changing every stack.push call site to stackPush seems fragile. Instead, could you change final Deque<Frame> stack to final Queue<Frame> stack, and assign to it an anonymous subclass of AbstractQueue<Frame> that checks for field-less RelNodes?
There was a problem hiding this comment.
Thank you for the suggestion, @julianhyde done in the latest commit.
|
@julianhyde Could you help to review this pr again when you have a time, Thanks! |
| * {@link #build()} to pop those inputs. */ | ||
| public RelBuilder push(RelNode node) { | ||
| stack.push(new Frame(node)); | ||
| stack.add(new Frame(node)); |
There was a problem hiding this comment.
changing this API seems dangerous. Why do you need to do it?
There was a problem hiding this comment.
It was my suggestion to convert stack from an ArrayDeque to a custom queue implementation. It will allow most code to continue unchanged.
This particular line of code could and should remain stack.push(new Frame(node)).
There was a problem hiding this comment.
Apologies for the delay in handling this(I was on vacation before).
Nice catch, that was unintended. All call sites are now back to the original stack.push() / stack.pop(). FrameStackprovides those methods( delegating to a backing ArrayDeque), so no call site changes remain; the only deleted line in RelBuilder is the old field declaration. cc @mihaibudiu @julianhyde
| protected final @Nullable RelOptSchema relOptSchema; | ||
| private final Deque<Frame> stack = new ArrayDeque<>(); | ||
| /** Stack of relational expressions that are being built. | ||
| * |
There was a problem hiding this comment.
This anonymous class is rather verbose to be in the section that declares fields. Make it a named inner class and move it towards the end of the file.
There was a problem hiding this comment.
Done, the anonymous class is now a named inner class, FrameStack, placed at the end of the file next to Frame. The check still happens in offer(), which push() delegates to.
|
Since it was approved and all comments should had been addressed , if there no other comments I would squash and merge it in coming days. |
7fae8c3 to
f773077
Compare
|
3 participants
jira: https://issues.apache.org/jira/browse/CALCITE-4597