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: [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]
