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

Adding compact check functionality to GenericByteViewArray #5720

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ClSlaid
Copy link
Contributor

@ClSlaid ClSlaid commented May 4, 2024

This part contains compact check functionality for GenericByteViewArray.

Which issue does this PR close?

Rationale for this change

Part of #5707.

What changes are included in this PR?

  • Enables GenericByteViewArray to check if its representation is compact.

Are there any user-facing changes?

None.

this part contains compact check functionality for GenericByteViewArray.

Signed-off-by: 蔡略 <[email protected]>
@github-actions github-actions bot added the arrow Changes to the arrow crate label May 4, 2024
Signed-off-by: 蔡略 <[email protected]>
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.

Thanks for this PR @ClSlaid

Would it be possible please to make a PR with ::gc() as described on #5720? first?

I think the logic to check for compacted is a nice (but new) additional API

This PR doesn't add any new APIs to ByteViewArray -- if we want to consider this code, perhaps we could add one like `GenericByteViewArray::is_compacted()?

But again i consider this a separate task

arrow-array/src/array/byte_view_array.rs Outdated Show resolved Hide resolved
@ClSlaid ClSlaid changed the title Adding GC functionality to GenericByteViewArray (Part 1/2) Adding compact check functionality to GenericByteViewArray May 5, 2024
@ClSlaid
Copy link
Contributor Author

ClSlaid commented May 5, 2024

Would it be possible please to make a PR with ::gc() as described on #5720? first?

I'd implement this first, instead of simultaneously or later.

So that I will not bother resolving git conflict 🪢, or have to implement a wasteful full-copying GC algorithm first. :P

This PR doesn't add any new APIs to ByteViewArray -- if we want to consider this code, perhaps we could add one like `GenericByteViewArray::is_compacted()?

As you wish, a new is_compacted :: &Self -> bool method is added.

But again i consider this a separate task

Exactly.

@ClSlaid
Copy link
Contributor Author

ClSlaid commented May 13, 2024

@alamb Would you kindly retrigger the CI? I don't see anything wrong with my code.
Merge this PR and then we can push forward implementing GC.

@alamb
Copy link
Contributor

alamb commented May 13, 2024

@alamb Would you kindly retrigger the CI? I don't see anything wrong with my code. Merge this PR and then we can push forward implementing GC.

I believe the CI errors are due to issues that are subsequently fixed (@tustvold fixed #5719 this morning)

Thus if you merge up from the master branch you should be able to get a clean CI run

@alamb
Copy link
Contributor

alamb commented May 13, 2024

I am very excited to see GenericByteViewArray::gc 🙏

I still don't fully understand the need for the compact check API in this PR. Perhaps we can make a PR for gc first and then revisit this?

@ClSlaid ClSlaid marked this pull request as draft May 14, 2024 08:20
@ClSlaid
Copy link
Contributor Author

ClSlaid commented May 14, 2024

will rewrite after GC merged

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.

2 participants