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

Support dictionary data type in array_to_string #10908

Merged
merged 10 commits into from
Jun 23, 2024
Merged

Support dictionary data type in array_to_string #10908

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
13b97c1
Support dictionary data type in array_to_string
EduardoVega Jun 13, 2024
7aa1cde
Fix import
EduardoVega Jun 16, 2024
0734cc9
Some tests
EduardoVega Jun 16, 2024
f638a09
Update datafusion/functions-array/src/string.rs
EduardoVega Jun 17, 2024
2af78d2
Merge remote-tracking branch 'apache/main' into support-array-to-stri…
alamb Jun 17, 2024
9fb5793
Add some tests showing incorrect results
alamb Jun 17, 2024
7b3da13
Get logical array
EduardoVega Jun 19, 2024
f58131c
Merge remote-tracking branch 'apache/main' into support-array-to-stri…
alamb Jun 21, 2024
f0434db
apply rust fmt
alamb Jun 21, 2024
5a5397b
Simplify implementation, avoid panics
alamb Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions datafusion/functions-array/src/string.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
//! [`ScalarUDFImpl`] definitions for array_to_string and string_to_array functions.

use arrow::array::{
Array, ArrayRef, BooleanArray, Float32Array, Float64Array, GenericListArray,
Array, ArrayRef, AsArray, BooleanArray, Float32Array, Float64Array, GenericListArray,
Int16Array, Int32Array, Int64Array, Int8Array, LargeStringArray, ListBuilder,
OffsetSizeTrait, StringArray, StringBuilder, UInt16Array, UInt32Array, UInt64Array,
UInt8Array,
Expand All @@ -31,7 +31,7 @@ use datafusion_common::{plan_err, DataFusionError, Result};
use std::any::{type_name, Any};

use crate::utils::{downcast_arg, make_scalar_function};
use arrow_schema::DataType::{FixedSizeList, LargeList, LargeUtf8, List, Null, Utf8};
use arrow_schema::DataType::{FixedSizeList, LargeList, LargeUtf8, List, Null, Utf8, Dictionary};
use datafusion_common::cast::{
as_generic_string_array, as_large_list_array, as_list_array, as_string_array,
};
Expand Down Expand Up @@ -281,6 +281,24 @@ 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( || {
DataFusionError::Internal(
format!("could not cast {} to AnyDictionaryArray", arr.data_type())
)
})?;
compute_array_to_string(
EduardoVega marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the feedback, let me figure it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

arg,
any_dict_array.values().clone(),
delimiter.clone(),
null_string.clone(),
with_null_string,
)?;

Ok(arg)
}
Null => Ok(arg),
data_type => {
macro_rules! array_function {
Expand Down
16 changes: 16 additions & 0 deletions datafusion/sqllogictest/test_files/array.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3769,6 +3769,22 @@ select array_to_string(make_array(), ',')
----
(empty)

# array to string dictionary
statement ok
CREATE TABLE table1 AS VALUES (1, arrow_cast('foo', 'Dictionary(Int32, Utf8)')), (2, arrow_cast('bar', 'Dictionary(Int32, Utf8)'));

query T
SELECT array_to_string(array_agg(column2),',') FROM (SELECT column2 FROM table1);
----
foo,bar

statement ok
CREATE TABLE table2 AS VALUES (1, arrow_cast(1, 'Dictionary(Int32, Int32)')), (2, arrow_cast(2, 'Dictionary(Int32, Int32)'));

query T
SELECT array_to_string(array_agg(column2),'-') FROM (SELECT column2 FROM table2);
----
1-2

## array_union (aliases: `list_union`)

Expand Down
Loading