Skip to content

Support Duration and Union types in ScalarValue::iter_to_array #10139

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
Merged
alamb merged 1 commit into from
Apr 21, 2024
Merged

Support Duration and Union types in ScalarValue::iter_to_array #10139

alamb merged 1 commit into from
Apr 21, 2024

Conversation

@joroKr21
Copy link
Contributor

@joroKr21 joroKr21 commented Apr 19, 2024

Which issue does this PR close?

Closes #10132.

Rationale for this change

ScalarValue::iter_to_array is a building block for many aggregation accumulators, so it should support as many data types as possible.

What changes are included in this PR?

Add support for Duration and Union types to ScalarValue::iter_to_array.

Are these changes tested?

Tested Duration. Not sure how we can test Union.

Are there any user-facing changes?

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Apr 19, 2024
@joroKr21 joroKr21 mentioned this pull request Apr 20, 2024
Merged
alamb
alamb approved these changes Apr 21, 2024
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Makes sense to me -- thank you @joroKr21

Tested Duration. Not sure how we can test Union.

Yeah, I agree -- I don't know how to create UNION arrays easily in SQL -- I think we would need to create them programatically (perhaps registering a table specially for similarly to

datafusion/datafusion/sqllogictest/src/test_context.rs

Lines 75 to 78 in 16e3831

match file_name {
"information_schema_table_types.slt" => {
info!("Registering local temporary table");
register_temp_table(test_ctx.session_ctx()).await;
)

Thought I reviewed the tests and I don't see much coverage of DataType::Union at all. I'll file a ticket to add the coverage

@alamb alamb mentioned this pull request Apr 21, 2024
Open
@alamb
Copy link
Contributor

alamb commented Apr 21, 2024

I am not sure how important UnionArray is to your usecase @joroKr21 / @thinkharderdev but it might be worth considering investing in some additional DataFusion testing to make sure it isn't broken

I filed #10161 with some ideas

@alamb alamb merged commit eb72deb into apache:main Apr 21, 2024
@joroKr21 joroKr21 deleted the iter-to-array branch April 21, 2024 15:25
@jayzhan211 jayzhan211 mentioned this pull request Apr 24, 2024
Closed
ccciudatu pushed a commit to hstack/datafusion that referenced this pull request Apr 26, 2024
@joroKr21 @ccciudatu
Support Duration and Union types in ScalarValue::iter_to_array (apach…
78b92dc 
…e#10139)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

Assignees

No one assigned

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ScalarValue::iter_to_array doesn't support Duration and Union types

2 participants

@joroKr21 @alamb