-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Introduce wildcard const for FixedSizeBinary type signature #17531
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
base: main
Are you sure you want to change the base?
Conversation
/// valid length. It exists to avoid the need to enumerate all possible fixed size binary lengths. | ||
/// | ||
/// The value is offset by 1 to make it distinct from [`FIXED_SIZE_LIST_WILDCARD`]. | ||
pub const FIXED_SIZE_BINARY_WILDCARD: i32 = i32::MIN + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case someone puts FIXED_SIZE_LIST_WILDCARD
when they meant FIXED_SIZE_BINARY_WILDCARD
, as otherwise they would be interchangeable
/// This is used where a function can accept a fixed size binary type with any | ||
/// valid length. It exists to avoid the need to enumerate all possible fixed size binary lengths. | ||
/// | ||
/// The value is offset by 1 to make it distinct from [`FIXED_SIZE_LIST_WILDCARD`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't have to be different?
@@ -913,6 +916,10 @@ fn coerced_from<'a>( | |||
Some(_) => Some(FixedSizeList(Arc::clone(f_into), *size_from)), | |||
_ => None, | |||
}, | |||
// should be able to coerce wildcard fixed size binary to non wildcard fixed size binary | |||
(FixedSizeBinary(FIXED_SIZE_BINARY_WILDCARD), FixedSizeBinary(size_from)) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically speaking, FixedSizeBinary(-2147483647)
is not a valid type, so it's not meaningful to define a coercion between it and any other type. I can see how it may be useful in case of function signatures though. What we actually want is generic signatures in functions. i.e. a signature that is not a type itself, but matches a type.
Binary, | ||
BinaryView, | ||
LargeBinary, | ||
FixedSizeBinary(FIXED_SIZE_BINARY_WILDCARD), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than use this invalid type hack, i'd recommend implementing this for now as a "user defined signature"
long term solution is to have Signature::uniform
accept generic types (type patterns).
Do we have any tracking issues for these atm? |
I realized we may get away without introducing a generic type concept. cc @jayzhan211 |
Let me take a look 👍 |
Which issue does this PR close?
Rationale for this change
Currently not able to easily represent arbitrary sized
FixedSizeBinary
s in type signature API.What changes are included in this PR?
Introduce new
FIXED_SIZE_BINARY_WILDCARD
const that allows specifying type signatures which accept arbitrarily sized fixed size binary, similar toFIXED_SIZE_LIST_WILDCARD
. Also modifybitmap_count
signature to use this new signature.Are these changes tested?
Are there any user-facing changes?