Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stricter behavior for parse_json, add try_parse_json, remove to_json #12920

Merged
merged 1 commit into from
Aug 23, 2022
Merged

stricter behavior for parse_json, add try_parse_json, remove to_json #12920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 81 additions & 64 deletions processing/src/main/java/org/apache/druid/query/expression/NestedDataExpressions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public ExprEval eval(ObjectBinding bindings)
ExprEval value = args.get(i + 1).eval(bindings);

Preconditions.checkArgument(field.type().is(ExprType.STRING), "field name must be a STRING");
theMap.put(field.asString(), maybeUnwrapStructuredData(value.value()));
theMap.put(field.asString(), unwrap(value));
}

return ExprEval.ofComplex(TYPE, theMap);
Expand Down Expand Up @@ -115,9 +115,19 @@ public String name()
}
}

public static class ToJsonExprMacro implements ExprMacroTable.ExprMacro
public static class ToJsonStringExprMacro implements ExprMacroTable.ExprMacro
{
public static final String NAME = "to_json";
public static final String NAME = "to_json_string";

private final ObjectMapper jsonMapper;

@Inject
public ToJsonStringExprMacro(
@Json ObjectMapper jsonMapper
)
{
this.jsonMapper = jsonMapper;
}

@Override
public String name()
Expand All @@ -128,9 +138,9 @@ public String name()
@Override
public Expr apply(List<Expr> args)
{
class ToJsonExpr extends ExprMacroTable.BaseScalarMacroFunctionExpr
class ToJsonStringExpr extends ExprMacroTable.BaseScalarMacroFunctionExpr
{
public ToJsonExpr(List<Expr> args)
public ToJsonStringExpr(List<Expr> args)
{
super(name(), args);
}
Expand All @@ -139,38 +149,45 @@ public ToJsonExpr(List<Expr> args)
public ExprEval eval(ObjectBinding bindings)
{
ExprEval input = args.get(0).eval(bindings);
return ExprEval.ofComplex(
TYPE,
maybeUnwrapStructuredData(input)
);
try {
final Object unwrapped = unwrap(input);
final String stringify = unwrapped == null ? null : jsonMapper.writeValueAsString(unwrapped);
return ExprEval.ofType(
ExpressionType.STRING,
stringify
);
}
catch (JsonProcessingException e) {
throw new IAE(e, "Unable to stringify [%s] to JSON", input.value());
}
}

@Override
public Expr visit(Shuttle shuttle)
{
List<Expr> newArgs = args.stream().map(x -> x.visit(shuttle)).collect(Collectors.toList());
return shuttle.visit(new ToJsonExpr(newArgs));
return shuttle.visit(new ToJsonStringExpr(newArgs));
}

@Nullable
@Override
public ExpressionType getOutputType(InputBindingInspector inspector)
{
return TYPE;
return ExpressionType.STRING;
}
}
return new ToJsonExpr(args);
return new ToJsonStringExpr(args);
}
}

public static class ToJsonStringExprMacro implements ExprMacroTable.ExprMacro
public static class ParseJsonExprMacro implements ExprMacroTable.ExprMacro
{
public static final String NAME = "to_json_string";
public static final String NAME = "parse_json";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a doc update required for this ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, docs in #12922 were written as if this PR were already merged, (to_json_string still exists, but to_json is removed and try_parse_json is added, so this is a result of funny diff stuff i think)


private final ObjectMapper jsonMapper;

@Inject
public ToJsonStringExprMacro(
public ParseJsonExprMacro(
@Json ObjectMapper jsonMapper
)
{
Expand All @@ -186,56 +203,66 @@ public String name()
@Override
public Expr apply(List<Expr> args)
{
class ToJsonStringExpr extends ExprMacroTable.BaseScalarMacroFunctionExpr
class ParseJsonExpr extends ExprMacroTable.BaseScalarMacroFunctionExpr
{
public ToJsonStringExpr(List<Expr> args)
public ParseJsonExpr(List<Expr> args)
{
super(name(), args);
}

@Override
public ExprEval eval(ObjectBinding bindings)
{
ExprEval input = args.get(0).eval(bindings);
try {
final Object unwrapped = maybeUnwrapStructuredData(input);
final String stringify = unwrapped == null ? null : jsonMapper.writeValueAsString(unwrapped);
return ExprEval.ofType(
ExpressionType.STRING,
stringify
);
ExprEval arg = args.get(0).eval(bindings);
if (arg.value() == null) {
return ExprEval.ofComplex(TYPE, null);
}
catch (JsonProcessingException e) {
throw new IAE(e, "Unable to stringify [%s] to JSON", input.value());
if (arg.type().is(ExprType.STRING)) {
try {
return ExprEval.ofComplex(
TYPE,
jsonMapper.readValue(arg.asString(), Object.class)
);
}
catch (JsonProcessingException e) {
throw new IAE("Bad string input [%s] to [%s]", arg.asString(), name());
}
}
throw new IAE(
"Invalid input [%s] of type [%s] to [%s], expected [%s]",
arg.asString(),
arg.type(),
name(),
ExpressionType.STRING
);
}

@Override
public Expr visit(Shuttle shuttle)
{
List<Expr> newArgs = args.stream().map(x -> x.visit(shuttle)).collect(Collectors.toList());
return shuttle.visit(new ToJsonStringExpr(newArgs));
return shuttle.visit(new ParseJsonExpr(newArgs));
}

@Nullable
@Override
public ExpressionType getOutputType(InputBindingInspector inspector)
{
return ExpressionType.STRING;
return TYPE;
}
}
return new ToJsonStringExpr(args);
return new ParseJsonExpr(args);
}
}

public static class ParseJsonExprMacro implements ExprMacroTable.ExprMacro
public static class TryParseJsonExprMacro implements ExprMacroTable.ExprMacro
{
public static final String NAME = "parse_json";
public static final String NAME = "try_parse_json";

private final ObjectMapper jsonMapper;

@Inject
public ParseJsonExprMacro(
public TryParseJsonExprMacro(
@Json ObjectMapper jsonMapper
)
{
Expand All @@ -262,18 +289,23 @@ public ParseJsonExpr(List<Expr> args)
public ExprEval eval(ObjectBinding bindings)
{
ExprEval arg = args.get(0).eval(bindings);
Object parsed = maybeUnwrapStructuredData(arg);
if (arg.type().is(ExprType.STRING) && arg.value() != null && maybeJson(arg.asString())) {
if (arg.type().is(ExprType.STRING) && arg.value() != null) {
try {
parsed = jsonMapper.readValue(arg.asString(), Object.class);
return ExprEval.ofComplex(
TYPE,
jsonMapper.readValue(arg.asString(), Object.class)
);
}
catch (JsonProcessingException e) {
throw new IAE("Bad string input [%s] to [%s]", arg.asString(), name());
return ExprEval.ofComplex(
TYPE,
null
);
}
}
return ExprEval.ofComplex(
TYPE,
parsed
null
);
}

Expand Down Expand Up @@ -323,7 +355,7 @@ public ExprEval eval(ObjectBinding bindings)
{
ExprEval input = args.get(0).eval(bindings);
return ExprEval.bestEffortOf(
NestedPathFinder.findLiteral(maybeUnwrapStructuredData(input), parts)
NestedPathFinder.findLiteral(unwrap(input), parts)
);
}

Expand Down Expand Up @@ -373,7 +405,7 @@ public ExprEval eval(ObjectBinding bindings)
ExprEval input = args.get(0).eval(bindings);
return ExprEval.ofComplex(
TYPE,
NestedPathFinder.find(maybeUnwrapStructuredData(input), parts)
NestedPathFinder.find(unwrap(input), parts)
);
}

Expand Down Expand Up @@ -422,7 +454,7 @@ public ExprEval eval(ObjectBinding bindings)
{
ExprEval input = args.get(0).eval(bindings);
return ExprEval.bestEffortOf(
NestedPathFinder.findLiteral(maybeUnwrapStructuredData(input), parts)
NestedPathFinder.findLiteral(unwrap(input), parts)
);
}

Expand Down Expand Up @@ -479,7 +511,7 @@ public ListPathsExpr(List<Expr> args)
public ExprEval eval(ObjectBinding bindings)
{
ExprEval input = args.get(0).eval(bindings);
StructuredDataProcessor.ProcessResults info = processor.processFields(maybeUnwrapStructuredData(input));
StructuredDataProcessor.ProcessResults info = processor.processFields(unwrap(input));
return ExprEval.ofType(
ExpressionType.STRING_ARRAY,
ImmutableList.copyOf(info.getLiteralFields())
Expand Down Expand Up @@ -539,7 +571,7 @@ public ExprEval eval(ObjectBinding bindings)
{
ExprEval input = args.get(0).eval(bindings);
// maybe in the future ProcessResults should deal in PathFinder.PathPart instead of strings for fields
StructuredDataProcessor.ProcessResults info = processor.processFields(maybeUnwrapStructuredData(input));
StructuredDataProcessor.ProcessResults info = processor.processFields(unwrap(input));
List<String> transformed = info.getLiteralFields()
.stream()
.map(p -> NestedPathFinder.toNormalizedJsonPath(NestedPathFinder.parseJqPath(p)))
Expand Down Expand Up @@ -595,7 +627,7 @@ public ExprEval eval(ObjectBinding bindings)
ExprEval input = args.get(0).eval(bindings);
return ExprEval.ofType(
ExpressionType.STRING_ARRAY,
NestedPathFinder.findKeys(maybeUnwrapStructuredData(input), parts)
NestedPathFinder.findKeys(unwrap(input), parts)
);
}

Expand Down Expand Up @@ -630,21 +662,17 @@ public String name()
}

@Nullable
static Object maybeUnwrapStructuredData(ExprEval input)
static Object unwrap(ExprEval input)
{
return maybeUnwrapStructuredData(input.value());
return unwrap(input.value());
}

static Object maybeUnwrapStructuredData(Object input)
static Object unwrap(Object input)
{
if (input instanceof StructuredData) {
StructuredData data = (StructuredData) input;
return data.getValue();
}
if (input instanceof Object[]) {
return Arrays.stream((Object[]) input).map(x -> maybeUnwrapStructuredData(x)).toArray();
return Arrays.stream((Object[]) input).map(NestedDataExpressions::unwrap).toArray();
}
return input;
return StructuredData.unwrap(input);
}


Expand Down Expand Up @@ -684,15 +712,4 @@ static List<NestedPathPart> getArg1JsonPathPartsFromLiteral(String fnName, List<
);
return parts;
}

static boolean maybeJson(@Nullable String val)
{
if (val == null) {
return false;
}
if (val.isEmpty()) {
return false;
}
return val.startsWith("[") || val.startsWith("{") || val.startsWith("\"") || Character.isDigit(val.charAt(0));
}
}
4 changes: 2 additions & 2 deletions processing/src/main/java/org/apache/druid/segment/NestedDataDimensionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ public class NestedDataDimensionHandler implements DimensionHandler<StructuredDa
{
private static Comparator<ColumnValueSelector> COMPARATOR = (s1, s2) ->
StructuredData.COMPARATOR.compare(
StructuredData.possiblyWrap(s1.getObject()),
StructuredData.possiblyWrap(s2.getObject())
StructuredData.wrap(s1.getObject()),
StructuredData.wrap(s2.getObject())
);

private final String name;
Expand Down
2 changes: 1 addition & 1 deletion processing/src/main/java/org/apache/druid/segment/nested/NestedDataComplexTypeSerde.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ public ObjectStrategy getObjectStrategy()
public int compare(Object o1, Object o2)
{
return Comparators.<StructuredData>naturalNullsFirst()
.compare(StructuredData.possiblyWrap(o1), StructuredData.possiblyWrap(o2));
.compare(StructuredData.wrap(o1), StructuredData.wrap(o2));
}

@Override
Expand Down
11 changes: 10 additions & 1 deletion processing/src/main/java/org/apache/druid/segment/nested/StructuredData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,23 @@ private static long computeHash(StructuredData data)
}

@Nullable
public static StructuredData possiblyWrap(@Nullable Object value)
public static StructuredData wrap(@Nullable Object value)
{
if (value == null || value instanceof StructuredData) {
return (StructuredData) value;
}
return new StructuredData(value);
}

@Nullable
public static Object unwrap(@Nullable Object value)
{
if (value instanceof StructuredData) {
return ((StructuredData) value).getValue();
}
return value;
}

@JsonCreator
public static StructuredData create(Object value)
{
Expand Down
14 changes: 8 additions & 6 deletions processing/src/main/java/org/apache/druid/segment/nested/StructuredDataProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public abstract class StructuredDataProcessor
public ProcessResults processFields(Object raw)
{
Queue<Field> toProcess = new ArrayDeque<>();
raw = StructuredData.unwrap(raw);
if (raw instanceof Map) {
toProcess.add(new MapField("", (Map<String, ?>) raw));
} else if (raw instanceof List) {
Expand Down Expand Up @@ -76,15 +77,16 @@ private ProcessResults processMapField(Queue<Field> toProcess, MapField map)
// add estimated size of string key
processResults.addSize(estimateStringSize(entry.getKey()));
final String fieldName = map.getName() + ".\"" + entry.getKey() + "\"";
Object value = StructuredData.unwrap(entry.getValue());
// lists and maps go back in the queue
if (entry.getValue() instanceof List) {
List<?> theList = (List<?>) entry.getValue();
if (value instanceof List) {
List<?> theList = (List<?>) value;
toProcess.add(new ListField(fieldName, theList));
} else if (entry.getValue() instanceof Map) {
toProcess.add(new MapField(fieldName, (Map<String, ?>) entry.getValue()));
} else if (value instanceof Map) {
toProcess.add(new MapField(fieldName, (Map<String, ?>) value));
} else {
// literals get processed
processResults.addLiteralField(fieldName, processLiteralField(fieldName, entry.getValue()));
processResults.addLiteralField(fieldName, processLiteralField(fieldName, value));
}
}
return processResults;
Expand All @@ -97,7 +99,7 @@ private ProcessResults processListField(Queue<Field> toProcess, ListField list)
final List<?> theList = list.getList();
for (int i = 0; i < theList.size(); i++) {
final String listFieldName = list.getName() + "[" + i + "]";
final Object element = theList.get(i);
final Object element = StructuredData.unwrap(theList.get(i));
// maps and lists go back into the queue
if (element instanceof Map) {
toProcess.add(new MapField(listFieldName, (Map<String, ?>) element));
Expand Down
Loading