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

Further seal the OffsetSizeTrait #1823

Closed
wants to merge 3 commits into from

Conversation

HaoYang670
Copy link
Contributor

Signed-off-by: remzi 13716567376yh@gmail.com

This is a follow-up PR of #1819.

Rationale for this change

OffsetSizeTrait should only be implemented for i32 and i64

What changes are included in this PR?

  1. seal the OffsetSizeTrait
  2. update docs

Are there any user-facing changes?

Yes.

verify

impl OffsetSizeTrait for u32 {
    const IS_LARGE: bool = false;
}

gives compile error

error[E0277]: the trait bound `u32: array_list::private::Sealed` is not satisfied
  --> arrow/src/array/array_list.rs:49:6
   |
49 | impl OffsetSizeTrait for u32 {
   |      ^^^^^^^^^^^^^^^ the trait `array_list::private::Sealed` is not implemented for `u32`
   |
   = help: the following implementations were found:
             <i32 as array_list::private::Sealed>
             <i64 as array_list::private::Sealed>
note: required by a bound in `array_list::OffsetSizeTrait`
  --> arrow/src/array/array_list.rs:36:55
   |
35 | pub trait OffsetSizeTrait:
   |           --------------- required by a bound in this
36 |     ArrowNativeType + std::ops::AddAssign + Integer + private::Sealed
   |                                                       ^^^^^^^^^^^^^^^ required by this bound in `array_list::OffsetSizeTrait`

, which is good!

Signed-off-by: remzi <13716567376yh@gmail.com>
@github-actions github-actions bot added the arrow Changes to the arrow crate label Jun 9, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2022

Codecov Report

Merging #1823 (93677f9) into master (ba38ebe) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1823      +/-   ##
==========================================
- Coverage   83.44%   83.44%   -0.01%     
==========================================
  Files         199      199              
  Lines       56651    56651              
==========================================
- Hits        47272    47270       -2     
- Misses       9379     9381       +2     
Impacted Files Coverage Δ
arrow/src/array/array_list.rs 96.16% <ø> (ø)
parquet_derive/src/parquet_field.rs 65.53% <0.00%> (-0.46%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba38ebe...93677f9. Read the comment docs.

Signed-off-by: remzi <13716567376yh@gmail.com>
@github-actions github-actions bot added the arrow-flight Changes to the arrow-flight crate label Jun 9, 2022
Signed-off-by: remzi <13716567376yh@gmail.com>
@tustvold
Copy link
Contributor

tustvold commented Jun 9, 2022

I don't follow why this is necessary, the orphan rules prevent defining OffsetSize for foreign types outside of arrow-rs. As ArrowNativeType is sealed, by definition there can be no non-foreign types that implement it?

To put it another way, impl OffsetSizeTrait for u32 is illegal outside of arrow-rs.

Further, the rationale for sealing is avoiding transmute funky. There are no such semantics attached to OffsetSizeTrait, only indirectly via its relationship with ArrowNativeType. Even if you could impl OffsetSizeTrait for u32, it wouldn't be potentially unsound.

@HaoYang670
Copy link
Contributor Author

Thank you @tustvold for your explanation, which makes perfect sense to me.
This is my first time learning Rust’s orphan rules. 😁

@HaoYang670
Copy link
Contributor Author

I will close this PR as OffsetSizeTrait has been sealed.
Thank you @tustvold for your review!

@HaoYang670 HaoYang670 closed this Jun 9, 2022
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 arrow-flight Changes to the arrow-flight crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants