You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/08/25 03:42:41 UTC
[GitHub] [druid] clintropolis opened a new pull request, #12968: json_value adjustments
clintropolis opened a new pull request, #12968:
URL: https://github.com/apache/druid/pull/12968
### Description
This PR makes some improvements to `JSON_VALUE` behavior, which fixes some bugs and inconsistency in behavior between the nested-field virtual column and the native `json_value` expression.
The first change is that the native `json_value` expression now has optional 3rd argument to specify type, which will cast all values to the specified type. This brings its behavior inline with `NestedFieldVirtualColumn`, whose selectors will cast values to the type specified by `expectedType`, regardless of the type of the underlying literal.
In attempting to wire this up, I realized how strangely I had `JSON_VALUE` wired up in the SQL layer. Prior to this PR, `JsonValueOperatorConversion` was wired up in the operator table as `JSON_VALUE`, and `JsonValueAnyOperatorConversion` was added as an alias to define an undocumented `JSON_VALUE_ANY` function (which is still undocumented, it doesn't really seem intended for direct use). The `JSON_VALUE_ANY` is there because the `StandardConvertletTable` has a rule that translates `JSON_VALUE(... RETURNING type)` into `CAST(JSON_VALUE_ANY(...) AS type)`. In effect, `JsonValueOperatorConversion` was only used indirectly via `JsonValueAnyOperatorConversion` via the convertlet, since it would be translated prior to translating to native query components.
This standard convertlet behavior isn't super useful because it means our operator conversion loses the type of the 'returning' which is now part of the outer cast, making it difficult to actually use the new type argument for the native expression.
Taking inspiration from the standard convertlet table behavior, I've added `DruidJsonValueConvertletFactory` to do this conversion ourselves, but more appropriate for our needs. This convertlet checks the type of the `RETURNING` argument, if present, and uses new internal only operators to handle type specific casts, `JsonValueBigintOperatorConversion`, `JsonValueDoubleOperatorConversion`, and `JsonValueVarcharOperatorConversion`, and changes `JsonValueAnyOperatorConversion` into a real conversion, taking the place of `JsonValueOperatorConversion` which is now removed because it is now handled by the convertlet. These internal functions will all remain undocumented for now at least.
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r955527392
##########
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:
oops, yes
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r955527046
##########
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:
It was documenting the old type syntax, which still works but shouldn't be encouraged I think, so i updated it
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r955617318
##########
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:
I think i've addressed everything except for this and finding a way to not have the internal functions directly callable. Since it isn't a new behavior that we ignore these parameters, does this need to block this PR?
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r955620981
##########
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:
OK, got it. I think it's fine if these functions remain accessible, in that case.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r955562617
##########
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:
yeah, `get_path` is sort of a relic from my earliest prototypes, and used to look a fair bit different (and only accepted 'jq' style paths), but does look the same now. Will consolidate it. Maybe we need an easy way to alias native expressions like we have for SQL expressions... I have a similar situation going on with `json_object` which is a wrapper around the earlier prototype `struct` function.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r955542934
##########
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:
well, i did it in the wrong PR, but added some tests to make sure i knew what i was talking about https://github.com/apache/druid/pull/12971/files#diff-8f872e230b0833df3a50ab4c9ef8283346e78fff991e0c3e53beb693329679d7R371
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r955620246
##########
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:
👍
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r955544274
##########
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:
I couldn't figure out a way to make it work, but it feels like something that should be possible....
```
org.apache.druid.sql.calcite.planner.UnsupportedSQLQueryException: Cannot build plan for query. Possible error: SQL query requires 'JSON_VALUE_DOUBLE' operator that is not supported.
``` is what happens if i remove them for a query using `JSON_VALUE`
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r955527222
##########
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:
no reason, will change :+1:
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r955528783
##########
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:
I couldn't find an easy way, but it is certainly possible that I didn't try everything yet. The reason being afaict the parser creates default values for all of these arguments when it is making the sql function call, and so it always first appears with 6 or 7 arguments and i wasn't too sure how to distinguish them explicitly being there from them being generated.
I can play around a bit to see if I can detect it, or at least maybe expect the default values that are generated and throw an error if they do not exist
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r955621136
##########
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:
👍
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r955544274
##########
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:
I couldn't figure out a way to make it work, but it feels like something that should be possible....
```
org.apache.druid.sql.calcite.planner.UnsupportedSQLQueryException: Cannot build plan for query. Possible error: SQL query requires 'JSON_VALUE_DOUBLE' operator that is not supported.
```
is what happens if i remove them for a query using `JSON_VALUE`
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r955620788
##########
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:
It doesn't need to block this PR. I'd encourage it as a follow up.
Btw, the "expect the default parameters" thing is what I was imagining. That way, if someone explicitly specifies the default, it still works fine. But if they try to use a different option, they get an error.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r955620096
##########
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:
Since we haven't done a release yet with this functionality, would it make sense to remove some of these functions? Are they going to be "born obsolete"?
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r956462642
##########
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:
ended up adding checks in this PR that only the default parameters are allowed (which thankfully matches the behavior we have implemented)
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r955590803
##########
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:
oh this is a stale comment, from a way earlier prototype when i did in fact only store string columns lol. Will remove since it is no longer true.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #12968:
URL: https://github.com/apache/druid/pull/12968#discussion_r956552430
##########
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:
Yeah that's a good point. I think i'll remove them. The only one I'm a bit partial to keeping is `struct` since longer term I sometimes imagine replacing `COMPLEX<json>` with `STRUCT` and `VARIANT` types so that the engine can be integrated a bit tighter with nested data columns. But, I can add it back whenever we get to that point, so will remove it too for now.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm merged pull request #12968: json_value adjustments
Posted by GitBox <gi...@apache.org>.
gianm merged PR #12968:
URL: https://github.com/apache/druid/pull/12968
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org