[GitHub] [calcite] julianhyde commented on a diff in pull request #2868: [CALCITE-5230] Fix PERCENTILE_DISC return type derivation

[GitBox <gi...@apache.org>]

julianhyde commented on code in PR #2868:
URL: https://github.com/apache/calcite/pull/2868#discussion_r985305240


##########
core/src/main/java/org/apache/calcite/sql/SqlWithinGroupOperator.java:
##########
@@ -79,7 +81,56 @@ public SqlWithinGroupOperator() {
       SqlValidator validator,
       SqlValidatorScope scope,
       SqlCall call) {
-    // Validate type of the inner aggregate call
-    return validateOperands(validator, scope, call);
+
+    SqlCall inner = call.operand(0);
+    throwIfNotAggregate(call, validator, inner.getOperator());
+    if (inner.getOperator().getKind() == SqlKind.PERCENTILE_DISC) {
+      // We first check the percentile call operands, and then derive the correct type using
+      // PercentileDiscCallBinding (See CALCITE-5230).
+      SqlCallBinding opBinding =
+          new PercentileDiscCallBinding(validator, scope, inner, getCollationColumn(call));
+      inner.getOperator().checkOperandTypes(opBinding, true);
+      RelDataType ret = inner.getOperator().inferReturnType(opBinding);
+      validator.setValidatedNodeType(inner, ret);
+      return ret;
+    } else {
+      return validateOperands(validator, scope, call);
+    }
+  }

Review Comment:
   There's a lot of duplicated code here. Maybe if you replace `new SqlCallBinding(validator, scope, call)` in the base method with a protected method, you can save all that duplication?



##########
core/src/main/java/org/apache/calcite/sql/SqlWithinGroupOperator.java:
##########
@@ -79,7 +81,56 @@ public SqlWithinGroupOperator() {
       SqlValidator validator,
       SqlValidatorScope scope,
       SqlCall call) {
-    // Validate type of the inner aggregate call
-    return validateOperands(validator, scope, call);
+
+    SqlCall inner = call.operand(0);
+    throwIfNotAggregate(call, validator, inner.getOperator());
+    if (inner.getOperator().getKind() == SqlKind.PERCENTILE_DISC) {
+      // We first check the percentile call operands, and then derive the correct type using
+      // PercentileDiscCallBinding (See CALCITE-5230).
+      SqlCallBinding opBinding =
+          new PercentileDiscCallBinding(validator, scope, inner, getCollationColumn(call));
+      inner.getOperator().checkOperandTypes(opBinding, true);
+      RelDataType ret = inner.getOperator().inferReturnType(opBinding);
+      validator.setValidatedNodeType(inner, ret);
+      return ret;
+    } else {
+      return validateOperands(validator, scope, call);
+    }
+  }
+
+  private SqlNode getCollationColumn(SqlCall call) {
+    return ((SqlNodeList) call.operand(1)).get(0);
+  }
+
+  private void throwIfNotAggregate(
+      SqlCall call,
+      SqlValidator validator,
+      SqlOperator operator
+  ) {
+    if (!operator.isAggregator()) {
+      throw validator.newValidationError(call,
+          RESOURCE.withinGroupNotAllowed(
+              operator.getName()));
+    }
+  }

Review Comment:
   I would remove the `throwIfNotAggregate` function. Methods that always throw are confusing to the reader, and leave regions of code that the compiler doesn't know are unreachable.



##########
core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java:
##########
@@ -988,4 +990,15 @@ public static SqlCall stripSeparator(SqlCall call) {
       return relDataType;
     }
   };
+
+  public static final SqlReturnTypeInference PERCENTILE_DISC = opBinding -> {
+    if (opBinding instanceof SqlWithinGroupOperator.PercentileDiscCallBinding) {

Review Comment:
   This 'if .. instanceof ... else if ... instanceof' pattern isn't good. How about adding a method `getCollationExpression()` to `SqlCallBinding`, with a default implementation that throws. Then you can remove both `getCollationType` methods.



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org