Seal ArrowPrimitiveType #3882
Merged
Seal ArrowPrimitiveType #3882
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tustvold
commented
Mar 17, 2023
| @@ -230,27 +230,7 @@ pub type Decimal128Array = PrimitiveArray<Decimal128Type>; | |||
| /// scale less or equal to 76. | |||
| pub type Decimal256Array = PrimitiveArray<Decimal256Type>; | |||
|
|
|||
| /// Trait bridging the dynamic-typed nature of Arrow (via [`DataType`]) with the | |||
| /// static-typed nature of rust types ([`ArrowNativeType`]) for all types that implement [`ArrowNativeType`]. | |||
| pub trait ArrowPrimitiveType: 'static { | |||
There was a problem hiding this comment.
I moved this into types along with all the other traits of this sort
tustvold
commented
Mar 17, 2023
| mod run { | ||
| use super::*; | ||
|
|
||
| pub trait RunEndTypeSealed {} |
There was a problem hiding this comment.
This is no longer necessary, as PrimitiveType is sealed
alamb
approved these changes
Mar 17, 2023
|
Fully agree with making this sealed 👍 |
viirya
reviewed
Mar 17, 2023
arrow-array/src/types.rs
Outdated
|
|
||
| impl RunEndTypeSealed for Int64Type {} | ||
| } | ||
|
|
||
| /// A subtype of primitive type that is used as run-ends index | ||
| /// in `RunArray`. | ||
| /// See <https://arrow.apache.org/docs/format/Columnar.html> | ||
| /// | ||
| /// Note: The implementation of this trait is sealed to avoid accidental misuse. |
There was a problem hiding this comment.
Now a redundant comment line:
Suggested change
| /// Note: The implementation of this trait is sealed to avoid accidental misuse. |
viirya
approved these changes
Mar 17, 2023
|
Making sense. 👍 |
spebern
pushed a commit
to spebern/arrow-rs
that referenced
this pull request
Mar 25, 2023
* Seal ArrowPrimitiveType * Fix doc * Review feedback
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #.
Rationale for this change
As ArrowNativeType is sealed, it is hard to see how or why anyone would implement this for a custom type, but sealing it makes this contract very clear.
What changes are included in this PR?
Are there any user-facing changes?
Technically this is a breaking change, practically I sincerely doubt anyone will notice