You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "viirya (via GitHub)" <gi...@apache.org> on 2023/12/19 20:37:10 UTC
[PR] Consider nullability when casting between structs [arrow-rs]
viirya opened a new pull request, #5226:
URL: https://github.com/apache/arrow-rs/pull/5226
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Different to casting from primitive types, casting between structs needs to consider nullability as the fields of structs have nullability properties. Query engines e.g., Spark usually use field nullability to do query optimization so it is necessary to make sure content nullability is consistent with metadata nullability.
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use `try_new` when casting between structs to propagate error [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5226:
URL: https://github.com/apache/arrow-rs/pull/5226#discussion_r1432362499
##########
arrow-cast/src/cast.rs:
##########
@@ -160,11 +160,18 @@ pub fn can_cast_types(from_type: &DataType, to_type: &DataType) -> bool {
(Decimal128(_, _) | Decimal256(_, _), Utf8 | LargeUtf8) => true,
// Utf8 to decimal
(Utf8 | LargeUtf8, Decimal128(_, _) | Decimal256(_, _)) => true,
- (Struct(from_fields), Struct(to_fields)) => {
Review Comment:
Do we still need to update can_cast_types?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consider nullability when casting between structs [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #5226:
URL: https://github.com/apache/arrow-rs/pull/5226#discussion_r1431916903
##########
arrow-cast/src/cast.rs:
##########
@@ -160,11 +160,18 @@ pub fn can_cast_types(from_type: &DataType, to_type: &DataType) -> bool {
(Decimal128(_, _) | Decimal256(_, _), Utf8 | LargeUtf8) => true,
// Utf8 to decimal
(Utf8 | LargeUtf8, Decimal128(_, _) | Decimal256(_, _)) => true,
- (Struct(from_fields), Struct(to_fields)) => {
Review Comment:
Btw, "tab" was used in the merged code. It made incorrect ident in diff display. I fixed it to use space.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use `try_new` when casting between structs to propagate error [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #5226:
URL: https://github.com/apache/arrow-rs/pull/5226#discussion_r1432376959
##########
arrow-cast/src/cast.rs:
##########
@@ -160,11 +160,18 @@ pub fn can_cast_types(from_type: &DataType, to_type: &DataType) -> bool {
(Decimal128(_, _) | Decimal256(_, _), Utf8 | LargeUtf8) => true,
// Utf8 to decimal
(Utf8 | LargeUtf8, Decimal128(_, _) | Decimal256(_, _)) => true,
- (Struct(from_fields), Struct(to_fields)) => {
Review Comment:
You mean tab -> space fix or the added comment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consider nullability when casting between structs [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5226:
URL: https://github.com/apache/arrow-rs/pull/5226#issuecomment-1863603694
Perhaps we could use StructArray::try_new which will validate nullability and return an error, if we aren't already. I think this would still preserve the invariant that nullable columns don't obtain nulls?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use `try_new` when casting between structs to propagate error [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #5226:
URL: https://github.com/apache/arrow-rs/pull/5226
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use `try_new` when casting between structs to propagate error [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5226:
URL: https://github.com/apache/arrow-rs/pull/5226#discussion_r1432378552
##########
arrow-cast/src/cast.rs:
##########
@@ -160,11 +160,18 @@ pub fn can_cast_types(from_type: &DataType, to_type: &DataType) -> bool {
(Decimal128(_, _) | Decimal256(_, _), Utf8 | LargeUtf8) => true,
// Utf8 to decimal
(Utf8 | LargeUtf8, Decimal128(_, _) | Decimal256(_, _)) => true,
- (Struct(from_fields), Struct(to_fields)) => {
Review Comment:
Sorry, I saw modifications and thought you hadn't updated this 😅
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consider nullability when casting between structs [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #5226:
URL: https://github.com/apache/arrow-rs/pull/5226#discussion_r1431917969
##########
arrow-cast/src/cast.rs:
##########
@@ -160,11 +160,18 @@ pub fn can_cast_types(from_type: &DataType, to_type: &DataType) -> bool {
(Decimal128(_, _) | Decimal256(_, _), Utf8 | LargeUtf8) => true,
// Utf8 to decimal
(Utf8 | LargeUtf8, Decimal128(_, _) | Decimal256(_, _)) => true,
- (Struct(from_fields), Struct(to_fields)) => {
- from_fields.len() == to_fields.len() &&
- from_fields.iter().zip(to_fields.iter()).all(|(f1, f2)| {
- can_cast_types(f1.data_type(), f2.data_type())
- })
+ (Struct(from_fields), Struct(to_fields)) => {
+ from_fields.len() == to_fields.len() &&
+ from_fields.iter().zip(to_fields.iter()).all(|(f1, f2)| {
+ // Only allows cast nullable or non-nullable fields to nullable fields.
+ // Although it is generally allowed to cast non-nullable fields to non-nullable fields,
+ // but if casting f1.data_type to f2.data_type may return null, e.g., overflow,
+ // it is not allowed when f2 is non-nullable. Because `can_cast_types` doesn't
+ // take `CastOptions` so we cannot check `safe` option here. We take safer
+ // approach to assume `safe` is true, so disallowing the case of casting non-nullable
+ // fields to non-nullable fields.
+ f2.is_nullable() && can_cast_types(f1.data_type(), f2.data_type())
Review Comment:
I took a safer approach here, except for we want to change `can_cast_types` to take `CastOptions` as new parameter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use `try_new` when casting between structs to propagate error [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #5226:
URL: https://github.com/apache/arrow-rs/pull/5226#discussion_r1432377995
##########
arrow-cast/src/cast.rs:
##########
@@ -160,11 +160,18 @@ pub fn can_cast_types(from_type: &DataType, to_type: &DataType) -> bool {
(Decimal128(_, _) | Decimal256(_, _), Utf8 | LargeUtf8) => true,
// Utf8 to decimal
(Utf8 | LargeUtf8, Decimal128(_, _) | Decimal256(_, _)) => true,
- (Struct(from_fields), Struct(to_fields)) => {
Review Comment:
I think it is better to fix the tab with space, otherwise the diff always shows a wrong indent.
The comment is good to have to explain why nullability is ignored there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consider nullability when casting between structs [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #5226:
URL: https://github.com/apache/arrow-rs/pull/5226#issuecomment-1863645356
> Perhaps we could use StructArray::try_new which will validate nullability and return an error, if we aren't already. I think this would still preserve the invariant that nullable columns don't obtain nulls?
>
> Or to phrase it differently, the array constructors already verify this, so perhaps this is sufficient??
Ah, that's good to know `StructArray` already does the check. But currently the cast kernel doesn't use `try_new` but `new`. Let me update it and add corresponding test.
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use `try_new` when casting between structs to propagate error [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #5226:
URL: https://github.com/apache/arrow-rs/pull/5226#issuecomment-1864956175
Thank you @tustvold
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org