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

Box FlightErrror::tonic to reduce size (fixes nightly clippy) #7229

Merged
merged 4 commits into from
Mar 7, 2025

Conversation

XiangpengHao
Copy link
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

When working on the nightly rust, I noticed that clippy warns about FlightError too large, more than 176 bytes.

I think it makes sense to keep the error small (32 bytes), as the Result<...> is used in a lot of places.

This PR basically makes Tonic(tonic::Status) into Tonic(Box<tonic::Status>), which reduces the FlightError from 176 bytes down to 32 bytes.

This adds an extra allocation on error, but should probably be fine as it's not on hot path.

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels Mar 3, 2025
@tustvold tustvold added the api-change Changes to the arrow API label Mar 3, 2025
@crepererum
Copy link
Contributor

which reduces the FlightError from 176 bytes down to 32 bytes.

can you add a test that checks that, e.g.

#[test]
fn test_error_size() {
    assert_eq!(std::mem::size_of::<FlightError>(), 32);
}

@alamb alamb added the next-major-release the PR has API changes and it waiting on the next major version label Mar 6, 2025
@alamb
Copy link
Contributor

alamb commented Mar 6, 2025

can you add a test that checks that, e.g.

I took the liberty of adding this in 94da215 and pushed to this branch

@alamb alamb changed the title Make flight tonic error boxed Box FlightErrror::tonic to reduce size (fixes nightly clippy) Mar 6, 2025
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.

@crepererum crepererum merged commit 9e91029 into apache:main Mar 7, 2025
13 checks passed
@tustvold
Copy link
Contributor

I think this has been merged by mistake? Is main open for breaking changes yet?

@crepererum
Copy link
Contributor

crepererum commented Mar 11, 2025

I think this has been merged by mistake? Is main open for breaking changes yet?

I forgot that, TBH I find it tiring to manually check the release schedule every time.

Edit: filed #7268 to prevent future annoyance.

crepererum added a commit that referenced this pull request Mar 11, 2025
tustvold pushed a commit that referenced this pull request Mar 11, 2025
@tustvold tustvold added the development-process Related to development process of arrow-rs label Mar 11, 2025
alamb added a commit to alamb/arrow-rs that referenced this pull request Mar 12, 2025
alamb added a commit to alamb/arrow-rs that referenced this pull request Mar 12, 2025
@alamb
Copy link
Contributor

alamb commented Mar 12, 2025

Update:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate development-process Related to development process of arrow-rs next-major-release the PR has API changes and it waiting on the next major version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants