[Variant] Introduce new type over &str for ShortString #7718
Conversation
friendlymatthew
force-pushed
the
string-vs-short-string
branch
2 times, most recently
from
043f082 to
a06720b
Compare
friendlymatthew
force-pushed
the
string-vs-short-string
branch
from
fea5ecd to
df42f00
Compare
There was a problem hiding this comment.
This looks great to me -- thank you @friendlymatthew . I left a few comments but we can do that as follow on PRs if you prefer
Note this is very likely to conflict with
friendlymatthew
force-pushed
the
string-vs-short-string
branch
from
df42f00 to
a6ad4e5
Compare
|
FYI @scovich perhaps you could offer your opinion on this PR as well |
friendlymatthew
force-pushed
the
string-vs-short-string
branch
from
e713fd4 to
2a62499
Compare
parquet-variant/src/variant.rs
Outdated
| /// | ||
| /// This constructor verifies that `value` is shorter than or equal to `MAX_SHORT_STRING_SIZE` | ||
| pub fn try_new(value: &'a str) -> Result<Self, ArrowError> { | ||
| if value.len() > MAX_SHORT_STRING_SIZE { |
There was a problem hiding this comment.
Worth adding a comment here that we are indeed supposed to check bytes and not characters, that's a common confusion with "string length"
There was a problem hiding this comment.
That is a great idea -- maybe we can even name the constant MAX_SHORT_STRING_BYTES to make it more self describing
friendlymatthew
force-pushed
the
string-vs-short-string
branch
from
dae45e6 to
09af8ff
Compare
| impl<'a> From<ShortString<'a>> for &'a str { | ||
| fn from(value: ShortString<'a>) -> Self { |
There was a problem hiding this comment.
This seems a bit... unorthodox? Would impl Deref be more traditional, to go with that impl AsRef?
There was a problem hiding this comment.
That makes a lot of sense
|
Thanks again @friendlymatthew and @scovich and @adriangb |
Which issue does this PR close?
This commit introduces
ShortString, a newtype that wraps around&strthat enforces a maximum length constraint. This also allows us to perform validation once and removes a superfluous validation check inappend_value.The now-superflous validation check was needed since users could construct
Variant::ShortStrings directly, without doing input validation. This means you can have a short string variant which actually contains a string that is no longer than 63 bytes.But since we enforce this check upon construction, we can directly match against
Variant::StringandVariant::ShortStringarms with their respective appending functions (append_stringandappend_short_string).