-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change ExecutionPlan::maintains_input_order to return vector (to support multi children executors better) #5035
Change ExecutionPlan::maintains_input_order to return vector (to support multi children executors better) #5035
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @mustafasrepo -- this is very exciting. There is one plan that doesn't look right to me and I have some other minor comments. I also plan to run the IOx tests with this change to see if ours come up with anything
FWIW I ran the IOx tests on the changes from this branch https://github.com/influxdata/influxdb_iox/pull/6681 and they seem to have passed 👍 |
Thanks for the review @alamb, we incorporated your suggestions. I am excited about this change, it paves the way to many more optimizations. |
There seems to be a CI failure ( |
Thanks @mustafasrepo and @ozankabak |
FYI any "no space left on device" CI errors should be fixed by merging up from master to get #5047 |
Done, feel free to merge whenever you like. |
Thanks -- I plan to merge this tomorrow so that @mingmwang / @yahoNanJing can review if they want |
Thanks again @mustafasrepo and @ozankabak |
Benchmark runs are scheduled for baseline = 2aa1490 and contender = 4a33f27. 4a33f27 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Sorry for the late response. I just come back from the holiday. |
Which issue does this PR close?
Closes #4980
Rationale for this change
Currently the
maintains_input_order
function returns abool
value indicating whether the executor maintains ordering. Obviously, for executors with multiple inputs, this incurs information loss -- which input ordering are we talking about? The best we can do is to assign a special meaning like any or all to this function, and this is enough for some executors. Obviously, there are (and will be) executors where this is insufficient.What changes are included in this PR?
In this PR, we change/generalize the signature of this function to properly support multi-children executors. The initial use case we have is the
UnionExec
implementation -- it will henceforth produce anoutput_ordering
not only when input orderings are strictly equal, but when there is a subset ordering that satisfy all input orderings.More use cases that will leverage this change are also on the way, such as:
Are these changes tested?
New tests are added to test the change.
Are there any user-facing changes?
This PR introduces an API change.
api change