feat: Add map_extract module and function #11969

Weijun-H · 2024-08-13T15:07:58Z

Which issue does this PR close?

Closes #11935

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Weijun-H · 2024-08-13T15:09:01Z

support related array tests after #11712

jonahgao · 2024-08-13T15:18:25Z

datafusion/expr-common/src/signature.rs

@@ -146,6 +146,8 @@ pub enum ArrayFunctionSignature {
    /// Specialized Signature for MapArray
    /// The function takes a single argument that must be a MapArray
    MapArray,
+    /// Specialized Signature for MapExtract and similar functions
+    MapArrayAndKey,


Would it be possible to use TypeSignature::UserDefined to avoid enumeration bloat and make it more cohesive?

Only if there are more than 1 functions utilize the same signature, we could add it

dharanad · 2024-08-13T17:29:50Z

I plan to review this tomorrow

docs/source/user-guide/sql/scalar_functions.md

datafusion/functions-nested/src/map_extract.rs

datafusion/common/src/utils/mod.rs

datafusion/functions-nested/src/map_extract.rs

alamb

Thanks @Weijun-H

I noticed that #11935 talks about element_at but this PR proposes adding map_extract

Is map_extract modeled on another Database?

It seems like map_extact is meant to find the key with the corresponding value?

datafusion/sqllogictest/test_files/map.slt

alamb · 2024-08-14T21:58:15Z

docs/source/user-guide/sql/scalar_functions.md

@@ -3700,6 +3701,36 @@ SELECT MAKE_MAP('POST', 41, 'HEAD', 33, 'PATCH', null);
 {POST: 41, HEAD: 33, PATCH: }
 ```

+### `map_extract`


Weijun-H · 2024-08-15T06:53:54Z

Is map_extract modeled on another Database?

It seems like map_extact is meant to find the key with the corresponding value?

The clickhouse provides a similar function mapExtractKeyLike , and DuckDB has the same function to get the value with the corresponding key. @alamb

datafusion/functions-nested/src/map_extract.rs

Weijun-H · 2024-08-15T09:53:23Z

datafusion/sqllogictest/test_files/map.slt

+# map_extract with columns
+query ???
+select map_extract(column1, 1), map_extract(column1, 4), map_extract(column1, 5) from map_array_table_1;
+----
+[1, , 3] NULL NULL
+NULL [1, , 3] [4, , 6]
+NULL NULL NULL
+
+query ???
+select map_extract(column1, column2), map_extract(column1, column3), map_extract(column1, column4) from map_array_table_1;
+----
+[1, , 3] [1, , 3] [1, , 3]
+[4, , 6] [4, , 6] [4, , 6]
+NULL NULL NULL
+
+query ???
+select map_extract(column1, column2), map_extract(column1, column3), map_extract(column1, column4) from map_array_table_2;
+----
+[1, , 3] NULL [1, , 3]
+[4, , 6] NULL [4, , 6]
+NULL NULL NULL
+
+query ???
+select map_extract(column1, 1), map_extract(column1, 4), map_extract(column1, 5) from map_array_table_2;
+----
+[1, , 3] NULL NULL
+NULL [1, , 3] [4, , 6]
+NULL NULL NULL
+


test map_extract in a table

alamb · 2024-08-15T11:03:31Z

The clickhouse provides a similar function mapExtractKeyLike , and DuckDB has the same function to get the value with the corresponding key. @alamb

Thank you -- I found it now

map_extract(map, key)

Description: Alias of element_at. Return a list containing the value for a given key or an empty list if the key is not contained in the map. The type of the key provided in the second parameter must match the type of the map's keys else an error is returned.

Example

map_extract(map([100, 5], [42, 43]), 100)

Result: [42]

alamb · 2024-08-15T11:06:14Z

datafusion/sqllogictest/test_files/map.slt

+select map_extract(MAP {'a': 1, 'b': NULL, 'c': 3}, 'a'), map_extract(MAP {'a': 1, 'b': NULL, 'c': 3}, 'b'),
+       map_extract(MAP {'a': 1, 'b': NULL, 'c': 3}, 'c'), map_extract(MAP {'a': 1, 'b': NULL, 'c': 3}, 'd');
+----
+1 NULL 3 NULL


By my understanding of duckdb's doc the values should be a list not a scalar. In other words the result should be [1], [], [3], [] not 1, NULL, 3, NULL:

D select map_extract(MAP {'a': 1, 'b': NULL, 'c': 3}, 'a'); ┌─────────────────────────────────────────────────────────────────────────────────────────┐ │ map_extract(main.map(main.list_value('a', 'b', 'c'), main.list_value(1, NULL, 3)), 'a') │ │ int32[] │ ├─────────────────────────────────────────────────────────────────────────────────────────┤ │ [1] │ └─────────────────────────────────────────────────────────────────────────────────────────┘ D D select map_extract(MAP {'a': 1, 'b': NULL, 'c': 3}, 'd'); ┌─────────────────────────────────────────────────────────────────────────────────────────┐ │ map_extract(main.map(main.list_value('a', 'b', 'c'), main.list_value(1, NULL, 3)), 'd') │ │ int32[] │ ├─────────────────────────────────────────────────────────────────────────────────────────┤ │ [] │ └─────────────────────────────────────────────────────────────────────────────────────────┘

WHile this choice is strange to me (a list that will always have 1 or 0 elements) I think it will make for an easy implementation -- you shouldn't have to make special implementations for each type of key -- instead you should just be able to calculate the offsets directly and make a list array that points to the keys array

@Weijun-H Here is a like to old converstation about how DataFuion strives not to invent / determine semantics.
#11436 (comment)

I should have pointed it earlier, sorry about that. We should be consistent with postgres/duckdb

I find it unintuitive to return a list in this context because, in a Map, each key is unique by definition. Returning a list implies the possibility of multiple values for a single key, which contradicts the core concept of how a Map operates. The key uniqueness in a Map should naturally lead to a single value or NULL, making a list redundant and potentially confusing. I refactored the code so it can avoid to make special implementations for each type of key now.

I check the history of map_extract in duckdb and find that there is no discussion about it. It is created by one man in early day
https://github.com/duckdb/duckdb/blob/a196a022ffcf427aa21184d295bb70caa42e0655/src/function/scalar/nested/map_extract.cpp

Given the rationale is not clear, I don't have strong opinion to strictly follow what duckdb is in this case

It is created by one man

I think this describes quite a bit of duckdb 😆

Weijun-H · 2024-08-16T02:52:38Z

Changed to follow DuckDB now

dharanad · 2024-08-16T07:57:52Z

datafusion/functions-nested/src/map_extract.rs

+        let query_key = query_keys_array.slice(row_index, 1);
+
+        let value_index = (0..len)
+            .find(|&i| keys.slice(start + i, 1) == Arc::<dyn Array>::clone(&query_key));


Nit: I dont think we need a clone for every iteration. How about creating a clone outside of this loop.

jayzhan211 · 2024-08-16T11:54:28Z

datafusion/functions-nested/src/map_extract.rs

+            return exec_err!("map_extract expects two arguments");
+        }
+        let map_type = &arg_types[0];
+        let map_fields = get_map_entry_field(map_type)?;


It seems like it is better to return data type directly?

The value type is not List, so we cannot return it directly.

datafusion/functions-nested/src/map_extract.rs

jayzhan211

Some comments left, but it is also good to go

Co-authored-by: Jay Zhan <[email protected]>

jayzhan211 · 2024-08-17T09:46:14Z

Thanks All

github-actions bot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) common Related to common crate labels Aug 13, 2024

Weijun-H mentioned this pull request Aug 13, 2024

Access a Map with a non-string keys #11785

Open

jonahgao reviewed Aug 13, 2024

View reviewed changes

github-actions bot added documentation Improvements or additions to documentation and removed logical-expr Logical plan and expressions labels Aug 14, 2024

Weijun-H requested a review from jonahgao August 14, 2024 07:26

dharanad reviewed Aug 14, 2024

View reviewed changes

Weijun-H requested a review from dharanad August 14, 2024 15:10

alamb reviewed Aug 14, 2024

View reviewed changes

Weijun-H added 16 commits August 15, 2024 14:57

feat: Add map_extract module and function

90f72c0

chore: Fix fmt

7cf0003

chore: Add tests

b489853

chore: Simplify

eb78a58

chore: Simplify

92f633f

chore: Fix clippy

dc7df96

doc: Add user doc

c55efd5

feat: use Signature::user_defined

3c28a06

chore: Update tests

0fff39a

chore: Fix fmt

2ca8457

chore: Fix clippy

9ac25f0

chore

5547f5a

chore: typo

bebb3b0

chore: Check args len in return_type

5779753

doc: Update doc

f906e76

chore: Simplify logic

64ef06a

Weijun-H added 5 commits August 15, 2024 14:57

chore: check args earlier

3bd2b51

feat: Support UTF8VIEW

b6e31e8

chore: Update doc

71b01d7

chore: Fic clippy

55bb3c7

refacotr: Use MutableArrayData

5e978b4

Weijun-H force-pushed the 11935-Support-element_at-for-Map-type branch from e0bcabb to 5e978b4 Compare August 15, 2024 09:46

Weijun-H commented Aug 15, 2024

View reviewed changes

datafusion/functions-nested/src/map_extract.rs Outdated Show resolved Hide resolved

Weijun-H commented Aug 15, 2024

View reviewed changes

Weijun-H requested review from alamb and jayzhan211 August 15, 2024 09:54

chore

bebc67d

alamb reviewed Aug 15, 2024

View reviewed changes

Weijun-H added 3 commits August 15, 2024 21:04

refactor: Avoid type conversion

2bb67bd

chore: Fix clippy

514ee77

chore: Follow DuckDB

c5b23ec

Weijun-H force-pushed the 11935-Support-element_at-for-Map-type branch from 594f463 to c5b23ec Compare August 16, 2024 02:51

dharanad approved these changes Aug 16, 2024

View reviewed changes

jayzhan211 reviewed Aug 16, 2024

View reviewed changes

jayzhan211 approved these changes Aug 16, 2024

View reviewed changes

Weijun-H and others added 2 commits August 16, 2024 22:04

Update datafusion/functions-nested/src/map_extract.rs

b4ad476

Co-authored-by: Jay Zhan <[email protected]>

chore: Fix fmt

f48b566

jayzhan211 merged commit 72b6a49 into apache:main Aug 17, 2024
25 checks passed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add map_extract module and function #11969

feat: Add map_extract module and function #11969

Weijun-H commented Aug 13, 2024

Weijun-H commented Aug 13, 2024

jonahgao Aug 13, 2024

jayzhan211 Aug 15, 2024 •

edited

Loading

dharanad commented Aug 13, 2024

alamb left a comment

alamb Aug 14, 2024

Weijun-H commented Aug 15, 2024

Weijun-H Aug 15, 2024 •

edited

Loading

alamb commented Aug 15, 2024

alamb Aug 15, 2024

alamb Aug 15, 2024

dharanad Aug 15, 2024 •

edited

Loading

Weijun-H Aug 15, 2024

jayzhan211 Aug 15, 2024 •

edited

Loading

alamb Aug 15, 2024

Weijun-H commented Aug 16, 2024

dharanad Aug 16, 2024

jayzhan211 Aug 16, 2024

Weijun-H Aug 16, 2024

jayzhan211 left a comment •

edited

Loading

jayzhan211 commented Aug 17, 2024

feat: Add map_extract module and function #11969

feat: Add map_extract module and function #11969

Conversation

Weijun-H commented Aug 13, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Weijun-H commented Aug 13, 2024

Choose a reason for hiding this comment

jayzhan211 Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

dharanad commented Aug 13, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Weijun-H commented Aug 15, 2024

Weijun-H Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

alamb commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dharanad Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Weijun-H commented Aug 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 left a comment • edited Loading

Choose a reason for hiding this comment

jayzhan211 commented Aug 17, 2024

jayzhan211 Aug 15, 2024 •

edited

Loading

Weijun-H Aug 15, 2024 •

edited

Loading

dharanad Aug 15, 2024 •

edited

Loading

jayzhan211 Aug 15, 2024 •

edited

Loading

jayzhan211 left a comment •

edited

Loading