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

allow zero sized empty fixed #4626

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

smiklos
Copy link
Contributor

@smiklos smiklos commented Aug 2, 2023

Which issue does this PR close?

Closes #4623.

Rationale for this change

Technically it's valid to construct fixed sized lists with 0 length values.

What changes are included in this PR?

The constructor will not panic if 0 is passed for value size. FixedsizeList's length will be the length of values array in this case.

Are there any user-facing changes?

No

No

@github-actions github-actions bot added the arrow Changes to the arrow crate label Aug 2, 2023
@smiklos
Copy link
Contributor Author

smiklos commented Aug 2, 2023

@tustvold

My concern with this is that FixedSizeList::value_offset will always return 0 if value_length is 0.
This could be an issue if callers expect FixedSizeList::value to panic if the index is out of bound, which it won't rn.

@tustvold
Copy link
Contributor

tustvold commented Aug 2, 2023

I don't see an issue with adding a length check to FixedSizeList::value, ultimately it is doing a dyn dispatch followed by potentially multiple atomic ref increments / allocations, so the additional branch is not going to make any difference. The value APIs are not an efficient way to interact with nested data, but are purely there for ergonomics

@smiklos
Copy link
Contributor Author

smiklos commented Aug 2, 2023

Looking at it more I think the values array should be always empty if value_length is empty. After all what would we even store there in that case? This is actually validated for in the builder variant but not in FixedSizeListArray::try_new.

I think the right solution is to add this validation to try_new such that we check that values.len() = value_length * len.

Then I believe ::value(i) for any i should just blow up if value_length is 0 coz the values array is than empty, and can't be indexed into

@tustvold
Copy link
Contributor

tustvold commented Aug 2, 2023

In general we only verify that buffers and children are at least as long as required, and don't error if they are longer than necessary. We could change this, but we should probably change that consistently if we do so.

@smiklos
Copy link
Contributor Author

smiklos commented Aug 2, 2023

Then perhaps it's enough what is already implemented in this pr?

@smiklos smiklos marked this pull request as ready for review August 2, 2023 15:03
@tustvold tustvold merged commit 0eb9049 into apache:master Aug 2, 2023
26 checks passed
@tustvold
Copy link
Contributor

tustvold commented Aug 2, 2023

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty lists in FixedSizeListArray::try_new is not handled
2 participants