You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "kurnoolsaketh (via GitHub)" <gi...@apache.org> on 2026/10/15 18:08:09 UTC
[PR] [FLINK-40963][table-planner] Add support for DATE and TIME conversion to JSON [flink]
kurnoolsaketh opened a new pull request, #29493:
URL: https://github.com/apache/flink/pull/29493
## What is the purpose of the change
The Flink planner rejects DATE and TIME when used in JSON conversion functions (e.g. JSON_STRING, JSON_OBJECT, JSON_ARRAY). This PR adds support for converting these fields.
## Brief change log
- JsonGenerateUtils.scala: add cases for DATE and TIME to generate JSON conversion code
- SpecificInputTypeStrategies.java: validate that DATETIME type family can be used in JSON context
- JsonFunctionsITCase.java: add IT coverage for the two missing types
## Verifying this change
This change added tests and can be verified as follows:
- [x] extend integration test for JSON functions (JsonFunctionsITCase.java)
## 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
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change the checkbox below to `[X]` and replace the placeholder in the "Generated-by"
line with the tool name and version. Otherwise remove the "Generated-by" line.
See the ASF Generative Tooling Guidance for details:
https://www.apache.org/legal/generative-tooling.html
You are responsible for the quality and correctness of every change in this PR
regardless of the tooling used. Low-effort AI-generated PRs will be closed. See
AGENTS.md for the full guidance.
-->
- [x] Yes (please specify the tool below)
Generated-by: [Claude Sonnet 5]
--
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-40963][table-planner] Add support for DATE and TIME conversion to JSON [flink]
Posted by "snuyanzin (via GitHub) via issues" <is...@flink.apache.org>.
snuyanzin commented on code in PR #29493:
URL: https://github.com/apache/flink/pull/29493#discussion_r4459790817
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/SpecificInputTypeStrategies.java:
##########
@@ -84,7 +84,7 @@ public static InputTypeStrategy windowTimeIndicator() {
or(
logical(LogicalTypeFamily.CHARACTER_STRING),
logical(LogicalTypeFamily.BINARY_STRING),
- logical(LogicalTypeFamily.TIMESTAMP),
+ logical(LogicalTypeFamily.DATETIME),
Review Comment:
why do you broaden `JSON_ARGUMENT` and add test only for json scalar?
what about ` JSON_OBJECTAGG`/`JSON_ARRAYAGG`?
--
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-40963][table-planner] Add support for DATE and TIME conversion to JSON [flink]
Posted by "kurnoolsaketh (via GitHub)" <gi...@apache.org>.
kurnoolsaketh commented on PR #29493:
URL: https://github.com/apache/flink/pull/29493#issuecomment-6300916935
> since the AI was used, it should be used Generated-by: instead of Co-authored-by: as mentioned in Apache recommendations
@snuyanzin I have updated the commit messages + PR description 👍
--
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-40963][table-planner] Add support for DATE and TIME conversion to JSON [flink]
Posted by "kurnoolsaketh (via GitHub) via issues" <is...@flink.apache.org>.
kurnoolsaketh commented on code in PR #29493:
URL: https://github.com/apache/flink/pull/29493#discussion_r4486752752
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/JsonGenerateUtils.scala:
##########
@@ -92,6 +92,18 @@ object JsonGenerateUtils {
case TIMESTAMP_WITH_LOCAL_TIME_ZONE => s"""$nodeFactoryTerm.textNode($isoTerm + "Z")"""
}
+ case TIME_WITHOUT_TIME_ZONE =>
+ val formatter = s"${typeTerm(classOf[DateTimeFormatter])}.ISO_LOCAL_TIME"
+ val isoTerm =
+ s"${typeTerm(classOf[DateTimeUtils])}.toLocalTime($valueTerm).format($formatter)"
+ s"$nodeFactoryTerm.textNode($isoTerm)"
+
+ case DATE =>
+ val formatter = s"${typeTerm(classOf[DateTimeFormatter])}.ISO_LOCAL_DATE"
+ val isoTerm =
+ s"${typeTerm(classOf[DateTimeUtils])}.toLocalDate($valueTerm).format($formatter)"
+ s"$nodeFactoryTerm.textNode($isoTerm)"
Review Comment:
> can we reuse DateTimeUtils
if I am understanding you correctly, we _are_ using DateTimeUtils here either way - we just have a choice on what helper function to use. See below for my opinion on that.
> for instance DateTimeUtils.formatDate
we can reuse it, but it doesn't handle edge cases well. For ex, overflow cases: `DateTimeUtils.formatDate(Integer.MAX_VALUE)` outputs `)*,0-0+-0(` (garbage date) whereas `DateTimeUtils.toLocalDate(Integer.MAX_VALUE).format(ISO_LOCAL_DATE)` throws since it's an invalid date. So, I prefer to use `toLocalDate` and `format` since they call java std libraries.
--
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-40963][table-planner] Add support for DATE and TIME conversion to JSON [flink]
Posted by "kurnoolsaketh (via GitHub)" <gi...@apache.org>.
kurnoolsaketh commented on code in PR #29493:
URL: https://github.com/apache/flink/pull/29493#discussion_r4329871389
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/JsonFunctionsITCase.java:
##########
@@ -1029,6 +1033,31 @@ private static List<TestSetSpec> jsonStringSpec() {
jsonString($("f0")),
"JSON_STRING(f0)",
"{\"field\\ttab\":\"val4\",\"field\\nline\":\"val3\",\"field\\rreturn\":\"val5\",\"field\\\"quote\":\"val1\",\"field\\\\slash\":\"val2\"}",
+ STRING().notNull()),
+ // DATE/TIME cases
Review Comment:
removed
--
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-40963][table-planner] Add support for DATE and TIME conversion to JSON [flink]
Posted by "snuyanzin (via GitHub) via issues" <is...@flink.apache.org>.
snuyanzin merged PR #29493:
URL: https://github.com/apache/flink/pull/29493
--
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-40963][table-planner] Add support for DATE and TIME conversion to JSON [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #29493:
URL: https://github.com/apache/flink/pull/29493#discussion_r4303722602
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/JsonGenerateUtils.scala:
##########
@@ -92,6 +92,18 @@ object JsonGenerateUtils {
case TIMESTAMP_WITH_LOCAL_TIME_ZONE => s"""$nodeFactoryTerm.textNode($isoTerm + "Z")"""
}
+ case TIME_WITHOUT_TIME_ZONE =>
+ val formatter = s"${typeTerm(classOf[DateTimeFormatter])}.ISO_LOCAL_TIME"
+ val isoTerm =
+ s"${typeTerm(classOf[DateTimeUtils])}.toLocalTime($valueTerm).format($formatter)"
+ s"$nodeFactoryTerm.textNode($isoTerm)"
+
+ case DATE =>
+ val formatter = s"${typeTerm(classOf[DateTimeFormatter])}.ISO_DATE"
Review Comment:
why do we use `ISO_LOCAL_TIME`, `ISO_LOCAL_TIMESTAMP`
and suddenly `ISO_DATE` without local?
--
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-40963][table-planner] Add support for DATE and TIME conversion to JSON [flink]
Posted by "kurnoolsaketh (via GitHub) via issues" <is...@flink.apache.org>.
kurnoolsaketh commented on code in PR #29493:
URL: https://github.com/apache/flink/pull/29493#discussion_r4486752752
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/JsonGenerateUtils.scala:
##########
@@ -92,6 +92,18 @@ object JsonGenerateUtils {
case TIMESTAMP_WITH_LOCAL_TIME_ZONE => s"""$nodeFactoryTerm.textNode($isoTerm + "Z")"""
}
+ case TIME_WITHOUT_TIME_ZONE =>
+ val formatter = s"${typeTerm(classOf[DateTimeFormatter])}.ISO_LOCAL_TIME"
+ val isoTerm =
+ s"${typeTerm(classOf[DateTimeUtils])}.toLocalTime($valueTerm).format($formatter)"
+ s"$nodeFactoryTerm.textNode($isoTerm)"
+
+ case DATE =>
+ val formatter = s"${typeTerm(classOf[DateTimeFormatter])}.ISO_LOCAL_DATE"
+ val isoTerm =
+ s"${typeTerm(classOf[DateTimeUtils])}.toLocalDate($valueTerm).format($formatter)"
+ s"$nodeFactoryTerm.textNode($isoTerm)"
Review Comment:
> can we reuse DateTimeUtils
if I am understanding you correctly, we _are_ using DateTimeUtils here either way - we just have a choice on what helper function to use. See below for my opinion on that.
> for instance DateTimeUtils.formatDate
we can reuse it, but it doesn't handle edge cases well. For ex, overflow cases: `DateTimeUtils.formatDate(Integer.MAX_VALUE)` outputs `)*,0-0+-0(` (garbage date) whereas `DateTimeUtils.toLocalDate(Integer.MAX_VALUE).format(ISO_LOCAL_DATE)` throws since it's an invalid date. So, I prefer to use `toLocalDate` and `format` since it is uses java std libraries.
--
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-40963][table-planner] Add support for DATE and TIME conversion to JSON [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #29493:
URL: https://github.com/apache/flink/pull/29493#discussion_r4303731399
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/JsonFunctionsITCase.java:
##########
@@ -1029,6 +1033,31 @@ private static List<TestSetSpec> jsonStringSpec() {
jsonString($("f0")),
"JSON_STRING(f0)",
"{\"field\\ttab\":\"val4\",\"field\\nline\":\"val3\",\"field\\rreturn\":\"val5\",\"field\\\"quote\":\"val1\",\"field\\\\slash\":\"val2\"}",
+ STRING().notNull()),
+ // DATE/TIME cases
Review Comment:
why do we need this comment?
isn't it obvious from the code?
--
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-40963][table-planner] Add support for DATE and TIME conversion to JSON [flink]
Posted by "kurnoolsaketh (via GitHub) via issues" <is...@flink.apache.org>.
kurnoolsaketh commented on code in PR #29493:
URL: https://github.com/apache/flink/pull/29493#discussion_r4486752752
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/JsonGenerateUtils.scala:
##########
@@ -92,6 +92,18 @@ object JsonGenerateUtils {
case TIMESTAMP_WITH_LOCAL_TIME_ZONE => s"""$nodeFactoryTerm.textNode($isoTerm + "Z")"""
}
+ case TIME_WITHOUT_TIME_ZONE =>
+ val formatter = s"${typeTerm(classOf[DateTimeFormatter])}.ISO_LOCAL_TIME"
+ val isoTerm =
+ s"${typeTerm(classOf[DateTimeUtils])}.toLocalTime($valueTerm).format($formatter)"
+ s"$nodeFactoryTerm.textNode($isoTerm)"
+
+ case DATE =>
+ val formatter = s"${typeTerm(classOf[DateTimeFormatter])}.ISO_LOCAL_DATE"
+ val isoTerm =
+ s"${typeTerm(classOf[DateTimeUtils])}.toLocalDate($valueTerm).format($formatter)"
+ s"$nodeFactoryTerm.textNode($isoTerm)"
Review Comment:
> can we reuse DateTimeUtils
if I am understanding you correctly, we _are_ using DateTimeUtils here either way - we just have a choice on what helper function to use. See below for my opinion on that.
> for instance DateTimeUtils.formatDate
we can reuse it, but it doesn't handle edge cases well. For ex, overflow cases: `DateTimeUtils.formatDate(Integer.MAX_VALUE)` outputs `)*,0-0+-0(` whereas `DateTimeUtils.toLocalDate(Integer.MAX_VALUE).format(ISO_LOCAL_DATE)` throws since it's an invalid date. So, I prefer to use `toLocalDate` and `format` since it is uses java std libraries.
--
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-40963][table-planner] Add support for DATE and TIME conversion to JSON [flink]
Posted by "kurnoolsaketh (via GitHub) via issues" <is...@flink.apache.org>.
kurnoolsaketh commented on code in PR #29493:
URL: https://github.com/apache/flink/pull/29493#discussion_r4487244737
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/SpecificInputTypeStrategies.java:
##########
@@ -84,7 +84,7 @@ public static InputTypeStrategy windowTimeIndicator() {
or(
logical(LogicalTypeFamily.CHARACTER_STRING),
logical(LogicalTypeFamily.BINARY_STRING),
- logical(LogicalTypeFamily.TIMESTAMP),
+ logical(LogicalTypeFamily.DATETIME),
Review Comment:
added DATE and TIME test coverage to all functions that accept `JSON_ARGUMENT`
--
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-40963][table-planner] Add support for DATE and TIME conversion to JSON [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #29493:
URL: https://github.com/apache/flink/pull/29493#issuecomment-6199173759
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "4ead9bba872ed0de26cc700038a4be9ee5ea9dcf",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "4ead9bba872ed0de26cc700038a4be9ee5ea9dcf",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 4ead9bba872ed0de26cc700038a4be9ee5ea9dcf 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-40963][table-planner] Add support for DATE and TIME conversion to JSON [flink]
Posted by "kurnoolsaketh (via GitHub)" <gi...@apache.org>.
kurnoolsaketh commented on code in PR #29493:
URL: https://github.com/apache/flink/pull/29493#discussion_r4329860656
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/JsonGenerateUtils.scala:
##########
@@ -92,6 +92,18 @@ object JsonGenerateUtils {
case TIMESTAMP_WITH_LOCAL_TIME_ZONE => s"""$nodeFactoryTerm.textNode($isoTerm + "Z")"""
}
+ case TIME_WITHOUT_TIME_ZONE =>
+ val formatter = s"${typeTerm(classOf[DateTimeFormatter])}.ISO_LOCAL_TIME"
+ val isoTerm =
+ s"${typeTerm(classOf[DateTimeUtils])}.toLocalTime($valueTerm).format($formatter)"
+ s"$nodeFactoryTerm.textNode($isoTerm)"
+
+ case DATE =>
+ val formatter = s"${typeTerm(classOf[DateTimeFormatter])}.ISO_DATE"
Review Comment:
it's a good catch, thank you. `DATE` logical type should be formatted using `ISO_LOCAL_DATE` since it doesn't contain a time-zone ID/offset. I've changed 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-40963][table-planner] Add support for DATE and TIME conversion to JSON [flink]
Posted by "snuyanzin (via GitHub) via issues" <is...@flink.apache.org>.
snuyanzin commented on code in PR #29493:
URL: https://github.com/apache/flink/pull/29493#discussion_r4459771362
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/JsonGenerateUtils.scala:
##########
@@ -92,6 +92,18 @@ object JsonGenerateUtils {
case TIMESTAMP_WITH_LOCAL_TIME_ZONE => s"""$nodeFactoryTerm.textNode($isoTerm + "Z")"""
}
+ case TIME_WITHOUT_TIME_ZONE =>
+ val formatter = s"${typeTerm(classOf[DateTimeFormatter])}.ISO_LOCAL_TIME"
+ val isoTerm =
+ s"${typeTerm(classOf[DateTimeUtils])}.toLocalTime($valueTerm).format($formatter)"
+ s"$nodeFactoryTerm.textNode($isoTerm)"
+
+ case DATE =>
+ val formatter = s"${typeTerm(classOf[DateTimeFormatter])}.ISO_LOCAL_DATE"
+ val isoTerm =
+ s"${typeTerm(classOf[DateTimeUtils])}.toLocalDate($valueTerm).format($formatter)"
+ s"$nodeFactoryTerm.textNode($isoTerm)"
Review Comment:
do we really need this code?
can we reuse `DateTimeUtils`?
for instance `DateTimeUtils.formatDate`?
--
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