Enable reordering of window operators #16482
Conversation
sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java
Outdated
Show resolved
Hide resolved
| * this allows us to sort the window groups in order to optimise the order of operators we would need to compute | ||
| * without losing the aggregate column name information (which is part of the computed WindowOperatorFactory) | ||
| */ | ||
| private static class WindowGroupProcessorWrapper implements Comparable<WindowGroupProcessorWrapper> |
There was a problem hiding this comment.
instead of implementing Comparable - a static field can be added to define the comparision logic ; that will provide a place to name the ordering ; and also a place to put usefull notes into its apidocs
There was a problem hiding this comment.
Modified. Now we can have multiple comparators.
I thought that we would not be requiring multiple, at the end we might have to only use the comparator that would optimise us to use minimal operators.
| // Need to work on this method to optimise the order in which we need to process based on the partitions | ||
| // currently just moves the empty windows to the front |
There was a problem hiding this comment.
it moves the empty to the front; however the order of the remaining elements are undefined - I wonder if that could cause different behaviour between jvms
something like a move-to-front logic might not have that problem
There was a problem hiding this comment.
the order of remaining elements would be as is, since in all other cases we return 0 to look like they are same. Would that not be sufficient?
| - type: "naivePartition" | ||
| partitionColumns: [ ] |
There was a problem hiding this comment.
not necessarily right now - but I think a check should be added to the NaivePartitioningOperator:
- if the
partitioningColumnsis empty - it may only get at most
1RACs
I wonder if we've already violated that or not - but probably not...
| * this allows us to sort the window groups in order to optimise the order of operators we would need to compute | ||
| * without losing the aggregate column name information (which is part of the computed WindowOperatorFactory) | ||
| */ | ||
| private static class WindowGroupProcessorWrapper implements Comparable<WindowGroupProcessorWrapper> |
There was a problem hiding this comment.
I would really like to suggest a better name for it - but don't really have any good suggestions.... WindowedCalculation ?
There was a problem hiding this comment.
I changed it to WindowComputationProcessor. let me know your thoughts on this name
github-actions
bot
added
Area - Batch Ingestion
Area - MSQ
| * Comparator on {@link WindowComputationProcessor} | ||
| * to move the empty windows to the front | ||
| */ | ||
| private static final Comparator<WindowComputationProcessor> MOVE_EMPTY_GROUPS_FIRST = (o1, o2) -> { |
There was a problem hiding this comment.
I think usually its more practical to have the comparator inside the class it belongs to
| /** | ||
| * A wrapper class which stores {@link WindowGroup} | ||
| * along with its computed {@link WindowOperatorFactory} | ||
| * <p> | ||
| * this allows us to sort the window groups in order to optimise the order of operators we would need to compute | ||
| * without losing the aggregate column name information (which is part of the computed WindowOperatorFactory) | ||
| */ |
There was a problem hiding this comment.
I don't feel like this apidoc adds much detail - I believe it would be ok even without it.
Usually I think its better to have the apidoc describe the role or mission of the class - I believe that adding classes like this are part of an emerging architecture.
Description
This PR aims to enable the re-ordering of window operators in order to optimise the sort and partition operators.
Example :
SELECT m1, m2, SUM(m1) OVER(PARTITION BY m2) as sum1, SUM(m2) OVER() as sum2 from numFoo GROUP BY m1,m2In order to compute this query, we can order the operators as to first compute the operators corresponding to sum2 and then place the operators corresponding to sum1 which would help us in reducing one sort operator if we order our operators by sum1 and then sum2.
Release note
Key changed/added classes in this PR
WindowingThis PR has: