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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
