alamb commented on code in PR #14167:
URL: https://github.com/apache/datafusion/pull/14167#discussion_r1920205796
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -2849,6 +2849,50 @@ impl ScalarValue {
ScalarValue::from(value).cast_to(target_type)
}
+ /// Returns the Some(`&str`) representation of `ScalarValue` of logical
string type
Review Comment:
this is the new function
##########
datafusion/functions-aggregate/src/string_agg.rs:
##########
@@ -108,15 +108,14 @@ impl AggregateUDFImpl for StringAgg {
fn accumulator(&self, acc_args: AccumulatorArgs) -> Result<Box<dyn
Accumulator>> {
if let Some(lit) =
acc_args.exprs[1].as_any().downcast_ref::<Literal>() {
- return match lit.value() {
- ScalarValue::Utf8(Some(delimiter))
- | ScalarValue::LargeUtf8(Some(delimiter)) => {
- Ok(Box::new(StringAggAccumulator::new(delimiter.as_str())))
+ return match lit.value().try_as_str() {
Review 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
##########
datafusion/functions/src/datetime/common.rs:
##########
@@ -270,10 +268,8 @@ where
}
},
// if the first argument is a scalar utf8 all arguments are expected
to be scalar utf8
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Utf8View(a)
- | ScalarValue::LargeUtf8(a)
- | ScalarValue::Utf8(a) => {
+ ColumnarValue::Scalar(scalar) => match scalar.try_as_str() {
+ Some(a) => {
Review Comment:
I think this is clearer now
##########
datafusion/functions/src/crypto/basic.rs:
##########
@@ -121,11 +121,9 @@ pub fn digest(args: &[ColumnarValue]) ->
Result<ColumnarValue> {
);
}
let digest_algorithm = match &args[1] {
- ColumnarValue::Scalar(scalar) => match scalar {
- ScalarValue::Utf8View(Some(method))
- | 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() {
Review 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()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
