FrankChen021 commented on code in PR #20782:
URL: https://github.com/apache/druid/pull/20782#discussion_r4434266178
##########
extensions-contrib/tdigestsketch/src/main/java/org/apache/druid/query/aggregation/tdigestsketch/sql/TDigestGenerateSketchSqlAggregator.java:
##########
@@ -154,7 +154,9 @@ private static class TDigestGenerateSketchSqlAggFunction
extends SqlAggFunction
DefaultOperandTypeChecker
.builder()
.operandNames("column", "compression")
- .operandTypes(SqlTypeFamily.ANY, SqlTypeFamily.NUMERIC)
+ .operandTypes(SqlTypeFamily.NUMERIC, SqlTypeFamily.NUMERIC)
+ .operandComplexTypes(0, TDigestSketchAggregatorFactory.TYPE)
+ .coercibleOperands(0)
Review Comment:
I think this declaration of operand type makes it not intutive because the
the operand type of 1st operand is declared via 3 builder API calls.
I'm thinking about sth like this:
1. caller side:
```java
.operandTypes(
OperandType.numeric()
.orComplex(TDigestSketchAggregatorFactory.TYPE)
.coerceStrings(),
OperandType.of(SqlTypeFamily.NUMERIC)
)
```
2. bulider side:
```java
// adding a new overlod, and keep the operands in the builder
public Builder operandTypes(final Operand... operands)
{
this.operands = Arrays.asList(operands);
return this;
}
// keep existing SqlTypeFamily based overloads of operandTypes, but delegate
to above one
public Builder operandTypes(final SqlTypeFamily... operandTypes)
{
return
operandTypes(Arrays.stream(operandTypes).map(Operand::of).toArray(Operand[]::new));
}
public Builder operandTypes(final List<SqlTypeFamily> operandTypes)
{
return
operandTypes(operandTypes.stream().map(Operand::of).toArray(Operand[]::new));
}
```
3. adding a new OperandType class
```java
public static final class Operand
{
private final SqlTypeFamily family;
private final Set<ColumnType> complexTypes;
private final boolean coerceStrings;
public static Operand of(SqlTypeFamily family) { ... }
public static Operand numeric()
{
return of(SqlTypeFamily.NUMERIC);
}
public Operand orComplex(ColumnType... types) { ... } // returns a new
Operand
public Operand coerceStrings()
{
if (family != SqlTypeFamily.NUMERIC) {
throw DruidException.defensive("coerceStrings() only applies to
NUMERIC operands");
}
...
}
@Override
public String toString() // used by getAllowedSignatures
{
// e.g. "NUMERIC | COMPLEX<tdigest>"
}
}
```
By this way, the operand type declaration is clear and intuitive, and your
existing checking logic is almost the same
--
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]
