[FLINK-40356][table] Add new MapFromEntries function - #28948
Conversation
- Pre-size the map since we already know the input - Fill the key and value arrays in one loop instead of using streams
| DataTypes.ARRAY(entryType), | ||
| DataTypes.ARRAY(nestedEntryType), | ||
| DataTypes.ARRAY(entryType)) | ||
| // duplicate keys: the last value wins |
There was a problem hiding this comment.
The whole EqualityAndHashcodeProvider/ObjectContainer machinery exists for non-trivial key types, but no case has duplicate STRING, ROW or ARRAY keys - the generated equality/hashcode path is never exercised. Please add one, plus an entry with a NULL value.
|
Thanks @VasShabu, first pass above |
Thankyou @gustavodemorais have made the changes as per your request, please let me know if you have any feedback. Thanks |
|
|
||
| /** A {@link MapData} backed directly by a key array and a value array. */ | ||
| @Internal | ||
| public class MapDataContainer implements MapData { |
There was a problem hiding this comment.
Why do we need it as a separate file using only in one place?
There was a problem hiding this comment.
btw there are already 2 similar inner classes in other classes?
Can we reuse instead?
There was a problem hiding this comment.
for instance: extract once and reuse
There was a problem hiding this comment.
+1 both MapFromArraysFunction and MapDataForMapFromArrays have the same logic.
MapFromArraysFunction needs a small adjustment. MapUnionFunction could use MapContainer as-is. I think all three could share this one util
There was a problem hiding this comment.
I think I should unify this MapDataContainer class for other functions in its own PR simply because one of the guidelines in the PR descriptions is that
- Each pull request should address only one issue, not mix up code from multiple issues
So I think it is best to create a new PR to unify the Container classes
Thoughts?
@gustavodemorais @snuyanzin
There was a problem hiding this comment.
you can do this in same PR, however different commit
There was a problem hiding this comment.
Hey @VasShabu, thanks for working on this. Some suggestions
|
|
||
| /** A {@link MapData} backed directly by a key array and a value array. */ | ||
| @Internal | ||
| public class MapDataContainer implements MapData { |
There was a problem hiding this comment.
+1 both MapFromArraysFunction and MapDataForMapFromArrays have the same logic.
MapFromArraysFunction needs a small adjustment. MapUnionFunction could use MapContainer as-is. I think all three could share this one util
2425e71 to
9d25cbe
Compare
snuyanzin
left a comment
There was a problem hiding this comment.
since the AI was used, it should be used Generated-by: instead of Co-authored-by: as mentioned in Apache recommendations https://www.apache.org/legal/generative-tooling.html
also the doc about this is in https://github.com/apache/flink/pull/27776/changes#r2979585031
I have already set it as Generated-by in the pr description |
look at this commit 9d25cbe |
I see will fix that. |
…pe access - Reformat the MAP_FROM_ENTRIES docs to the fenced sql / "-- Returns" style across sql_functions.yml, its zh mirror, BaseExpressions javadoc and the pyflink docstring - Reword the non-array argument error to quote 'input' and report the actual type - Read the entry field types via DataType.getFieldDataTypes() instead of the generic getChildren() Generated-By: Claude Opus 4.8 <noreply@anthropic.com>
9d25cbe to
0fc0fdd
Compare
What is the purpose of the change
The Purpose of this change was to add a new builtin function, MapFromEntries Function to Flink
Brief change log
-Added mapFromEntries functionality
-Wired mapFromEntries function to flink
Verifying this change
Please make sure both new and modified tests in this PR follow the conventions for tests defined in our code quality guide.
(Please pick either of the following options)
This change is already covered by existing tests, such as the ones located in
to run test you need to run this command:
./mvnw -o -pl flink-table/flink-table-planner -Dtest='JsonFunctionsITCase' -Dsurefire.failIfNoSpecifiedTests=false -Dcheckstyle.skip=true -Dspotless.check.skip=true -Drat.skip=true -e -Denforcer.skip=true test
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no)Documentation
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Opus 4.8