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: mark (Large)ListView as nested and support in equal data type #6995

Merged
merged 1 commit into from
Jan 19, 2025

Conversation

rluvaton
Copy link
Contributor

Which issue does this PR close?

N/A

Rationale for this change

ListView and LargeListView are nested and should have correct equal data type impl

What changes are included in this PR?

  1. Add to is_nested in the datatype.rs file to return true for ListView and LargeListView
  2. Add to equals_datatype in the datatype.rs file to skip checking file field name and metadata for ListView and LargeListView
  3. Add to contains in the datatype.rs file to use the same logic for ListView and LargeListView that is used for the List data type

Are there any user-facing changes?

Kind of.

and this is considered breaking change if looking at it from behavior perspective, but I don't think it should be marked as one

@github-actions github-actions bot added the arrow Changes to the arrow crate label Jan 19, 2025
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't we should use wildcard pattern in this file for matching data type

Copy link
Contributor

Choose a reason for hiding this comment

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

Given how rarely this enumeration changes, I'm inclined to optimise for legibility and continue to use wildcard matches

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

I agree this is a bug fix and not a breaking change, especially given ListView isn't really supported anywhere at the moment

@tustvold tustvold merged commit 764b34a into apache:main Jan 19, 2025
26 checks passed
totoroyyb pushed a commit to totoroyyb/arrow-rs that referenced this pull request Jan 20, 2025
@alamb alamb added the enhancement Any new improvement worthy of a entry in the changelog label Jan 27, 2025
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 enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants