Support dictionary data type in array_to_string #10908

EduardoVega · 2024-06-14T00:42:30Z

Which issue does this PR close?

Rationale for this change

Go to issue.

What changes are included in this PR?

Dictionary is added as a supported data type in compute_array_to_string, so that it can properly handle a Dictionary Array.

Are these changes tested?

Yes

Are there any user-facing changes?

alamb

Thank you su much for this contribution @EduardoVega

Can you please add some tests, ideally in sqllogicteset format?

Here are the instructions: https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest

Ideally you should be able to extend one of the existing test files in https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest/test_files with the @appletreeisyellow 's reproduer from #10862

For example, this might be a good spit

datafusion/datafusion/sqllogictest/test_files/array.slt

Line 3752 in cc60278

## array_to_string (aliases: `list_to_string`, `array_join`, `list_join`)

datafusion/functions-array/src/string.rs

Co-authored-by: Alex Huang <[email protected]>

…ng-for-dict

alamb

Thank you again for this PR @EduardoVega -- very much appreciated

I still think this PR is not quite correct as it is operating on the dictionary rather than the logical values of the dictionary array.

I pushed a new commit to your branch with some additional tests that I think show wrong results.

I can help fix the code too, but figured I would leave the feedback in case you wanted to do so.

alamb · 2024-06-17T16:23:59Z

datafusion/functions-array/src/string.rs

+                            format!("could not cast {} to AnyDictionaryArray", arr.data_type())
+                        )
+                    })?;
+                compute_array_to_string(


I pushed some tests showing how this results in incorrect values.

I think you may be able to call to_string! directly to iterate over the values of the dictionary.

thanks for the feedback, let me figure it out.

@alamb thanks for the tests, I think I have a better understanding of DictionaryArray.

Based on my understanding, I converted dictionary keys and values to something iterable. Then using the keys to access the values I got the logical array. I think tests are now showing correct results.

Finally, I am happy to work on any new feedback I receive.

Amazing @EduardoVega -- thank you so much -- this is quite an impressive first contribution. I didn't realize it was going to be quite as tricky.

…ng-for-dict

alamb

Thank you so much for working on this @EduardoVega -- it is quite a bit more complicated than I realized.

alamb · 2024-06-21T21:36:33Z

datafusion/functions-array/src/string.rs

@@ -281,6 +283,49 @@ pub(super) fn array_to_string_inner(args: &[ArrayRef]) -> Result<ArrayRef> {

                Ok(arg)
            }
+            Dictionary(_, _) => {
+                let any_dict_array = arr.as_any_dictionary_opt().ok_or_else(|| {


Thank you @EduardoVega -- it is very impressive you got this to work

I am somewhat concerned that this is quite a bit of code (it actually handles dictionaries of any type, not just string dictionaries, and that it also supports arbitrary key types -- not just the integer types specified)

I am going to try and simplify this a bit

I pushed 5a5397b that I think simplifies the implementation a bit -- let me know what you think

alamb · 2024-06-21T21:37:06Z

datafusion/functions-array/src/string.rs

+                    ($ARRAY_TYPE:ident) => {{
+                        for x in downcast_arg!(any_dict_array.keys(), $ARRAY_TYPE) {
+                            if let Some(x) = x {
+                                keys.push(x.to_string().parse::<usize>().unwrap());


this works, but converts integers to strings just to parse them again -- I think we can just support string dictionaries and avoid this

alamb · 2024-06-23T10:14:48Z

Thanks again @EduardoVega and @Weijun-H

* Support dictionary data type in array_to_string * Fix import * Some tests * Update datafusion/functions-array/src/string.rs Co-authored-by: Alex Huang <[email protected]> * Add some tests showing incorrect results * Get logical array * apply rust fmt * Simplify implementation, avoid panics --------- Co-authored-by: Alex Huang <[email protected]> Co-authored-by: Andrew Lamb <[email protected]>

Support dictionary data type in array_to_string

13b97c1

alamb reviewed Jun 14, 2024

View reviewed changes

datafusion/functions-array/src/string.rs Show resolved Hide resolved

EduardoVega added 2 commits June 16, 2024 10:19

Fix import

7aa1cde

Some tests

0734cc9

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jun 16, 2024

Weijun-H reviewed Jun 17, 2024

View reviewed changes

datafusion/functions-array/src/string.rs Outdated Show resolved Hide resolved

EduardoVega and others added 3 commits June 17, 2024 06:40

Update datafusion/functions-array/src/string.rs

f638a09

Co-authored-by: Alex Huang <[email protected]>

Merge remote-tracking branch 'apache/main' into support-array-to-stri…

2af78d2

…ng-for-dict

Add some tests showing incorrect results

9fb5793

alamb reviewed Jun 17, 2024

View reviewed changes

alamb marked this pull request as draft June 17, 2024 16:24

EduardoVega and others added 3 commits June 19, 2024 17:38

Get logical array

7b3da13

Merge remote-tracking branch 'apache/main' into support-array-to-stri…

f58131c

…ng-for-dict

apply rust fmt

f0434db

alamb reviewed Jun 21, 2024

View reviewed changes

Simplify implementation, avoid panics

5a5397b

alamb marked this pull request as ready for review June 21, 2024 21:45

alamb approved these changes Jun 21, 2024

View reviewed changes

alamb merged commit 8d8dd90 into apache:main Jun 23, 2024
23 checks passed

appletreeisyellow mentioned this pull request Jun 24, 2024

WIP(iox-10892): patched df upgrade 202-06-16 influxdata/arrow-datafusion#28

Closed

appletreeisyellow mentioned this pull request Jun 25, 2024

WIP(iox-10893): patched df upgrade 2024-06-22 influxdata/arrow-datafusion#29

Closed

appletreeisyellow mentioned this pull request Jun 26, 2024

WIP(iox-10893): patched df upgrade 2024-06-26 influxdata/arrow-datafusion#30

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support dictionary data type in array_to_string #10908

Support dictionary data type in array_to_string #10908

EduardoVega commented Jun 14, 2024 •

edited

Loading

alamb left a comment

alamb left a comment

alamb Jun 17, 2024

EduardoVega Jun 17, 2024

EduardoVega Jun 19, 2024

alamb Jun 21, 2024

alamb left a comment

alamb Jun 21, 2024

alamb Jun 21, 2024

alamb Jun 21, 2024

alamb commented Jun 23, 2024

Support dictionary data type in array_to_string #10908

Support dictionary data type in array_to_string #10908

Conversation

EduardoVega commented Jun 14, 2024 • edited Loading

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?

alamb left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Jun 17, 2024

Choose a reason for hiding this comment

EduardoVega Jun 17, 2024

Choose a reason for hiding this comment

EduardoVega Jun 19, 2024

Choose a reason for hiding this comment

alamb Jun 21, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Jun 21, 2024

Choose a reason for hiding this comment

alamb Jun 21, 2024

Choose a reason for hiding this comment

alamb Jun 21, 2024

Choose a reason for hiding this comment

alamb commented Jun 23, 2024

EduardoVega commented Jun 14, 2024 •

edited

Loading