[FLINK-40268][table] Add new JSON_TYPE function - #28850
Conversation
|
General feedback after the first pass JSON itself only knows six kinds of value: object, array, string, number, boolean, and null. That's it - there's no such thing as a "date" or a "float" in JSON. So if someone writes {"d": "2015-01-01"}, that d is a string that happens to look like a date. The PR guesses and reports DATE, and similarly guesses FLOAT vs DOUBLE based on how precise the number is. Our recommendation is not to guess: just report what's actually there. Other databases sometimes do report DATE, which is why this looks like precedent - but they can only do it because they store JSON in their own typed format, so a real date got written in as a real date. Flink is handed a plain string of text, which carries no such information. And it matters practically: if you write CASE JSON_TYPE(x) WHEN 'STRING' THEN ..., every date-looking string silently skips your branch. Reporting the six real types keeps the answer both true, predictable and useful. We want to implement this in a way it's useful for typical use cases: users reach for it to branch on fields whose shape varies - a payload where errors is sometimes a string, sometimes an array. For that, following the json native way is a simpler and sufficient. That said, some concrete suggestions:
|
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
| - sql: JSON_TYPE(jsonValue) | ||
| table: jsonType(jsonValue) | ||
| description: | | ||
| Returns a string value indicating the type of jsonValue. Returns `NULL` if jsonValue is | ||
| `NULL` or is not valid JSON. |
There was a problem hiding this comment.
[Suggestion] The Chinese docs file (sql_functions_zh.yml) contains an untranslated English description for JSON_TYPE, while all surrounding entries are in Chinese. — Concrete cost: the adjacent JSON_ARRAY entry reads "将数值列表构建为 JSON 数组字符串"; a Chinese-speaking user encounters a fully English JSON_TYPE entry (56 lines of prose and SQL comments) copy-pasted from sql_functions.yml, breaking the consistency of the localized documentation.
Translate the description block to Chinese, matching the style of neighbouring entries (e.g., JSON_ARRAY, JSON_OBJECT). The SQL examples can remain as-is; only the prose and SQL comments need translation.
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
As I do not speak chinese myself, I do not think it is a good idea for me to google translate and hope it works. I was thinking if a chinese contributer could give me a hand with the translations here, would be a great help.
Thanks
| * SqlJsonUtils.JsonValueContext} and the type flag is read off it, guarded so that a NULL | ||
| * argument yields a NULL result. | ||
| * | ||
| * <p>The parsed context is shared with {@code JSON_VALUE} and {@code JSON_QUERY} over the same |
There was a problem hiding this comment.
why do we need mentioning functions here?
Do we have a functionality to update the comment each time the list of functions among which it is shared will be updated?
| } else if (val == null) { | ||
| return "NULL"; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Why
SELECT json_type('null') IS NULL, json_type('null2') IS NULLreturns FALSE TRUE?
I would expected TRUE TRUE
There was a problem hiding this comment.
I needed a way to differentiate invalid JSON to a literal json null. So I decided to make it so that
invalid json -> null
null literal -> "NULL"
let me know if I should change this implementation
There was a problem hiding this comment.
is there any other vendor behaving like that or why this kind of behavior was selected?
| * <p>JSON's grammar cannot express a date, and it has a single number rule with no width, so | ||
| * {@code DATE} and {@code FLOAT} cannot be read off the Java type the parser produces: a date | ||
| * arrives as a {@link String}, and every non-integral number arrives as a {@link BigDecimal} | ||
| * (the shared mapper enables {@code USE_BIG_DECIMAL_FOR_FLOATS}). Both flags are therefore |
There was a problem hiding this comment.
why do we need to put anything about 3rd party libs configuration in this javadoc?
| * Date-times stay {@code STRING} in every spelling: there is no timestamp flag to return, | ||
| * and answering {@code DATE} for a value carrying a time of day would be worse than | ||
| * saying nothing. Flink has no single date-time spelling to defer to either — {@code | ||
| * CAST}/{@code TO_TIMESTAMP} accept only a space separator, the JSON format accepts a |
There was a problem hiding this comment.
why are we talking about TIMESTAMP here which is not supported in this function?
There was a problem hiding this comment.
this paragraph has been cut because i have dropped the date type
| // assign it only inside their own args-not-null guard. A NULL path argument in a preceding | ||
| // call therefore leaves it null here even though the input itself was fine. Report NULL | ||
| // instead of failing, which is how those functions already degrade in the same situation | ||
| // (their NPE is swallowed by jsonApiCommonSyntax and falls through to ON ERROR -> NULL). |
There was a problem hiding this comment.
do not put low level details
who and how will update this comment if anything is changing?
also jsonApiCommonSyntax - who will update this name if it will be renamed because of refactoring?
| // Values too large for 32 bits saturate to an infinity, which BigDecimal cannot represent. | ||
| if (!Float.isFinite(asFloat)) { | ||
| return false; | ||
| } | ||
| // Widening to double is lossless, so BigDecimal(double) yields the float's exact value. | ||
| // Float.toString must not be used here: it returns the shortest string that round-trips to | ||
| // the same float, so it would reproduce the input literal and report everything as FLOAT. | ||
| return new BigDecimal((double) asFloat).compareTo(value) == 0; |
There was a problem hiding this comment.
can you tell your AI being more concise?
… support,simplified comments
What is the purpose of the change
This PR is adding the implementation of the builting function JSON_TYPE builtin function to Apache Flink.
(For example: This pull request makes task deployment go through the blob server, rather than through RPC. That way we avoid re-transferring them on each deployment (during recovery).)
Brief change log
(for example:)
Verifying this change
./mvnw -o -pl flink-table/flink-table-planner -Dtest='JsonFunctionsITCase' -Dsurefire.failIfNoSpecifiedTests=false -Dcheckstyle.skip=true -Dspotless.check.skip=true -e -Drat.skip=true -Denforcer.skip=true test
or (depending on version)
mvn -o -pl flink-table/flink-table-planner -Dtest='JsonFunctionsITCase' -Dsurefire.failIfNoSpecifiedTests=false -Dcheckstyle.skip=true -Dspotless.check.skip=true -e -Drat.skip=true -Denforcer.skip=true test
Please make sure both new and modified tests in this PR follow the conventions for tests defined in our code quality guide.
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation
Was generative AI tooling used to co-author this PR?