-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[FLINK-16206][table-planner] Support JSON_ARRAYAGG #17562
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 7a90ad8 (Mon Oct 25 18:40:01 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
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.
Thanks @Airblader. I added some final comments.
...rc/main/java/org/apache/flink/table/planner/functions/aggfunctions/JsonArrayAggFunction.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/flink/table/planner/functions/aggfunctions/JsonArrayAggFunction.java
Outdated
Show resolved
Hide resolved
try { | ||
for (final StringData item : acc.list.get()) { | ||
final JsonNode itemNode = | ||
getNodeFactory().rawValueNode(new RawValue(item.toString())); |
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 we also somehow sort the list? otherwise the merge
might behave non-deterministic
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.
Hm, this is actually a problem. In JSON_OBJECTAGG we sorted the keys, and that's fine, but arrays are order-sensitive. Maybe we shouldn't support merge
at all? We'd need WITHIN GROUP
here, but I'm guessing Calcite doesn't support that keyword there yet (will have to check, though).
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 removed merge
for now, documented the limitation and raised FLINK-24664. Since we're facing the same issue for FIRST_VALUE / LAST_VALUE (and in theory also e.g. LISTAGG), I've also cross-linked them.
7a90ad8
to
dc21e87
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.
Thanks for the update. LGTM
We represent (NULL|ABSENT) ON NULL as two separate built-in functions for now. This is necessary because otherwise we would have to ship the symbol across the network for each record, which leads to various problems. Calcite essentially uses the same workaround. This is the same as was done for JSON_OBJECTAGG. This closes apache#17562.
@flinkbot run azure |
@flinkbot run azure |
We represent (NULL|ABSENT) ON NULL as two separate built-in functions for now. This is necessary because otherwise we would have to ship the symbol across the network for each record, which leads to various problems. Calcite essentially uses the same workaround. This is the same as was done for JSON_OBJECTAGG. This closes apache#17562.
What is the purpose of the change
This introduces
JSON_ARRAYAGG
akin toJSON_OBJECTAGG
from #17549.Note that this PR is based off of that PR and thus needs to be rebased once #17549 is merged. Keeping it in draft until then.
supersedes #11370
Verifying this change
JsonAggregationFunctionsITCase
WrapJsonAggFunctionArgumentsRuleTest
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: yesDocumentation