kgyrtkirk commented on code in PR #16482:
URL: https://github.com/apache/druid/pull/16482#discussion_r1608974070
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java:
##########
@@ -270,6 +278,74 @@ public static Windowing fromCalciteStuff(
}
}
+ /**
+ * A wrapper object which stores {@link
org.apache.calcite.rel.core.Window.Group}
+ * along with its computed {@link WindowOperatorFactory}
+ *
+ * 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>
Review 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
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java:
##########
@@ -270,6 +278,74 @@ public static Windowing fromCalciteStuff(
}
}
+ /**
+ * A wrapper object which stores {@link
org.apache.calcite.rel.core.Window.Group}
+ * along with its computed {@link WindowOperatorFactory}
+ *
+ * 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>
+ {
+ private final Window.Group group;
+ private final OperatorFactory processorOperatorFactory;
+
+ public WindowGroupProcessorWrapper(Window.Group group, OperatorFactory
processorOperatorFactory)
+ {
+ this.group = group;
+ this.processorOperatorFactory = processorOperatorFactory;
+ }
+
+ public Window.Group getGroup()
+ {
+ return group;
+ }
+
+ public OperatorFactory getProcessorOperatorFactory()
+ {
+ return processorOperatorFactory;
+ }
+
+ @Override
+ public int compareTo(WindowGroupProcessorWrapper o)
+ {
+ // 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
Review 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
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java:
##########
@@ -270,6 +278,74 @@ public static Windowing fromCalciteStuff(
}
}
+ /**
+ * A wrapper object which stores {@link
org.apache.calcite.rel.core.Window.Group}
+ * along with its computed {@link WindowOperatorFactory}
+ *
+ * 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>
Review Comment:
I would really like to suggest a better name for it - but don't really have
any good suggestions.... `WindowedCalculation` ?
##########
sql/src/test/resources/calcite/tests/window/WindowOpReorder.sqlTest:
##########
@@ -0,0 +1,51 @@
+type: "operatorValidation"
+
+sql: |
+ SELECT
+ m1,
+ m2,
+ SUM(m1) OVER(PARTITION BY m2) as sum1,
+ SUM(m2) OVER() as sum2
+ from druid.numfoo
+ GROUP BY m1,m2
+
+expectedOperators:
+ - type: "naivePartition"
+ partitionColumns: [ ]
Review Comment:
not necessarily right now - but I think a check should be added to the
`NaivePartitioningOperator`:
* if the `partitioningColumns` is empty
* it may only get at most `1` RACs
I wonder if we've already violated that or not - but probably not...
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java:
##########
@@ -232,10 +202,48 @@ public static Windowing fromCalciteStuff(
throw new ISE("No processors from Window[%s], why was this code
called?", window);
}
- ops.add(new WindowOperatorFactory(
+ wrapperObjs.add(new WindowGroupProcessorWrapper(windowGroup, new
WindowOperatorFactory(
Review Comment:
why not pass `group` instead of `windowGroup` ?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
