feat: cast (Large)List to FixedSizeList #5081

wjones127 · 2023-11-16T00:29:41Z

Which issue does this PR close?

Rationale for this change

Adds cast from list to FixedSizeList. I intend to use this in DataFusion, where I'd like to allow users to write array literals in SQL (which default to List) and have the planner cast them to FixedSizeList when appropriate.

What changes are included in this PR?

cast_with_options() now supports List to FixedSizeList.

Are there any user-facing changes?

This adds a new feature. A suite of tests are added. Let me know if you think there are important cases missing.

tustvold · 2023-11-16T10:09:47Z

Perhaps we could provide these on the arrays themselves and then use this within the cast kernel, much like we do for the reverse transformation - https://docs.rs/arrow-array/latest/arrow_array/array/struct.GenericListArray.html#impl-From%3CFixedSizeListArray%3E-for-GenericListArray%3COffsetSize%3E

wjones127 · 2023-11-16T16:23:21Z

@tustvold I'm actually not sure how to satisfy the test_can_cast_types test. Right now can_cast_types() can validate the child types can be cast, but can't validate the lengths are of appropriate size.

One option is to change the implementation so that if safe is true, then we truncate or fill nulls into the arrays to coerce them into the correct size. That brings issues though if the output field is not nullable.

Are there other cases of types where can_cast_types() isn't a strong guarantee that the cast() function won't error?

wjones127 · 2023-11-16T16:24:38Z

Perhaps we could provide these on the arrays themselves and then use this within the cast kernel, much like we do for the reverse transformation

I can look at this. The reason I didn't put it there was I didn't think I could bring the cast kernel into that file, since we want to cast the child values. But maybe that's okay? Or I can refactor somehow to do the child cast in cast.rs? I'll see what I can do.

tustvold · 2023-11-16T16:39:52Z

Are there other cases of types where can_cast_types() isn't a strong guarantee that the cast() function won't error?

The string parsing ones

I would expect when safe is true to fill the FixedSizeListArray with a null value for the errored index. This is consistent with how it behaves for other casts. The nullability concern is valid, and is why DataFusion has both CastExpr and TryCastExpr.

Tbh I'm not a fan of the safe option, but it is important for spark compatibility

Or I can refactor somehow to do the child cast in cast.rs? I'll see what I can do.

Aah forgot about that, perhaps not worth it then

wjones127 · 2023-11-16T17:03:28Z

Tbh I'm not a fan of the safe option

Ha I always get confused as to whether "safe" means "I will silently create nulls" or "I will raise an error if the cast cannot be done". PyArrow has a somewhat opposite definition of safe.

wjones127 · 2023-11-17T03:08:50Z

arrow-cast/src/cast.rs

+/// * List to FixedSizeList: the underlying data type is cast. If the list size is different
+///   than the FixedSizeList size and safe casting is requested, then lists are
+///   truncated or filled will some value to achieve the requested size. If the output
+///   field is nullable, the fill value is null, otherwise it is the first value.


I don't love this behavior, but I'm not sure a better way in the current framework. Personally, I don't think I ever care to use safe=True here. But I'd also like it to not be too shocking to those that do use it.

We allow nulls in non-nullable children provided they are masked by the parents null mask.

I'm not sure about truncating, I'll think on it

Ah I hadn't thought about having the list itself just making anything improperly sized be null. That seems like it would be reasonable behavior. We can ignore truncation, I don't think we want that.

tustvold · 2023-11-17T09:49:04Z

I plan to have a play with this today to see if I can't come up with something a little simpler

tustvold · 2023-11-17T13:25:24Z

I've pushed a PR that uses MutableArrayData to avoid doing multiple passes of the array, I think it should be faster and I find it a bit easier to follow. PTAL and let me know what you think

wjones127 · 2023-11-17T16:27:03Z

arrow-cast/src/cast.rs

+                }
+                mutable.extend_nulls(size as _);
+                nulls.as_mut().unwrap().set_bit(idx, false);
+                last_pos = end_pos


I think this works, but I found the logic of last_pos confusing. I'd like to add a comment about how it pads

When we detect a list doesn't start at the correct point
we use mutable.extend() to pad the previous list. last_pos
tracks the ending position of the last incorrectly sized list.

However, I'm still confused on where the truncation of too long values happens. Where is that?

It doesn't it replaces it with null or errors, why do you want it to truncate?

I don't mean truncate logically. I mean that if a section of values has list_size + 1 elements, won't that shift all the subsequent values to the right by 1? I would think to solve this we need to make sure truncate / shift values over to the left as needed.

If a section of values has list_size + 1 elements, we would add nulls, and then set last_pos to the end of that slice. I will add a commit explicitly testing this and some more comments

Sorry, I see how this works now. I misunderstood how MutableArrayData works, but re-reading the docs this makes sense now.

arrow-cast/src/cast.rs

wjones127 · 2023-11-17T17:46:09Z

I can't approve my own PR, but this LGTM. Thanks for your help @tustvold. MutableArrayData is quite a nice abstraction!

tustvold · 2023-11-17T17:47:57Z

MutableArrayData is quite a nice abstraction!

Until you look under the hood at least 😅, it is probably not how I would build such a construct today, but it is useful for sure.

wjones127 added arrow Changes to the arrow crate enhancement Any new improvement worthy of a entry in the changelog labels Nov 16, 2023

feat: cast (Large)List to FixedSizeList

456c883

wjones127 force-pushed the cast-list-fsl branch from e5aed11 to 456c883 Compare November 16, 2023 00:34

fix: support 'safe' casting of list to FSL

ba214ab

wjones127 commented Nov 17, 2023

View reviewed changes

fix: if target is non-null, use non-null sentinel value

ad87f8e

wjones127 force-pushed the cast-list-fsl branch from d422617 to ad87f8e Compare November 17, 2023 03:10

Use MutableArrayData

a245f07

wjones127 commented Nov 17, 2023

View reviewed changes

tustvold force-pushed the cast-list-fsl branch from ea8d3d7 to 2725ee4 Compare November 17, 2023 17:21

wjones127 commented Nov 17, 2023

View reviewed changes

arrow-cast/src/cast.rs Outdated Show resolved Hide resolved

Docs

d474e38

tustvold force-pushed the cast-list-fsl branch from 2725ee4 to d474e38 Compare November 17, 2023 17:29

tustvold approved these changes Nov 17, 2023

View reviewed changes

wjones127 merged commit dc75a28 into apache:master Nov 17, 2023
26 checks passed

wjones127 deleted the cast-list-fsl branch November 17, 2023 18:09

wjones127 mentioned this pull request Feb 7, 2024

feat: support casting FixedSizeList with new child type #5360

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: cast (Large)List to FixedSizeList #5081

feat: cast (Large)List to FixedSizeList #5081

wjones127 commented Nov 16, 2023

tustvold commented Nov 16, 2023

wjones127 commented Nov 16, 2023

wjones127 commented Nov 16, 2023

tustvold commented Nov 16, 2023 •

edited

Loading

wjones127 commented Nov 16, 2023

wjones127 Nov 17, 2023 •

edited

Loading

tustvold Nov 17, 2023 •

edited

Loading

wjones127 Nov 17, 2023

tustvold commented Nov 17, 2023

tustvold commented Nov 17, 2023

wjones127 Nov 17, 2023

tustvold Nov 17, 2023 •

edited

Loading

wjones127 Nov 17, 2023

tustvold Nov 17, 2023

wjones127 Nov 17, 2023

wjones127 commented Nov 17, 2023

tustvold commented Nov 17, 2023

feat: cast (Large)List to FixedSizeList #5081

feat: cast (Large)List to FixedSizeList #5081

Conversation

wjones127 commented Nov 16, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold commented Nov 16, 2023

wjones127 commented Nov 16, 2023

wjones127 commented Nov 16, 2023

tustvold commented Nov 16, 2023 • edited Loading

wjones127 commented Nov 16, 2023

wjones127 Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

tustvold Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

wjones127 Nov 17, 2023

Choose a reason for hiding this comment

tustvold commented Nov 17, 2023

tustvold commented Nov 17, 2023

wjones127 Nov 17, 2023

Choose a reason for hiding this comment

tustvold Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

wjones127 Nov 17, 2023

Choose a reason for hiding this comment

tustvold Nov 17, 2023

Choose a reason for hiding this comment

wjones127 Nov 17, 2023

Choose a reason for hiding this comment

wjones127 commented Nov 17, 2023

tustvold commented Nov 17, 2023

tustvold commented Nov 16, 2023 •

edited

Loading

wjones127 Nov 17, 2023 •

edited

Loading

tustvold Nov 17, 2023 •

edited

Loading

tustvold Nov 17, 2023 •

edited

Loading