goldmedal commented on code in PR #12259:
URL: https://github.com/apache/datafusion/pull/12259#discussion_r1739635149
##########
datafusion/functions/src/core/getfield.rs:
##########
@@ -184,9 +184,29 @@ impl ScalarUDFImpl for GetFieldFunc {
};
match (array.data_type(), name) {
- (DataType::Map(_, _), ScalarValue::Utf8(Some(k))) => {
+ (DataType::Map(_, _), name) => {
let map_array = as_map_array(array.as_ref())?;
- let key_scalar:
Scalar<arrow::array::GenericByteArray<arrow::datatypes::GenericStringType<i32>>>
= Scalar::new(StringArray::from(vec![k.clone()]));
+ if !matches!(name, ScalarValue::Utf8(_) |
ScalarValue::Int64(_) | ScalarValue::Float64(_)) {
+ return exec_err!(
+ "get indexed field is only possible on map with utf8, int64
and float64 indexes. \
+ Tried with {name:?} index")
+ }
+ let key_array: ArrayRef = match name {
+ ScalarValue::Int64(Some(k)) => {
+ Arc::new(Int64Array::from(vec![*k]))
+ }
+ ScalarValue::Utf8(Some(k)) => {
+ Arc::new(StringArray::from(vec![k.clone()]))
+ }
+ ScalarValue::Float64(Some(k)) => {
+ Arc::new(Float64Array::from(vec![*k]))
+ }
+ _ => {
+ unreachable!();
Review Comment:
Instead of `unreachable` panic, I prefer to throw the Datafuion error
`internal_err` like
```
return internal_err!("Unexpected scalar value type: {name}");
```
Although I think L189 has protected it, if any change breaks this
protection, we can find it easily.
##########
datafusion/functions/src/core/mod.rs:
##########
@@ -84,6 +84,12 @@ pub mod expr_fn {
pub fn get_field(arg1: Expr, arg2: impl Literal) -> Expr {
super::get_field().call(vec![arg1, arg2.lit()])
}
+
+ /// Returns the value of the field with the given name from the struct.
+ /// **Internal use only.** This function is added to support the map use
case.
+ pub fn _get_field(arg1: Expr, arg2: Expr) -> Expr {
+ super::get_field().call(vec![arg1, arg2])
+ }
Review Comment:
I'm not sure if it's a common naming habit to add an `_` prefix for internal
functions in Rust 🤔 . How about naming it get_field_from_expr? I think it would
be more clear. We can also use it for the Literal case.
```rust
#[doc = "Returns the value of the field with the given name from the
struct"]
pub fn get_field(arg1: Expr, arg2: impl Literal) -> Expr {
get_field_from_expr(arg1, arg2.lit())
}
/// Returns the value of the field with the given name from the struct.
/// This function is added to support the map use case.
pub fn get_field_from_expr(arg1: Expr, arg2: Expr) -> Expr {
super::get_field().call(vec![arg1, arg2])
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
