jkosh44 commented on code in PR #14532:
URL: https://github.com/apache/datafusion/pull/14532#discussion_r1952912279
##########
datafusion/expr-common/src/signature.rs:
##########
@@ -286,6 +258,72 @@ impl Display for ArrayFunctionSignature {
}
}
+/// A wrapper around a vec of [`ArrayFunctionArgument`], to ensure that the
vec has at least one
+/// array element.
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
+pub struct ArrayFunctionArguments {
+ /// A full list of the arguments accepted by a function.
+ arguments: Vec<ArrayFunctionArgument>,
+}
+
+impl ArrayFunctionArguments {
+ /// Returns an error if there are no [`ArrayFunctionArgument::Array`]
arguments.
+ pub fn new(arguments: Vec<ArrayFunctionArgument>) -> Result<Self, &'static
str> {
+ if !arguments
+ .iter()
+ .any(|arg| *arg == ArrayFunctionArgument::Array)
Review Comment:
That's actually what I originally had. The benefit of the current approach
is that the error happens during startup and is very clear. Otherwise the error
doesn't happen until the function is first executed and the error is not as
informative. Additionally, it forces people to to validate the invariant that
`ArrayFunctionSignature::Array` contains at least one array when initializing
the struct because they're forced to call `unwrap()` or `expect()`.
On startup:
```
DataFusion CLI v45.0.0
thread 'main' panicked at datafusion/expr-common/src/signature.rs:695:22:
contains array: "missing array argument"
stack backtrace:
0: rust_begin_unwind
at
/rustc/e71f9a9a98b0faf423844bf0ba7438f29dc27d58/library/std/src/panicking.rs:665:5
1: core::panicking::panic_fmt
at
/rustc/e71f9a9a98b0faf423844bf0ba7438f29dc27d58/library/core/src/panicking.rs:76:14
2: core::result::unwrap_failed
at
/rustc/e71f9a9a98b0faf423844bf0ba7438f29dc27d58/library/core/src/result.rs:1699:5
3: core::result::Result<T,E>::expect
at
/home/joe/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/result.rs:1061:23
4: datafusion_expr_common::signature::Signature::array_and_index
at
/home/joe/Projects/datafusion/datafusion/expr-common/src/signature.rs:691:32
5: datafusion_functions_nested::extract::ArrayElement::new
at
/home/joe/Projects/datafusion/datafusion/functions-nested/src/extract.rs:121:24
6:
datafusion_functions_nested::extract::array_element_udf::INSTANCE::{{closure}}
at
/home/joe/Projects/datafusion/datafusion/functions-nested/src/macros.rs:95:29
7: core::ops::function::FnOnce::call_once
at
/home/joe/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
8: core::ops::function::FnOnce::call_once
at
/home/joe/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
9: std::sync::lazy_lock::LazyLock<T,F>::force::{{closure}}
at
/home/joe/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sync/lazy_lock.rs:212:25
10: std::sync::once::Once::call_once::{{closure}}
at
/home/joe/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sync/once.rs:158:41
11: std::sys::sync::once::futex::Once::call
at
/rustc/e71f9a9a98b0faf423844bf0ba7438f29dc27d58/library/std/src/sys/sync/once/futex.rs:176:21
12: std::sync::once::Once::call_once
at
/home/joe/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sync/once.rs:158:9
13: std::sync::lazy_lock::LazyLock<T,F>::force
at
/home/joe/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sync/lazy_lock.rs:208:9
14: <std::sync::lazy_lock::LazyLock<T,F> as core::ops::deref::Deref>::deref
at
/home/joe/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sync/lazy_lock.rs:311:9
15: datafusion_functions_nested::extract::array_element_udf
at
/home/joe/Projects/datafusion/datafusion/functions-nested/src/macros.rs:98:39
16: datafusion_functions_nested::all_default_nested_functions
at
/home/joe/Projects/datafusion/datafusion/functions-nested/src/lib.rs:129:9
17:
datafusion::execution::session_state_defaults::SessionStateDefaults::default_scalar_functions
at
/home/joe/Projects/datafusion/datafusion/core/src/execution/session_state_defaults.rs:108:31
18:
datafusion::execution::session_state::SessionStateBuilder::with_default_features
at
/home/joe/Projects/datafusion/datafusion/core/src/execution/session_state.rs:1088:36
19: datafusion::execution::context::SessionContext::new_with_config_rt
at
/home/joe/Projects/datafusion/datafusion/core/src/execution/context/mod.rs:330:21
20: datafusion_cli::main_inner::{{closure}}
at ./src/main.rs:173:15
21: datafusion_cli::main::{{closure}}
at ./src/main.rs:131:34
22: <core::pin::Pin<P> as core::future::future::Future>::poll
at
/home/joe/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/future/future.rs:124:9
23: tokio::runtime::park::CachedParkThread::block_on::{{closure}}
at
/home/joe/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.43.0/src/runtime/park.rs:284:63
24: tokio::runtime::coop::with_budget
at
/home/joe/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.43.0/src/runtime/coop.rs:107:5
25: tokio::runtime::coop::budget
at
/home/joe/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.43.0/src/runtime/coop.rs:73:5
26: tokio::runtime::park::CachedParkThread::block_on
at
/home/joe/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.43.0/src/runtime/park.rs:284:31
27: tokio::runtime::context::blocking::BlockingRegionGuard::block_on
at
/home/joe/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.43.0/src/runtime/context/blocking.rs:66:9
28:
tokio::runtime::scheduler::multi_thread::MultiThread::block_on::{{closure}}
at
/home/joe/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.43.0/src/runtime/scheduler/multi_thread/mod.rs:87:13
29: tokio::runtime::context::runtime::enter_runtime
at
/home/joe/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.43.0/src/runtime/context/runtime.rs:65:16
30: tokio::runtime::scheduler::multi_thread::MultiThread::block_on
at
/home/joe/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.43.0/src/runtime/scheduler/multi_thread/mod.rs:86:9
31: tokio::runtime::runtime::Runtime::block_on_inner
at
/home/joe/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.43.0/src/runtime/runtime.rs:370:45
32: tokio::runtime::runtime::Runtime::block_on
at
/home/joe/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.43.0/src/runtime/runtime.rs:340:13
33: datafusion_cli::main
at ./src/main.rs:131:5
34: core::ops::function::FnOnce::call_once
at
/home/joe/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose
backtrace.
Process finished with exit code 101
```
During function execution:
```
> SELECT array_element(1);
Error during planning: Internal error: Function 'array_element' expected at
least one argument array argument.
This was likely caused by a bug in DataFusion's code and we would welcome
that you file an bug report in our issue tracker No function matches the given
name and argument types 'array_element(Int64)'. You might need to add explicit
type casts.
Candidate functions:
array_element(index)
```
I've removed `ArrayFunctionArguments`, but still wanted to explain it's
rationale in case it wasn't obvious.
--
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]
