Add ScalarValue::try_as_str to get str value from logical strings#14167
Add ScalarValue::try_as_str to get str value from logical strings#14167alamb merged 1 commit intoapache:mainfrom
ScalarValue::try_as_str to get str value from logical strings#14167Conversation
github-actions
bot
added
physical-expr
| ScalarValue::from(value).cast_to(target_type) | ||
| } | ||
|
|
||
| /// Returns the Some(`&str`) representation of `ScalarValue` of logical string type |
There was a problem hiding this comment.
this is the new function
| ScalarValue::Utf8(Some(delimiter)) | ||
| | ScalarValue::LargeUtf8(Some(delimiter)) => { | ||
| Ok(Box::new(StringAggAccumulator::new(delimiter.as_str()))) | ||
| return match lit.value().try_as_str() { |
There was a problem hiding this comment.
This is a pretty good example of can reducing the repetition in the code to check for string literal values. This also now implicitly will work for Dictionary values where it would not have before
| | ScalarValue::Utf8(Some(method)) | ||
| | ScalarValue::LargeUtf8(Some(method)) => method.parse::<DigestAlgorithm>(), | ||
| other => exec_err!("Unsupported data type {other:?} for function digest"), | ||
| ColumnarValue::Scalar(scalar) => match scalar.try_as_str() { |
There was a problem hiding this comment.
We could also avoid a bunch more duplication of stuff like this:
let part = if let ColumnarValue::Scalar(ScalarValue::Utf8(Some(v))) = part {
If we added a similar convenience method ColumnarValue::try_as_scalar_str() that returned a Option<Option<&str>>
Similarly we could do the same with Expr::try_as_scalar_str()
| | ScalarValue::LargeUtf8(a) | ||
| | ScalarValue::Utf8(a) => { | ||
| ColumnarValue::Scalar(scalar) => match scalar.try_as_str() { | ||
| Some(a) => { |
There was a problem hiding this comment.
I think this is clearer now
| ScalarValue::from(value).cast_to(target_type) | ||
| } | ||
|
|
||
| /// Returns the Some(`&str`) representation of `ScalarValue` of logical string type |
| /// let scalar = ScalarValue::from("hello"); | ||
| /// assert_eq!(scalar.try_as_str().flatten(), Some("hello")); | ||
| /// ``` | ||
| pub fn try_as_str(&self) -> Option<Option<&str>> { |
There was a problem hiding this comment.
Why not -> Result<Option<&str>> for this try method?
Caller can always convert to an option.
(Also, most of the use cases in this PR are converting a returned None to an error).
There was a problem hiding this comment.
Thank you for the review @wiedld
DataFusionError always has an owned String in it, so returning an Result is actually quite slow as it needs to allocate some memory and copy stuff around. Thus I think this API should return an Option
|
Thanks @wiedld and @xudong963 ! |
3 participants
Which issue does this PR close?
ScalarValues as&str#14166Rationale for this change
See #14166
TLDR is I don't want to have to remember to check all the variants of ScalarValue that can contain a string
What changes are included in this PR?
ScalarValue::try_as_strto get str value from logical stringsAre these changes tested?
yes, by doc tests and examples
Are there any user-facing changes?
there is a new API but all existing APIs still work