-
Notifications
You must be signed in to change notification settings - Fork 1k
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.
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
feat: Add is_json_string UDF #8600
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
e50f1ea
to
a7174e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
ksqldb-engine/src/main/java/io/confluent/ksql/function/udf/json/IsJsonString.java
Outdated
Show resolved
Hide resolved
ksqldb-engine/src/main/java/io/confluent/ksql/function/udf/json/IsJsonString.java
Outdated
Show resolved
Hide resolved
e3e5bf8
to
a37c305
Compare
ksqldb-engine/src/main/java/io/confluent/ksql/function/udf/json/UdfJsonMapper.java
Show resolved
Hide resolved
038574d
to
dfd1c82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one question in the check function, otherwise LGTM!
} catch (KsqlFunctionException e) { | ||
return false; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return true at the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please elaborate the reasoning behind your suggestion? As I see it, this function return true
only if the string was parsed as JSON without exception and the underlying node is not MissingNode
that represents broken JSON.
In any case, I think it is worth switching to a primitive type to signal that the return value is never null
- 2f556c9#diff-48af5e77f4b3c1e1ee5cb1d402f247c3e70b4649c1de07e5171f8526dd0d908aR34.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to merge this PR as a bunch of other patches are blocked on it. We can continue the discussion here; I'll do a follow-up patch if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry my bad, miss-read the line of .parseJson(input).isMissingNode();
please ignore :P
"CREATE STREAM test (K STRING KEY, maybeJSON STRING) WITH (kafka_topic='test_topic', VALUE_FORMAT='JSON');", | ||
"CREATE STREAM OUTPUT AS SELECT K, maybeJSON as JSON FROM test WHERE IS_JSON_STRING(maybeJSON);" | ||
], | ||
"inputs": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we test for ""
as well, as the docs indicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dfd1c82
to
19698dd
Compare
19698dd
to
2f556c9
Compare
Description
This PR introduces a new UDF -
is_json_string
that returnstrue
if the argument string can be parsed as a valid JSON value,false
otherwise. See the corresponding section in KIP-59 for more details.Testing done
Added unit and functional tests.
Reviewer checklist