clintropolis commented on code in PR #16216: URL: https://github.com/apache/druid/pull/16216#discussion_r1543826889
########## sql/src/main/java/org/apache/druid/sql/calcite/expression/OperatorConversions.java: ########## @@ -779,4 +781,13 @@ public static DirectOperatorConversion druidUnaryDoubleFn(String sqlOperator, St druidFunctionName ); } + + public static SqlReturnTypeInference complexReturnTypeWithNullability(ColumnType columnType, boolean nullable) Review Comment: nit: there are some `SqlReturnTypeInference` defined in `Calcites`, maybe this should also live there (or those should live here?) ########## extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/theta/sql/ThetaSketchEstimateWithErrorBoundsOperatorConversion.java: ########## @@ -46,7 +46,8 @@ public class ThetaSketchEstimateWithErrorBoundsOperatorConversion implements Sql private static final SqlFunction SQL_FUNCTION = OperatorConversions .operatorBuilder(StringUtils.toUpperCase(FUNCTION_NAME)) .operandTypes(SqlTypeFamily.ANY, SqlTypeFamily.INTEGER) - .returnTypeNonNull(SqlTypeName.OTHER) + .returnTypeInference( + OperatorConversions.complexReturnTypeWithNullability(SketchModule.BUILD_TYPE, false)) Review Comment: i guess the changes to `ComplexSqlType` make this not really matter anymore, but technically `THETA_SKETCH` is the type string for what gets stored in columns and is associated with the serde that is common between build/merge (and the postagg the function maps to is actually using the merge type as its output type) also, nit formatting, trailing `)` should be on newline (though style stuff doesn't always flag this consistently). same formatting nit for other similar operator function changes ########## extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/quantiles/sql/DoublesSketchObjectSqlAggregator.java: ########## @@ -52,7 +52,8 @@ public class DoublesSketchObjectSqlAggregator implements SqlAggregator OperatorConversions.aggregatorBuilder(NAME) .operandNames("column", "k") .operandTypes(SqlTypeFamily.ANY, SqlTypeFamily.EXACT_NUMERIC) - .returnTypeNonNull(SqlTypeName.OTHER) + .returnTypeInference( + OperatorConversions.complexReturnTypeWithNullability(HllSketchModule.TYPE, false)) Review Comment: this should be `DoublesSketchModule.DOUBLES_SKETCH` i think? ########## extensions-core/datasketches/src/main/java/org/apache/druid/query/aggregation/datasketches/hll/sql/HllSketchEstimateWithErrorBoundsOperatorConversion.java: ########## @@ -47,7 +47,8 @@ public class HllSketchEstimateWithErrorBoundsOperatorConversion implements SqlOp .operatorBuilder(StringUtils.toUpperCase(FUNCTION_NAME)) .operandTypes(SqlTypeFamily.ANY, SqlTypeFamily.INTEGER) .requiredOperandCount(1) - .returnTypeNonNull(SqlTypeName.OTHER) + .returnTypeInference( Review Comment: this function returns `ARRAY<DOUBLE>` as far as I can tell. I think there is a `.returnTypeNullableArrayWithNullableElements` that can be used instead of `.returnTypeInference` -- 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...@druid.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org