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

fix: List of FixedSizeList coercion issue in SQL #14468

Merged
merged 2 commits into from
Feb 5, 2025

Conversation

alan910127
Copy link
Contributor

@alan910127 alan910127 commented Feb 4, 2025

Which issue does this PR close?

Rationale for this change

Currently, FixedSizeList values inside a make_array call are eagerly coerced into List, resulting in non-intuitive behavior.

To improve consistency, this PR aligns the behavior with DuckDB: coercion should only occur when the FixedSizeList is modified. This approach makes coercion more predictable and avoids unnecessary type transformations.

What changes are included in this PR?

This PR removes the coercion logic in make_array when the type union resolves to FixedSizeList, ensuring that make_array no longer performs eager coercion.

Creating List(FixedSizeList)

Previously, make_array would automatically coerce FixedSizeList into List, even when no operations were performed. With this fix, the expected behavior is preserved:

> SELECT arrow_typeof([arrow_cast([1, 2, 3], 'FixedSizeList(3, Int64)')]);
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| arrow_typeof(make_array(arrow_cast(make_array(Int64(1),Int64(2),Int64(3)),Utf8("FixedSizeList(3, Int64)"))))                                                                                                                            |
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| List(Field { name: "item", data_type: FixedSizeList(Field { name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }, 3), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }) |
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row(s) fetched.
Elapsed 0.002 seconds.

Appending After Casting to FixedSizeList

If an element is appended to a FixedSizeList, coercion to List will still occur, as expected:

> SELECT arrow_typeof([array_append(arrow_cast([1, 2, 3], 'FixedSizeList(3, Int64)'), 4)]);
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| arrow_typeof(make_array(array_append(arrow_cast(make_array(Int64(1),Int64(2),Int64(3)),Utf8("FixedSizeList(3, Int64)")),Int64(4))))                                                                                         |
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| List(Field { name: "item", data_type: List(Field { name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }) |
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row(s) fetched.
Elapsed 0.002 seconds.

Are these changes tested?

One of the test cases in sqllogictest has been modified to reflect the updated behavior.

Are there any user-facing changes?

Yes, users who previously relied on make_array coercing FixedSizeList into List by default will see a change in behavior. Now, coercion only occurs when modifications (e.g., array_append) are applied.

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Feb 4, 2025
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.

Thank you for this contribution @alan910127 ❤️

List(Field { name: "item", data_type: List(Field { name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} })
List(Field { name: "item", data_type: List(Field { name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} })

List(Field { name: "item", data_type: FixedSizeList(Field { name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }, 2), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} })
Copy link
Contributor

Choose a reason for hiding this comment

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

List of FixedSizeList makes more sense to me

@alan910127
Copy link
Contributor Author

Thank you for this contribution @alan910127 ❤️

Thank you, @alamb! 😊 Glad to contribute! Let me know if there's anything else I can improve.

@alamb alamb merged commit 22a2061 into apache:main Feb 5, 2025
26 checks passed
@alamb
Copy link
Contributor

alamb commented Feb 5, 2025

🚀 -- I normally try to leave PRs open for 24 hours before merging them to ensure other committers in different timezones have a chance to review too if they like

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot create a List of FixedSizedList in SQL
2 participants