Skip to content

Parquet: Expose accessors from ArrowReaderOptions #7400

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

Conversation

kylebarron
Copy link
Contributor

Which issue does this PR close?

Ref #7342 (comment)

We can create a standalone issue if needed.

Closes #.

Rationale for this change

I was just updating some of my code to use the v55 release candidate, but it doesn't seem possible for third party code to use the options argument in AsyncFileReader::get_metadata because no internal data from ArrowReaderOptions is publicly accessible.

The internal implementation here on AsyncRead and AsyncSeek accesses the page_index and file_decryption_properties attributes:

.with_page_indexes(options.is_some_and(|o| o.page_index));

options.and_then(|o| o.file_decryption_properties.as_ref()),

But third party code can't do that.

Exposing accessors on ArrowReaderOptions allows third party implementations of AsyncFileReader::get_metadata to use the values set on ArrowReaderOptions.

What changes are included in this PR?

Expose attributes from ArrowReaderOptions.

Are there any user-facing changes?

New methods.

@kylebarron kylebarron changed the title Expose accessors from ArrowReaderOptions Parquet: Expose accessors from ArrowReaderOptions Apr 9, 2025
Copy link
Contributor

@etseidl etseidl left a comment

Choose a reason for hiding this comment

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

LGTM. Adding accessors should not be a breaking change, right?

@jayzhan211 jayzhan211 merged commit 959499b into apache:main Apr 10, 2025
16 of 17 checks passed
@jayzhan211
Copy link
Contributor

Thanks @kylebarron @etseidl @mbrobbel

@Xuanwo
Copy link
Member

Xuanwo commented May 6, 2025

Thank you @kylebarron for this. I just encountered the same issue and am very happy to see it has already been fixed.

CC @alamb: Would it be possible to release a patch that includes this change? This issue is breaking our existing Parquet file reads, as we can't obtain the correct ArrowReaderOptions inside AsyncFileReader.

@alamb
Copy link
Contributor

alamb commented May 6, 2025

Thank you @kylebarron for this. I just encountered the same issue and am very happy to see it has already been fixed.

CC @alamb: Would it be possible to release a patch that includes this change? This issue is breaking our existing Parquet file reads, as we can't obtain the correct ArrowReaderOptions inside AsyncFileReader.

We have a release scheduled for later this month:

Would you like to make one sooner? If so we could release a 55.0.1 release potentially, or accelerate the timeline for 55.1.0?

@Xuanwo
Copy link
Member

Xuanwo commented May 7, 2025

Would you like to make one sooner?

Yeah, I wish we can have this commit sooner so that we can catch up the next release of iceberg-rust. That's will be very appreciated.

If so we could release a 55.0.1 release potentially, or accelerate the timeline for 55.1.0?

Both looks good to me. I didn't check the change list yet. Maybe it's better to cherry pick this commit only? I will leave this decision to you.

I'm willing to take tasks needed for this accelerated release.

@alamb
Copy link
Contributor

alamb commented May 8, 2025

Would you like to make one sooner?

Yeah, I wish we can have this commit sooner so that we can catch up the next release of iceberg-rust. That's will be very appreciated.

If so we could release a 55.0.1 release potentially, or accelerate the timeline for 55.1.0?

Both looks good to me. I didn't check the change list yet. Maybe it's better to cherry pick this commit only? I will leave this decision to you.

I think we have flexibility with release timelines, so I think we should just accelerate the release of 55.1.0. I explained this on #7393 (comment)

I'm willing to take tasks needed for this accelerated release.

Thank you -- since this will just be a normal release I don't think any extra tasks are required.

I'll try and get the RC ready for voting today or tomorrow and thus a release out early next week

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

Successfully merging this pull request may close these issues.

6 participants