soumyava commented on code in PR #16063:
URL: https://github.com/apache/druid/pull/16063#discussion_r1520252343
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java:
##########
@@ -136,6 +136,24 @@ public void validateWindow(SqlNode windowOrId,
SqlValidatorScope scope, @Nullabl
);
}
+ boolean hasBounds = lowerBound != null || upperBound != null;
Review Comment:
Can we add a negative test here to indicate what kind of queries are we
restricting so that the user can get an idea. Also good candidate to indicate
in the docs as well as NTILE does not support framing
##########
sql/src/main/java/org/apache/druid/sql/calcite/rule/RewriteFirstValueLastValueRule.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.calcite.rule;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.rules.SubstitutionRule;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.rex.RexOver;
+import org.apache.calcite.rex.RexShuttle;
+import org.apache.calcite.rex.RexWindow;
+import org.apache.calcite.rex.RexWindowBound;
+import org.apache.calcite.sql.SqlAggFunction;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+
+import java.math.BigDecimal;
+import java.util.List;
+
+public class RewriteFirstValueLastValueRule extends RelOptRule implements
SubstitutionRule
+{
+ public RewriteFirstValueLastValueRule()
+ {
+ super(operand(RelNode.class, any()));
+ }
+
+ @Override
+ public void onMatch(RelOptRuleCall call)
+ {
+ final RelNode oldNode = call.rel(0);
+ final RewriteShuttle shuttle = new
RewriteShuttle(oldNode.getCluster().getRexBuilder());
+ final RelNode newNode = oldNode.accept(shuttle);
+
+ // noinspection ObjectEquality
+ if (newNode != oldNode) {
+ call.transformTo(newNode);
+ call.getPlanner().prune(oldNode);
+ }
+ }
+
+ private static class RewriteShuttle extends RexShuttle
+ {
+ private final RexBuilder rexBuilder;
+
+ public RewriteShuttle(RexBuilder rexBuilder)
+ {
+ this.rexBuilder = rexBuilder;
+ }
+
+ @Override
+ public RexNode visitOver(RexOver over)
+ {
+ SqlOperator operator = over.getOperator();
+ RexWindow window = over.getWindow();
+ RexWindowBound upperBound = window.getUpperBound();
+ RexWindowBound lowerBound = window.getLowerBound();
+
+ if (window.orderKeys.size() > 0) {
+ if (operator.getKind() == SqlKind.LAST_VALUE &&
!upperBound.isUnbounded()) {
+ if (upperBound.isCurrentRow()) {
+ return rewriteToReferenceCurrentRow(over);
+ }
+ }
+ if (operator.getKind() == SqlKind.FIRST_VALUE &&
!lowerBound.isUnbounded()) {
+ if (lowerBound.isCurrentRow()) {
+ return rewriteToReferenceCurrentRow(over);
+ }
+ }
+ }
+ return super.visitOver(over);
+ }
+
+ private RexNode rewriteToReferenceCurrentRow(RexOver over)
+ {
+ // could remove `last_value( x ) over ( .... order by y )`
+ // best would be to: return over.getOperands().get(0);
+ // however that make some queries too good
Review Comment:
What does this comment mean by makig queries too good ?
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java:
##########
@@ -64,11 +63,7 @@ public class CalciteWindowQueryTest extends
BaseCalciteQueryTest
public static final boolean DUMP_ACTUAL_RESULTS = Boolean.parseBoolean(
System.getProperty("druid.tests.sql.dumpActualResults")
- );
-
- static {
- NullHandling.initializeForTests();
- }
+ ) || developerIDEdetected();
Review Comment:
Why is this change needed ? Is there a special handling only for eclipse ?
##########
sql/src/main/java/org/apache/druid/sql/calcite/rule/RewriteFirstValueLastValueRule.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.calcite.rule;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.rules.SubstitutionRule;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.rex.RexOver;
+import org.apache.calcite.rex.RexShuttle;
+import org.apache.calcite.rex.RexWindow;
+import org.apache.calcite.rex.RexWindowBound;
+import org.apache.calcite.sql.SqlAggFunction;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+
+import java.math.BigDecimal;
+import java.util.List;
+
+public class RewriteFirstValueLastValueRule extends RelOptRule implements
SubstitutionRule
+{
+ public RewriteFirstValueLastValueRule()
+ {
+ super(operand(RelNode.class, any()));
+ }
+
+ @Override
+ public void onMatch(RelOptRuleCall call)
+ {
+ final RelNode oldNode = call.rel(0);
+ final RewriteShuttle shuttle = new
RewriteShuttle(oldNode.getCluster().getRexBuilder());
+ final RelNode newNode = oldNode.accept(shuttle);
+
+ // noinspection ObjectEquality
+ if (newNode != oldNode) {
+ call.transformTo(newNode);
+ call.getPlanner().prune(oldNode);
+ }
+ }
+
+ private static class RewriteShuttle extends RexShuttle
+ {
+ private final RexBuilder rexBuilder;
+
+ public RewriteShuttle(RexBuilder rexBuilder)
+ {
+ this.rexBuilder = rexBuilder;
+ }
+
+ @Override
+ public RexNode visitOver(RexOver over)
+ {
+ SqlOperator operator = over.getOperator();
+ RexWindow window = over.getWindow();
+ RexWindowBound upperBound = window.getUpperBound();
+ RexWindowBound lowerBound = window.getLowerBound();
+
+ if (window.orderKeys.size() > 0) {
+ if (operator.getKind() == SqlKind.LAST_VALUE &&
!upperBound.isUnbounded()) {
+ if (upperBound.isCurrentRow()) {
+ return rewriteToReferenceCurrentRow(over);
+ }
+ }
+ if (operator.getKind() == SqlKind.FIRST_VALUE &&
!lowerBound.isUnbounded()) {
+ if (lowerBound.isCurrentRow()) {
+ return rewriteToReferenceCurrentRow(over);
+ }
+ }
+ }
+ return super.visitOver(over);
+ }
+
+ private RexNode rewriteToReferenceCurrentRow(RexOver over)
+ {
+ // could remove `last_value( x ) over ( .... order by y )`
+ // best would be to: return over.getOperands().get(0);
+ // however that make some queries too good
+ return makeOver(
+ over,
+ over.getWindow(),
+ SqlStdOperatorTable.LAG,
Review Comment:
If the operator.getKind() is LAST_VALUE we transform to LAG(x,0) but if the
operator.getKind() is FIRST_VALUE do we need to transform to LEAD(x,0) ?
Probably a comment can help here
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java:
##########
@@ -330,6 +331,7 @@ private Program buildReductionProgram(final PlannerContext
plannerContext, final
// make it impossible to convert to COALESCE.
builder.addRuleInstance(new CaseToCoalesceRule());
builder.addRuleInstance(new CoalesceLookupRule());
+ builder.addRuleInstance(new RewriteFirstValueLastValueRule());
Review Comment:
It will be good to callout in the description as to what was the need for
this new rule. Maybe a simple example with LAST_VALUE and the reason that was
not working and how this would rewrite the same query to using a LAG
--
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]
