You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "gustavodemorais (via GitHub)" <gi...@apache.org> on 2025/04/17 11:39:56 UTC
[PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
gustavodemorais opened a new pull request, #26474:
URL: https://github.com/apache/flink/pull/26474
<!--
*Thank you very much for contributing to Apache Flink - we are happy that you want to help us improve Flink. To help the community review your contribution in the best possible way, please go through the checklist below, which will get the contribution into a shape in which it can be best reviewed.*
*Please understand that we do not do this to make contributions to Flink a hassle. In order to uphold a high standard of quality for code contributions, while at the same time managing a large number of contributions, we need contributors to prepare the contributions well, and give reviewers enough contextual information for the review. Please also understand that contributions that do not follow this guide will take longer to review and thus typically be picked up with lower priority by the community.*
## Contribution Checklist
- Make sure that the pull request corresponds to a [JIRA issue](https://issues.apache.org/jira/projects/FLINK/issues). Exceptions are made for typos in JavaDoc or documentation files, which need no JIRA issue.
- Name the pull request in the form "[FLINK-XXXX] [component] Title of the pull request", where *FLINK-XXXX* should be replaced by the actual issue number. Skip *component* if you are unsure about which is the best component.
Typo fixes that have no associated JIRA issue should be named following this pattern: `[hotfix] [docs] Fix typo in event time introduction` or `[hotfix] [javadocs] Expand JavaDoc for PuncuatedWatermarkGenerator`.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Make sure that the change passes the automated tests, i.e., `mvn clean verify` passes. You can set up Azure Pipelines CI to do that following [this guide](https://cwiki.apache.org/confluence/display/FLINK/Azure+Pipelines#AzurePipelines-Tutorial:SettingupAzurePipelinesforaforkoftheFlinkrepository).
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message (including the JIRA id)
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
## What is the purpose of the change
Parsing for a JSON() function call fail to its position if not in the first param.
`SELECT JSON_OBJECT('key' VALUE JSON('{}'), 'key_2' VALUE JSON('{}'));`
Outputs:
```
The JSON() function is currently only supported inside a JSON_OBJECT() or JSON_ARRAY() function. Example: JSON_OBJECT('a', JSON('{\"key\": \"value\"}')) or JSON_ARRAY(JSON('{\"key\": \"value\"}'))
```
This is incorrect, we were too strict with the limitations. We're stricly allowing json to be called only as the second param in the function, when it should be allowed for every second.
## Brief change log
*(for example:)*
- Check if the `JSON` call is for any of the VALUES param inside a `JSON_OBJECT` and not only the first one
## Verifying this change
This change added tests and can be verified as follows:
- Added unit tests
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no)
- The serializers: (no)
- The runtime per-record code paths (performance sensitive): (no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
- The S3 file system connector: (no)
## Documentation
- Does this pull request introduce a new feature? (no)
- If yes, how is the feature documented? (already covered by docs / JavaDocs)
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin merged PR #26474:
URL: https://github.com/apache/flink/pull/26474
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
Posted by "gustavodemorais (via GitHub)" <gi...@apache.org>.
gustavodemorais commented on code in PR #26474:
URL: https://github.com/apache/flink/pull/26474#discussion_r2057881368
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/JsonGenerateUtils.scala:
##########
@@ -227,6 +227,23 @@ object JsonGenerateUtils {
}
}
+ /**
+ * Determines if the parameter index is % 2, which means checking that we only allow JSON calls
+ * for the VALUE parameter of a JSON_OBJECT call.
+ */
+ def inValuesParamOfJsonObject(i: Int): Boolean = {
+ (i % 2) == 0
+ }
+
+ /**
+ * Determines whether a JSON function is allowed in the current context. JSON functions are
+ * allowed as values in JSON_ARRAY calls or as value parameters in JSON_OBJECT calls.
+ */
+ def isSupportedJsonOperand(operand: RexNode, call: RexNode, i: Int): Boolean = {
+ isJsonFunctionOperand(operand) &&
+ (isJsonArrayOperand(call) || isJsonObjectOperand(call) && inValuesParamOfJsonObject(i))
+ }
Review Comment:
IMO `inValuesParamOfJsonObject` is more readable than `i % 2 == 0`? Just reading the function, the user might ask itself, what is `i` here? Why does it has to pass this condition? I think the extra function makes it easier to understand. Wdyt?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
Posted by "lihaosky (via GitHub)" <gi...@apache.org>.
lihaosky commented on code in PR #26474:
URL: https://github.com/apache/flink/pull/26474#discussion_r2066877602
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/JsonFunctionsITCase.java:
##########
@@ -821,12 +821,18 @@ private static List<TestSetSpec> jsonSpec() {
json("[1, 2, 3]")),
"JSON_OBJECT(KEY 'p1' VALUE JSON(f0), KEY 'p2' VALUE JSON(f1), KEY 'p3' VALUE JSON('[1, 2, 3]'))",
"{\"p1\":{\"key\":\"value\"},\"p2\":{\"key\":{\"value\":42}},\"p3\":[1,2,3]}",
- STRING().notNull()),
-
- // Tests for JSON calls inside of JSON_ARRAY
- TestSetSpec.forFunction(BuiltInFunctionDefinitions.JSON_ARRAY)
- .onFieldsWithData("{\"key\":\"value\"}", "{\"key\": {\"value\": 42}}")
- .andDataTypes(STRING(), STRING())
+ STRING().notNull())
+ .testSqlValidationError(
+ "JSON_OBJECT(KEY JSON('{}') VALUE 'value' ABSENT ON NULL)",
+ "The JSON() function is currently only supported inside a JSON_OBJECT() or JSON_ARRAY() function.")
Review Comment:
The error message is confusing though. `JSON()` function is indeed inside `JSON_OBJECT`. We need to make it more accurate that it also needs to be in value position?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #26474:
URL: https://github.com/apache/flink/pull/26474#discussion_r2057965475
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/JsonGenerateUtils.scala:
##########
@@ -227,6 +227,23 @@ object JsonGenerateUtils {
}
}
+ /**
+ * Determines if the parameter index is % 2, which means checking that we only allow JSON calls
+ * for the VALUE parameter of a JSON_OBJECT call.
+ */
+ def inValuesParamOfJsonObject(i: Int): Boolean = {
+ (i % 2) == 0
+ }
+
+ /**
+ * Determines whether a JSON function is allowed in the current context. JSON functions are
+ * allowed as values in JSON_ARRAY calls or as value parameters in JSON_OBJECT calls.
+ */
+ def isSupportedJsonOperand(operand: RexNode, call: RexNode, i: Int): Boolean = {
+ isJsonFunctionOperand(operand) &&
+ (isJsonArrayOperand(call) || isJsonObjectOperand(call) && inValuesParamOfJsonObject(i))
+ }
Review Comment:
the confusing part for me is
1. the line which is body of function is about 10 symbols length
2. we add a separate method for that and 5 lines of comments for description
why not just inline it and add one-line comment describing the intention?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
Posted by "lihaosky (via GitHub)" <gi...@apache.org>.
lihaosky commented on code in PR #26474:
URL: https://github.com/apache/flink/pull/26474#discussion_r2058791859
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/JsonFunctionsITCase.java:
##########
@@ -782,7 +782,47 @@ private static List<TestSetSpec> jsonSpec() {
.testTableApiRuntimeError(
jsonObject(JsonOnNull.NULL, "K", json("{")),
TableRuntimeException.class,
- "Invalid JSON string in JSON(value) function"),
+ "Invalid JSON string in JSON(value) function")
+
+ // Tests for JSON_OBJECT with multiple parameters
+ .testResult(
+ jsonObject(JsonOnNull.NULL, "key1", "val", "key2", json($("f1"))),
Review Comment:
Can you test put `json($("f1"))` as key?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #26474:
URL: https://github.com/apache/flink/pull/26474#discussion_r2053670854
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/JsonGenerateUtils.scala:
##########
@@ -227,6 +227,14 @@ object JsonGenerateUtils {
}
}
+ /**
+ * Determines if the parameter index is % 2, which means checking that we only allow JSON calls
+ * for the VALUE parameter of a JSON_OBJECT call.
+ */
+ def inValuesParam(i: Int): Boolean = {
Review Comment:
I think we can drop this method if we extract the whole `if` condition into a method.
Also about naming: it might make confused some people since in SQL there is also `VALUES` and this method has nothing to do with 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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #26474:
URL: https://github.com/apache/flink/pull/26474#discussion_r2057791718
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/JsonGenerateUtils.scala:
##########
@@ -227,6 +227,23 @@ object JsonGenerateUtils {
}
}
+ /**
+ * Determines if the parameter index is % 2, which means checking that we only allow JSON calls
+ * for the VALUE parameter of a JSON_OBJECT call.
+ */
+ def inValuesParamOfJsonObject(i: Int): Boolean = {
+ (i % 2) == 0
+ }
+
+ /**
+ * Determines whether a JSON function is allowed in the current context. JSON functions are
+ * allowed as values in JSON_ARRAY calls or as value parameters in JSON_OBJECT calls.
+ */
+ def isSupportedJsonOperand(operand: RexNode, call: RexNode, i: Int): Boolean = {
+ isJsonFunctionOperand(operand) &&
+ (isJsonArrayOperand(call) || isJsonObjectOperand(call) && inValuesParamOfJsonObject(i))
+ }
Review Comment:
```suggestion
def isSupportedJsonOperand(operand: RexNode, call: RexNode, i: Int): Boolean = {
isJsonFunctionOperand(operand) &&
(isJsonArrayOperand(call) || i % 2 == 0 && isJsonObjectOperand(call))
}
```
nit
I think we can completely remove `inValuesParamOfJsonObject`
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
Posted by "gustavodemorais (via GitHub)" <gi...@apache.org>.
gustavodemorais commented on code in PR #26474:
URL: https://github.com/apache/flink/pull/26474#discussion_r2058574599
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/JsonGenerateUtils.scala:
##########
@@ -227,6 +227,23 @@ object JsonGenerateUtils {
}
}
+ /**
+ * Determines if the parameter index is % 2, which means checking that we only allow JSON calls
+ * for the VALUE parameter of a JSON_OBJECT call.
+ */
+ def inValuesParamOfJsonObject(i: Int): Boolean = {
+ (i % 2) == 0
+ }
+
+ /**
+ * Determines whether a JSON function is allowed in the current context. JSON functions are
+ * allowed as values in JSON_ARRAY calls or as value parameters in JSON_OBJECT calls.
+ */
+ def isSupportedJsonOperand(operand: RexNode, call: RexNode, i: Int): Boolean = {
+ isJsonFunctionOperand(operand) &&
+ (isJsonArrayOperand(call) || isJsonObjectOperand(call) && inValuesParamOfJsonObject(i))
+ }
Review Comment:
I usually try to use more code than comments to make it understandable and not run the risk of having it outdated eventually. But I see where you're coming from in this case 👍 It is indeed shorter to go with a comment here.
I've moved the check and complemented the comment. [Take a look](https://github.com/apache/flink/pull/26474/commits/e30c207e371794adaac0261ed20a58a7c812f192)
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #26474:
URL: https://github.com/apache/flink/pull/26474#discussion_r2053643885
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/ExprCodeGenerator.scala:
##########
@@ -489,7 +489,7 @@ class ExprCodeGenerator(ctx: CodeGeneratorContext, nullableInput: Boolean)
// We only support JSON function operands as the value param of a JSON_OBJECT or JSON_ARRAY function
case (operand: RexNode, i)
if isJsonFunctionOperand(operand) &&
- (isJsonArrayOperand(call) || i == 2 && isJsonObjectOperand(call)) =>
+ (isJsonArrayOperand(call) || isJsonObjectOperand(call) && inValuesParam(i)) =>
Review Comment:
i wonder whether we can extract the whole `if` condition into method something like `isJsonOperand`
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
Posted by "gustavodemorais (via GitHub)" <gi...@apache.org>.
gustavodemorais commented on code in PR #26474:
URL: https://github.com/apache/flink/pull/26474#discussion_r2057881368
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/JsonGenerateUtils.scala:
##########
@@ -227,6 +227,23 @@ object JsonGenerateUtils {
}
}
+ /**
+ * Determines if the parameter index is % 2, which means checking that we only allow JSON calls
+ * for the VALUE parameter of a JSON_OBJECT call.
+ */
+ def inValuesParamOfJsonObject(i: Int): Boolean = {
+ (i % 2) == 0
+ }
+
+ /**
+ * Determines whether a JSON function is allowed in the current context. JSON functions are
+ * allowed as values in JSON_ARRAY calls or as value parameters in JSON_OBJECT calls.
+ */
+ def isSupportedJsonOperand(operand: RexNode, call: RexNode, i: Int): Boolean = {
+ isJsonFunctionOperand(operand) &&
+ (isJsonArrayOperand(call) || isJsonObjectOperand(call) && inValuesParamOfJsonObject(i))
+ }
Review Comment:
Don't you think `inValuesParamOfJsonObject` is more readable than `i % 2 == 0`? Just reading the function, the user might ask itself, what is `i`? Why does it has to pass this condition? I think the extra function makes it easier to understand. Wdyt?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
Posted by "gustavodemorais (via GitHub)" <gi...@apache.org>.
gustavodemorais commented on code in PR #26474:
URL: https://github.com/apache/flink/pull/26474#discussion_r2055511354
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/ExprCodeGenerator.scala:
##########
@@ -489,7 +489,7 @@ class ExprCodeGenerator(ctx: CodeGeneratorContext, nullableInput: Boolean)
// We only support JSON function operands as the value param of a JSON_OBJECT or JSON_ARRAY function
case (operand: RexNode, i)
if isJsonFunctionOperand(operand) &&
- (isJsonArrayOperand(call) || i == 2 && isJsonObjectOperand(call)) =>
+ (isJsonArrayOperand(call) || isJsonObjectOperand(call) && inValuesParam(i)) =>
Review Comment:
Created a helper function called `isSupportedJsonOperand`
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/JsonGenerateUtils.scala:
##########
@@ -227,6 +227,14 @@ object JsonGenerateUtils {
}
}
+ /**
+ * Determines if the parameter index is % 2, which means checking that we only allow JSON calls
+ * for the VALUE parameter of a JSON_OBJECT call.
+ */
+ def inValuesParam(i: Int): Boolean = {
Review Comment:
I renamed it to inValuesParamOfJsonObject? I think the code is more readable if you read `inValuesParamOfJsonObject` as part of the if condition than `i%2`
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
Posted by "lihaosky (via GitHub)" <gi...@apache.org>.
lihaosky commented on code in PR #26474:
URL: https://github.com/apache/flink/pull/26474#discussion_r2066987345
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/JsonFunctionsITCase.java:
##########
@@ -821,12 +821,18 @@ private static List<TestSetSpec> jsonSpec() {
json("[1, 2, 3]")),
"JSON_OBJECT(KEY 'p1' VALUE JSON(f0), KEY 'p2' VALUE JSON(f1), KEY 'p3' VALUE JSON('[1, 2, 3]'))",
"{\"p1\":{\"key\":\"value\"},\"p2\":{\"key\":{\"value\":42}},\"p3\":[1,2,3]}",
- STRING().notNull()),
-
- // Tests for JSON calls inside of JSON_ARRAY
- TestSetSpec.forFunction(BuiltInFunctionDefinitions.JSON_ARRAY)
- .onFieldsWithData("{\"key\":\"value\"}", "{\"key\": {\"value\": 42}}")
- .andDataTypes(STRING(), STRING())
+ STRING().notNull())
+ .testSqlValidationError(
+ "JSON_OBJECT(KEY JSON('{}') VALUE 'value' ABSENT ON NULL)",
+ "The JSON() function is currently only supported inside a JSON_OBJECT() or JSON_ARRAY() function.")
Review Comment:
`The JSON() function is currently only supported inside a JSON_OBJECT() or JSON_ARRAY() as values`? Is this more accurate?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #26474:
URL: https://github.com/apache/flink/pull/26474#issuecomment-2812614744
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "53efbaee5160a76ba2eecd802b6ce247dcfcaaae",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "53efbaee5160a76ba2eecd802b6ce247dcfcaaae",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 53efbaee5160a76ba2eecd802b6ce247dcfcaaae UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
Posted by "gustavodemorais (via GitHub)" <gi...@apache.org>.
gustavodemorais commented on code in PR #26474:
URL: https://github.com/apache/flink/pull/26474#discussion_r2067090762
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/JsonFunctionsITCase.java:
##########
@@ -821,12 +821,18 @@ private static List<TestSetSpec> jsonSpec() {
json("[1, 2, 3]")),
"JSON_OBJECT(KEY 'p1' VALUE JSON(f0), KEY 'p2' VALUE JSON(f1), KEY 'p3' VALUE JSON('[1, 2, 3]'))",
"{\"p1\":{\"key\":\"value\"},\"p2\":{\"key\":{\"value\":42}},\"p3\":[1,2,3]}",
- STRING().notNull()),
-
- // Tests for JSON calls inside of JSON_ARRAY
- TestSetSpec.forFunction(BuiltInFunctionDefinitions.JSON_ARRAY)
- .onFieldsWithData("{\"key\":\"value\"}", "{\"key\": {\"value\": 42}}")
- .andDataTypes(STRING(), STRING())
+ STRING().notNull())
+ .testSqlValidationError(
+ "JSON_OBJECT(KEY JSON('{}') VALUE 'value' ABSENT ON NULL)",
+ "The JSON() function is currently only supported inside a JSON_OBJECT() or JSON_ARRAY() function.")
Review Comment:
The param is actually called `VALUE`. What do you think:
`The JSON() function is currently only supported inside JSON_ARRAY() or as the VALUE param of JSON_OBJECT()`
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
Posted by "gustavodemorais (via GitHub)" <gi...@apache.org>.
gustavodemorais commented on code in PR #26474:
URL: https://github.com/apache/flink/pull/26474#discussion_r2066973510
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/JsonFunctionsITCase.java:
##########
@@ -821,12 +821,18 @@ private static List<TestSetSpec> jsonSpec() {
json("[1, 2, 3]")),
"JSON_OBJECT(KEY 'p1' VALUE JSON(f0), KEY 'p2' VALUE JSON(f1), KEY 'p3' VALUE JSON('[1, 2, 3]'))",
"{\"p1\":{\"key\":\"value\"},\"p2\":{\"key\":{\"value\":42}},\"p3\":[1,2,3]}",
- STRING().notNull()),
-
- // Tests for JSON calls inside of JSON_ARRAY
- TestSetSpec.forFunction(BuiltInFunctionDefinitions.JSON_ARRAY)
- .onFieldsWithData("{\"key\":\"value\"}", "{\"key\": {\"value\": 42}}")
- .andDataTypes(STRING(), STRING())
+ STRING().notNull())
+ .testSqlValidationError(
+ "JSON_OBJECT(KEY JSON('{}') VALUE 'value' ABSENT ON NULL)",
+ "The JSON() function is currently only supported inside a JSON_OBJECT() or JSON_ARRAY() function.")
Review Comment:
The full message contains examples that should help the user understand the expected use. [See](https://github.com/apache/flink/blob/e30c207e371794adaac0261ed20a58a7c812f192/flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/ExprCodeGenerator.scala#L467):
```
The JSON() function is currently only supported inside a JSON_OBJECT() or JSON_ARRAY()" +
" function. Example: JSON_OBJECT('a', JSON('{"key": "value"}')) or " +
"JSON_ARRAY(JSON('{"key": "value"}')).
```
Do you think that's clear enough? Let me know if you have a specific suggestion
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-37695][table] Fix parsing for built-in function JSON in JSON_OBJECT for all positions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #26474:
URL: https://github.com/apache/flink/pull/26474#discussion_r2053643885
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/ExprCodeGenerator.scala:
##########
@@ -489,7 +489,7 @@ class ExprCodeGenerator(ctx: CodeGeneratorContext, nullableInput: Boolean)
// We only support JSON function operands as the value param of a JSON_OBJECT or JSON_ARRAY function
case (operand: RexNode, i)
if isJsonFunctionOperand(operand) &&
- (isJsonArrayOperand(call) || i == 2 && isJsonObjectOperand(call)) =>
+ (isJsonArrayOperand(call) || isJsonObjectOperand(call) && inValuesParam(i)) =>
Review Comment:
i wonder whether we can extract the whole `if` condition into method
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org