Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[FLINK-31677][table] Add built-in MAP_ENTRIES function. #22312

Merged
merged 4 commits into from Jun 8, 2023
Merged

[FLINK-31677][table] Add built-in MAP_ENTRIES function. #22312

merged 4 commits into from Jun 8, 2023

Conversation

liuyongvs
Copy link
Contributor

@liuyongvs liuyongvs commented Mar 31, 2023 •
edited

  • What is the purpose of the change
    This is an implementation of MAP_ENTRIES

  • Brief change log
    MAP_ENTRIES for Table API and SQL

map_entries(map) - Returns an unordered array of all entries in the given map.
Syntax:
    map_entries(map)
Arguments:
    map: An MAP to be handled.
Returns:Returns an unordered array of all entries in the given map. Returns null if the argument is null

> SELECT map_entries(map[1, 'a', 2, 'b']);
 [(1,"a"),(2,"b")]

See also
presto https://prestodb.io/docs/current/functions/map.html
spark https://spark.apache.org/docs/latest/api/sql/index.html#map_entries

  • Verifying this change
    This change added tests in MapFunctionITCase.

  • 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): (yes )
    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? (yes)
    If yes, how is the feature documented? (docs)

@flinkbot
Copy link
Collaborator

flinkbot commented Mar 31, 2023 •
edited

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@liuyongvs
Copy link
Contributor Author

hi @snuyanzin do you have time to review it?

Aitozi
Aitozi approved these changes Apr 12, 2023
View reviewed changes
Copy link
Contributor

@Aitozi Aitozi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@liuyongvs
Copy link
Contributor Author

hi @dawidwys could you have a look this ,it is long time no one to review and merge?

@liuyongvs
Copy link
Contributor Author

hi @snuyanzin do you have time to review it?

snuyanzin
snuyanzin reviewed May 22, 2023
View reviewed changes
docs/data/sql_functions.yml Outdated
@@ -646,6 +646,9 @@ collection:
- sql: MAP_VALUES(map)
table: MAP.mapValues()
description: Returns the values of the map as array. No order guaranteed.
- sql: MAP_ENTRIES(map)
table: MAP.mapEntries()
description: Returns an unordered array of all entries in the given map. No order guaranteed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what means unordered here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keys are not sorted, i will remove it @snuyanzin

snuyanzin
snuyanzin reviewed May 22, 2023
View reviewed changes
flink-python/docs/reference/pyflink.table/expressions.rst Outdated
@@ -231,6 +231,7 @@ advanced type helper functions
Expression.array_remove
Expression.map_keys
Expression.map_values
Expression.map_entries
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's better to have abc order

snuyanzin
snuyanzin reviewed May 22, 2023
View reviewed changes
flink-python/pyflink/table/expression.py Outdated
@property
def map_entries(self) -> 'Expression':
"""
Returns an unordered array of all entries in the given map. No order guaranteed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same about unordered

snuyanzin
snuyanzin reviewed May 22, 2023
View reviewed changes
.../flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/BaseExpressions.java Outdated
@@ -1396,6 +1397,11 @@ public OutType mapValues() {
return toApiSpecificExpression(unresolvedCall(MAP_VALUES, toExpr()));
}

/** Returns an unordered array of all entries in the given map. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same about unordered

@liuyongvs
Copy link
Contributor Author

liuyongvs commented May 23, 2023 •
edited

hi @snuyanzin rebase and fix your reviews and ci passed

@liuyongvs liuyongvs requested a review from snuyanzin May 23, 2023 02:08
@liuyongvs
Copy link
Contributor Author

hi @snuyanzin do you have time look again?

1 similar comment
@liuyongvs
Copy link
Contributor Author

hi @snuyanzin do you have time look again?

luoyuxia
luoyuxia reviewed Jun 7, 2023
View reviewed changes
Copy link
Contributor

@luoyuxia luoyuxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liuyongvs Thanks for this PR. I left one comment.

.../src/main/java/org/apache/flink/table/types/inference/strategies/SpecificTypeStrategies.java
callContext ->
Optional.of(
DataTypes.ARRAY(
DataTypes.ROW(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to specify the name for each field of the Row type? Btw, seems spark use

      StructType(
        StructField("key", childDataType.keyType, false) ::
        StructField("value", childDataType.valueType, childDataType.valueContainsNull) ::
        Nil)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added @luoyuxia

@liuyongvs liuyongvs requested a review from luoyuxia June 7, 2023 08:43
luoyuxia
luoyuxia approved these changes Jun 7, 2023
View reviewed changes
Copy link
Contributor

@luoyuxia luoyuxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liuyongvs Thanks for updating. LGTM. Could you please rebase master. When test pass, please ping me. I can help merge.

@luoyuxia luoyuxia merged commit 79b06cf into apache:master Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aitozi Aitozi Aitozi approved these changes

@luoyuxia luoyuxia luoyuxia approved these changes

@snuyanzin snuyanzin Awaiting requested review from snuyanzin

Assignees
No one assigned
Labels
component=TableSQL/Planner
Projects
None yet
Milestone
No milestone
5 participants
@liuyongvs @flinkbot @snuyanzin @Aitozi @luoyuxia