gianm commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r955517310
##########
docs/misc/math-expr.md:
##########
@@ -63,7 +63,7 @@ The following built-in functions are available.
|name|description|
|----|-----------|
-|cast|cast(expr,'LONG' or 'DOUBLE' or 'STRING' or 'LONG_ARRAY', or
'DOUBLE_ARRAY' or 'STRING_ARRAY') returns expr with specified type. exception
can be thrown. Scalar types may be cast to array types and will take the form
of a single element list (null will still be null). |
+|cast|cast(expr,'LONG' or 'DOUBLE' or 'STRING' or 'ARRAY<LONG>', or
'ARRAY<DOUBLE>' or 'ARRAY<STRING>') returns expr with specified type. exception
can be thrown. Scalar types may be cast to array types and will take the form
of a single element list (null will still be null). |
Review Comment:
Is this a backwards compatibility issue (did something really change) or
fixing a doc bug?
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidOperatorTable.java:
##########
@@ -304,8 +304,10 @@ public class DruidOperatorTable implements SqlOperatorTable
.add(new
NestedDataOperatorConversions.JsonKeysOperatorConversion())
.add(new
NestedDataOperatorConversions.JsonPathsOperatorConversion())
.add(new
NestedDataOperatorConversions.JsonQueryOperatorConversion())
- .add(new
NestedDataOperatorConversions.JsonValueOperatorConversion())
.add(new
NestedDataOperatorConversions.JsonValueAnyOperatorConversion())
+ .add(new
NestedDataOperatorConversions.JsonValueBigintOperatorConversion())
+ .add(new
NestedDataOperatorConversions.JsonValueDoubleOperatorConversion())
+ .add(new
NestedDataOperatorConversions.JsonValueStringOperatorConversion())
Review Comment:
Do these need to be registered with the operator table in order for the
convertlet to return them? Asking since, if possible, it'd be nice if end users
couldn't write these functions directly.
Not a requirement, I suppose. If they have to be here then it's fine.
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/NestedDataOperatorConversions.java:
##########
@@ -395,24 +465,182 @@ public DruidExpression toDruidExpression(
)
);
}
- return DruidExpression.ofExpression(ColumnType.STRING, builder,
druidExpressions);
+ return DruidExpression.ofExpression(druidType, builder,
druidExpressions);
+ }
+
+ static SqlFunction buildFunction(String functionName, SqlTypeName typeName)
+ {
+ return OperatorConversions.operatorBuilder(functionName)
+ .operandTypeChecker(
+ OperandTypes.sequence(
+ "(expr,path)",
+ OperandTypes.family(SqlTypeFamily.ANY),
+
OperandTypes.family(SqlTypeFamily.STRING)
+ )
+ )
+ .returnTypeInference(
+ ReturnTypes.cascade(
+ opBinding ->
opBinding.getTypeFactory().createSqlType(typeName),
+ SqlTypeTransforms.FORCE_NULLABLE
+ )
+ )
+
.functionCategory(SqlFunctionCategory.USER_DEFINED_FUNCTION)
+ .build();
+ }
+ }
+
+ public static class JsonValueBigintOperatorConversion extends
JsonValueReturningTypeOperatorConversion
+ {
+ private static final SqlFunction FUNCTION =
buildFunction("JSON_VALUE_BIGINT", SqlTypeName.BIGINT);
+
+ public JsonValueBigintOperatorConversion()
+ {
+ super(FUNCTION, ColumnType.LONG);
+ }
+ }
+
+ public static class JsonValueDoubleOperatorConversion extends
JsonValueReturningTypeOperatorConversion
+ {
+ private static final SqlFunction FUNCTION =
buildFunction("JSON_VALUE_DOUBLE", SqlTypeName.DOUBLE);
+
+ public JsonValueDoubleOperatorConversion()
+ {
+ super(FUNCTION, ColumnType.DOUBLE);
}
}
- // calcite converts JSON_VALUE to JSON_VALUE_ANY so we have to wire that up
too...
- public static class JsonValueAnyOperatorConversion extends
AliasedOperatorConversion
+ public static class JsonValueStringOperatorConversion extends
JsonValueReturningTypeOperatorConversion
{
- private static final String FUNCTION_NAME =
StringUtils.toUpperCase("json_value_any");
+ private static final SqlFunction FUNCTION =
buildFunction("JSON_VALUE_VARCHAR", SqlTypeName.VARCHAR);
- public JsonValueAnyOperatorConversion()
+ public JsonValueStringOperatorConversion()
{
- super(new JsonValueOperatorConversion(), FUNCTION_NAME);
+ super(FUNCTION, ColumnType.STRING);
}
+ }
+
+ /**
+ * Calcites {@link org.apache.calcite.sql2rel.StandardConvertletTable}
translates JSON_VALUE
Review Comment:
Incomplete comment?
##########
processing/src/main/java/org/apache/druid/query/expression/NestedDataExpressions.java:
##########
@@ -442,38 +442,76 @@ public String name()
public Expr apply(List<Expr> args)
{
final List<NestedPathPart> parts =
getArg1JsonPathPartsFromLiteral(name(), args);
- class JsonValueExpr extends ExprMacroTable.BaseScalarMacroFunctionExpr
- {
- public JsonValueExpr(List<Expr> args)
- {
- super(name(), args);
+ if (args.size() == 3 && args.get(2).isLiteral()) {
+ final ExpressionType castTo = ExpressionType.fromString((String)
args.get(2).getLiteralValue());
+ if (castTo == null) {
+ throw new IAE("Invalid output type: [%s]",
args.get(2).getLiteralValue());
}
-
- @Override
- public ExprEval eval(ObjectBinding bindings)
+ class CastJsonValueExpr extends
ExprMacroTable.BaseScalarMacroFunctionExpr
{
- ExprEval input = args.get(0).eval(bindings);
- return ExprEval.bestEffortOf(
- NestedPathFinder.findLiteral(unwrap(input), parts)
- );
- }
+ public CastJsonValueExpr(List<Expr> args)
+ {
+ super(name(), args);
+ }
- @Override
- public Expr visit(Shuttle shuttle)
- {
- List<Expr> newArgs = args.stream().map(x ->
x.visit(shuttle)).collect(Collectors.toList());
- return shuttle.visit(new JsonValueExpr(newArgs));
- }
+ @Override
+ public ExprEval eval(ObjectBinding bindings)
+ {
+ ExprEval input = args.get(0).eval(bindings);
+ return ExprEval.bestEffortOf(
+ NestedPathFinder.findLiteral(unwrap(input), parts)
+ ).castTo(castTo);
+ }
- @Nullable
- @Override
- public ExpressionType getOutputType(InputBindingInspector inspector)
+ @Override
+ public Expr visit(Shuttle shuttle)
+ {
+ List<Expr> newArgs = args.stream().map(x ->
x.visit(shuttle)).collect(Collectors.toList());
+ return shuttle.visit(new CastJsonValueExpr(newArgs));
+ }
+
+ @Nullable
+ @Override
+ public ExpressionType getOutputType(InputBindingInspector inspector)
+ {
+ return castTo;
+ }
+ }
+ return new CastJsonValueExpr(args);
+ } else {
+ class JsonValueExpr extends ExprMacroTable.BaseScalarMacroFunctionExpr
{
- // we cannot infer the output type (well we could say it is 'STRING'
right now because is all we support...
- return null;
+ public JsonValueExpr(List<Expr> args)
Review Comment:
Seems like the logic is the same between `get_path` and the no-cast version
of `json_value`. Can we share the code too? Maybe use the same static class for
both?
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/NestedDataOperatorConversions.java:
##########
@@ -395,24 +465,182 @@ public DruidExpression toDruidExpression(
)
);
}
- return DruidExpression.ofExpression(ColumnType.STRING, builder,
druidExpressions);
+ return DruidExpression.ofExpression(druidType, builder,
druidExpressions);
+ }
+
+ static SqlFunction buildFunction(String functionName, SqlTypeName typeName)
+ {
+ return OperatorConversions.operatorBuilder(functionName)
+ .operandTypeChecker(
+ OperandTypes.sequence(
+ "(expr,path)",
+ OperandTypes.family(SqlTypeFamily.ANY),
+
OperandTypes.family(SqlTypeFamily.STRING)
+ )
+ )
+ .returnTypeInference(
+ ReturnTypes.cascade(
+ opBinding ->
opBinding.getTypeFactory().createSqlType(typeName),
+ SqlTypeTransforms.FORCE_NULLABLE
+ )
+ )
+
.functionCategory(SqlFunctionCategory.USER_DEFINED_FUNCTION)
+ .build();
+ }
+ }
+
+ public static class JsonValueBigintOperatorConversion extends
JsonValueReturningTypeOperatorConversion
+ {
+ private static final SqlFunction FUNCTION =
buildFunction("JSON_VALUE_BIGINT", SqlTypeName.BIGINT);
+
+ public JsonValueBigintOperatorConversion()
+ {
+ super(FUNCTION, ColumnType.LONG);
+ }
+ }
+
+ public static class JsonValueDoubleOperatorConversion extends
JsonValueReturningTypeOperatorConversion
+ {
+ private static final SqlFunction FUNCTION =
buildFunction("JSON_VALUE_DOUBLE", SqlTypeName.DOUBLE);
+
+ public JsonValueDoubleOperatorConversion()
+ {
+ super(FUNCTION, ColumnType.DOUBLE);
}
}
- // calcite converts JSON_VALUE to JSON_VALUE_ANY so we have to wire that up
too...
- public static class JsonValueAnyOperatorConversion extends
AliasedOperatorConversion
+ public static class JsonValueStringOperatorConversion extends
JsonValueReturningTypeOperatorConversion
{
- private static final String FUNCTION_NAME =
StringUtils.toUpperCase("json_value_any");
+ private static final SqlFunction FUNCTION =
buildFunction("JSON_VALUE_VARCHAR", SqlTypeName.VARCHAR);
- public JsonValueAnyOperatorConversion()
+ public JsonValueStringOperatorConversion()
{
- super(new JsonValueOperatorConversion(), FUNCTION_NAME);
+ super(FUNCTION, ColumnType.STRING);
}
+ }
+
+ /**
+ * Calcites {@link org.apache.calcite.sql2rel.StandardConvertletTable}
translates JSON_VALUE
+ */
+ public static class JsonValueAnyOperatorConversion implements
SqlOperatorConversion
+ {
+ private static final SqlFunction FUNCTION =
+ OperatorConversions.operatorBuilder("JSON_VALUE_ANY")
+ .operandTypeChecker(
+ OperandTypes.or(
+ OperandTypes.sequence(
+ "(expr,path)",
+ OperandTypes.family(SqlTypeFamily.ANY),
+
OperandTypes.family(SqlTypeFamily.STRING)
+ ),
+ OperandTypes.family(
+ SqlTypeFamily.ANY,
+ SqlTypeFamily.CHARACTER,
+ SqlTypeFamily.ANY,
+ SqlTypeFamily.ANY,
+ SqlTypeFamily.ANY,
+ SqlTypeFamily.ANY,
+ SqlTypeFamily.ANY
+ )
+ )
+ )
+ .operandTypeInference((callBinding, returnType,
operandTypes) -> {
+ RelDataTypeFactory typeFactory =
callBinding.getTypeFactory();
+ if (operandTypes.length > 5) {
+ operandTypes[3] =
typeFactory.createSqlType(SqlTypeName.ANY);
+ operandTypes[5] =
typeFactory.createSqlType(SqlTypeName.ANY);
+ }
+ })
+ .returnTypeInference(
+ ReturnTypes.cascade(
+ opBinding ->
opBinding.getTypeFactory().createTypeWithNullability(
+ // STRING is the closest thing we have
to an ANY type
+ // however, this should really be using
SqlTypeName.ANY.. someday
+
opBinding.getTypeFactory().createSqlType(SqlTypeName.VARCHAR),
+ true
+ ),
+ SqlTypeTransforms.FORCE_NULLABLE
+ )
+ )
+ .functionCategory(SqlFunctionCategory.SYSTEM)
+ .build();
@Override
public SqlOperator calciteOperator()
{
- return SqlStdOperatorTable.JSON_VALUE_ANY;
+ return FUNCTION;
+ }
+
+ @Nullable
+ @Override
+ public DruidExpression toDruidExpression(
+ PlannerContext plannerContext,
+ RowSignature rowSignature,
+ RexNode rexNode
+ )
+ {
+ final RexCall call = (RexCall) rexNode;
+
+ // calcite parser can allow for a bunch of junk in here that we don't
care about right now, so the call looks
Review Comment:
Some of the extra parameters look like things that are meant to alter the
behavior of the function. (like controlling what the error behavior is.) If we
don't support them, better to throw an error than silently ignore them. Is this
feasible?
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/DruidConvertletTable.java:
##########
@@ -44,6 +45,7 @@ public class DruidConvertletTable implements
SqlRexConvertletTable
ImmutableList.<DruidConvertletFactory>builder()
.add(CurrentTimestampAndFriendsConvertletFactory.INSTANCE)
.add(TimeInIntervalConvertletFactory.INSTANCE)
+ .add(new
NestedDataOperatorConversions.DruidJsonValueConvertletFactory())
Review Comment:
Any reason this isn't in a static field like the other ones?
##########
processing/src/main/java/org/apache/druid/query/expression/NestedDataExpressions.java:
##########
@@ -442,38 +442,76 @@ public String name()
public Expr apply(List<Expr> args)
{
final List<NestedPathPart> parts =
getArg1JsonPathPartsFromLiteral(name(), args);
- class JsonValueExpr extends ExprMacroTable.BaseScalarMacroFunctionExpr
- {
- public JsonValueExpr(List<Expr> args)
- {
- super(name(), args);
+ if (args.size() == 3 && args.get(2).isLiteral()) {
+ final ExpressionType castTo = ExpressionType.fromString((String)
args.get(2).getLiteralValue());
+ if (castTo == null) {
+ throw new IAE("Invalid output type: [%s]",
args.get(2).getLiteralValue());
}
-
- @Override
- public ExprEval eval(ObjectBinding bindings)
+ class CastJsonValueExpr extends
ExprMacroTable.BaseScalarMacroFunctionExpr
{
- ExprEval input = args.get(0).eval(bindings);
- return ExprEval.bestEffortOf(
- NestedPathFinder.findLiteral(unwrap(input), parts)
- );
- }
+ public CastJsonValueExpr(List<Expr> args)
+ {
+ super(name(), args);
+ }
- @Override
- public Expr visit(Shuttle shuttle)
- {
- List<Expr> newArgs = args.stream().map(x ->
x.visit(shuttle)).collect(Collectors.toList());
- return shuttle.visit(new JsonValueExpr(newArgs));
- }
+ @Override
+ public ExprEval eval(ObjectBinding bindings)
+ {
+ ExprEval input = args.get(0).eval(bindings);
+ return ExprEval.bestEffortOf(
+ NestedPathFinder.findLiteral(unwrap(input), parts)
+ ).castTo(castTo);
+ }
- @Nullable
- @Override
- public ExpressionType getOutputType(InputBindingInspector inspector)
+ @Override
+ public Expr visit(Shuttle shuttle)
+ {
+ List<Expr> newArgs = args.stream().map(x ->
x.visit(shuttle)).collect(Collectors.toList());
+ return shuttle.visit(new CastJsonValueExpr(newArgs));
+ }
+
+ @Nullable
+ @Override
+ public ExpressionType getOutputType(InputBindingInspector inspector)
+ {
+ return castTo;
+ }
+ }
+ return new CastJsonValueExpr(args);
+ } else {
+ class JsonValueExpr extends ExprMacroTable.BaseScalarMacroFunctionExpr
{
- // we cannot infer the output type (well we could say it is 'STRING'
right now because is all we support...
- return null;
+ public JsonValueExpr(List<Expr> args)
+ {
+ super(name(), args);
+ }
+
+ @Override
+ public ExprEval eval(ObjectBinding bindings)
+ {
+ ExprEval input = args.get(0).eval(bindings);
+ return ExprEval.bestEffortOf(
+ NestedPathFinder.findLiteral(unwrap(input), parts)
+ );
+ }
+
+ @Override
+ public Expr visit(Shuttle shuttle)
+ {
+ List<Expr> newArgs = args.stream().map(x ->
x.visit(shuttle)).collect(Collectors.toList());
+ return shuttle.visit(new JsonValueExpr(newArgs));
+ }
+
+ @Nullable
+ @Override
+ public ExpressionType getOutputType(InputBindingInspector inspector)
+ {
+ // we cannot infer the output type (well we could say it is
'STRING' right now because is all we support...
Review Comment:
Then why not return STRING?
--
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]
