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.

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

Support map_keys & map_values for MAP type #12194

Merged
merged 17 commits into from
Sep 1, 2024

Conversation

dharanad
Copy link
Contributor

@dharanad dharanad commented Aug 27, 2024

Which issue does this PR close?

Closes #12147 & #12148

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the common Related to common crate label Aug 27, 2024
@dharanad dharanad changed the title Support map_values for MAP type Support map_keys for MAP type Aug 27, 2024
@dharanad
Copy link
Contributor Author

Encounter a bug. Fixed it by changing field name to item. But why ?

Internal error: Failed due to a difference in schemas, original schema: DFSchema { inner: Schema { fields: [Field { name: "map_keys(map(make_array(Int64(1),Int64(2),Int64(3)),make_array(Int64(1),Int64(2),Int64(3))))", data_type: List(Field { name: "keys", data_type: Int64, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} }), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }], metadata: {} }, field_qualifiers: [None], functional_dependencies: FunctionalDependencies { deps: [] } }, new schema: DFSchema { inner: Schema { fields: [Field { name: "map_keys(map(make_array(Int64(1),Int64(2),Int64(3)),make_array(Int64(1),Int64(2),Int64(3))))", data_type: List(Field { name: "item", data_type: Int64, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} }), nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} }], metadata: {} }, field_qualifiers: [None], functional_dependencies: FunctionalDependencies { deps: [] } }.

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Aug 27, 2024
@dharanad dharanad marked this pull request as ready for review August 27, 2024 10:42
@Weijun-H
Copy link
Member

Encounter a bug. Fixed it by changing field name to item. But why ?

Internal error: Failed due to a difference in schemas, original schema: DFSchema { inner: Schema { fields: [Field { name: "map_keys(map(make_array(Int64(1),Int64(2),Int64(3)),make_array(Int64(1),Int64(2),Int64(3))))", data_type: List(Field { name: "keys", data_type: Int64, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} }), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }], metadata: {} }, field_qualifiers: [None], functional_dependencies: FunctionalDependencies { deps: [] } }, new schema: DFSchema { inner: Schema { fields: [Field { name: "map_keys(map(make_array(Int64(1),Int64(2),Int64(3)),make_array(Int64(1),Int64(2),Int64(3))))", data_type: List(Field { name: "item", data_type: Int64, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} }), nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} }], metadata: {} }, field_qualifiers: [None], functional_dependencies: FunctionalDependencies { deps: [] } }.

I think it is caused by the pre-defined field name in make_array.

/// Returns a map created from a key list and a value list
pub fn map(keys: Vec<Expr>, values: Vec<Expr>) -> Expr {
let keys = make_array(keys);
let values = make_array(values);
Expr::ScalarFunction(ScalarFunction::new_udf(map_udf(), vec![keys, values]))
}

Copy link
Member

@Weijun-H Weijun-H left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @dharanad

…-support-map-values

# Conflicts:
#	datafusion/functions-nested/src/lib.rs
@dharanad dharanad requested a review from jayzhan211 August 28, 2024 18:06
@Weijun-H
Copy link
Member

@dharanad could you add some doc for this function?

@dharanad dharanad changed the title Support map_keys for MAP type Support map_keys & map_values for MAP type Aug 29, 2024
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Aug 29, 2024
@dharanad
Copy link
Contributor Author

Sorry for the mess, i thought since the change was similar for both map_keys && map_values why not club it.

@dharanad dharanad requested review from Weijun-H and Blizzara August 29, 2024 20:25
@dharanad dharanad requested a review from Weijun-H August 31, 2024 08:30
@Weijun-H
Copy link
Member

After fixing CI, it is ready to go

@dharanad
Copy link
Contributor Author

After fixing CI, it is ready to go

@Weijun-H Any idea, how do i fix the docs formatting issue

@Weijun-H
Copy link
Member

Weijun-H commented Aug 31, 2024

After fixing CI, it is ready to go

@Weijun-H Any idea, how do i fix the docs formatting issue

You could check here https://datafusion.apache.org/contributor-guide/howtos.html#how-to-format-md-document

Co-authored-by: Alex Huang <[email protected]>
@Weijun-H
Copy link
Member

ship it!

@dharanad dharanad requested a review from jayzhan211 August 31, 2024 13:09
@Weijun-H Weijun-H merged commit 8746e07 into apache:main Sep 1, 2024
25 checks passed
@Weijun-H
Copy link
Member

Weijun-H commented Sep 1, 2024

Thanks @dharanad contribution, and @Blizzara and @jayzhan211 for reviewing.

@dharanad
Copy link
Contributor Author

dharanad commented Sep 3, 2024

Thank You @Weijun-H @jayzhan211 @Blizzara

@dharanad dharanad deleted the feat/12147-support-map-keys branch September 3, 2024 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Related to common crate documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support map_keys function for Map type
4 participants