alamb commented on code in PR #17515:
URL: https://github.com/apache/datafusion/pull/17515#discussion_r2343788618
##########
datafusion/spark/src/function/url/parse_url.rs:
##########
@@ -222,7 +223,7 @@ fn spark_parse_url(args: &[ArrayRef]) -> Result<ArrayRef> {
)
}
(DataType::Utf8View, DataType::Utf8View, DataType::Utf8View) => {
- process_parse_url::<_, _, _, StringArray>(
+ process_parse_url::<_, _, _, StringViewArray>(
Review Comment:
nice
##########
datafusion/expr/src/udf.rs:
##########
@@ -233,7 +233,25 @@ impl ScalarUDF {
///
/// See [`ScalarUDFImpl::invoke_with_args`] for details.
pub fn invoke_with_args(&self, args: ScalarFunctionArgs) ->
Result<ColumnarValue> {
- self.inner.invoke_with_args(args)
+ #[cfg(debug_assertions)]
Review Comment:
I played around with this code a little bit and I found it could be made
more concise with less indenting. Not needed, just for your consideration
```rust
/// Invoke the function on `args`, returning the appropriate result.
///
/// See [`ScalarUDFImpl::invoke_with_args`] for details.
pub fn invoke_with_args(&self, args: ScalarFunctionArgs) ->
Result<ColumnarValue> {
#[cfg(debug_assertions)]
let return_field = Arc::clone(&args.return_field);
let value = self.inner.invoke_with_args(args)?;
// Maybe this could be enabled always?
// This doesn't use debug_assert!, but it's meant to run anywhere
except on production, so it's same in spirit, so conditioning on
debug_assertions.
#[cfg(debug_assertions)]
if &value.data_type() != return_field.data_type() {
return datafusion_common::exec_err!("Function '{}' returned
value of type '{:?}' while the following type was promised at planning time and
expected: '{:?}'",
self.name(),
value.data_type(),
return_field.data_type()
);
}
// TODO verify return data is non-null when it was promised to be?
Ok(value)
}
```
##########
datafusion/expr/src/udf.rs:
##########
@@ -233,7 +233,25 @@ impl ScalarUDF {
///
/// See [`ScalarUDFImpl::invoke_with_args`] for details.
pub fn invoke_with_args(&self, args: ScalarFunctionArgs) ->
Result<ColumnarValue> {
- self.inner.invoke_with_args(args)
+ #[cfg(debug_assertions)]
+ let return_field = Arc::clone(&args.return_field);
+ match self.inner.invoke_with_args(args) {
+ Ok(value) => {
+ // Maybe this could be enabled always?
+ // This doesn't use debug_assert!, but it's meant to run
anywhere except on production, so it's same in spirit, so conditioning on
debug_assertions.
+ #[cfg(debug_assertions)]
+ if &value.data_type() != return_field.data_type() {
+ return datafusion_common::exec_err!("Function '{}'
returned value of type '{:?}' while the following type was promised at planning
time and expected: '{:?}'",
Review Comment:
It probably doesn't matter, but I would suggeset making this `internal_err`
instead of `exec_err` as it signifies a bug rather than some other potentially
expected runtime error
```suggestion
return datafusion_common::internal_err!("Function '{}'
returned value of type '{:?}' while the following type was promised at planning
time and expected: '{:?}'",
```
--
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
