jayzhan211 commented on code in PR #9920:
URL: https://github.com/apache/arrow-datafusion/pull/9920#discussion_r1549603208
##########
datafusion/expr/src/function.rs:
##########
@@ -67,6 +88,24 @@ impl<'a> AccumulatorArgs<'a> {
sort_exprs,
}
}
+
+ /// Return a not yet implemented error if IGNORE NULLs is true
+ pub fn check_ignore_nulls(&self, name: &str) -> Result<()> {
+ if self.ignore_nulls {
Review Comment:
But I think there is also a problem because if they forget to add
`check_ignore_nulls/check_order_by` in the accumulator, they can still run the
function successfully. This approach does not *force* the user to check their
options because datafusion implements them, not the user.
To *enforce* they specified the options for their functions, I think we can
implement options directly in `AggregateUDFImpl`.
Either
```rust
// We will check if the AccArgs meet the requirement or not.
fn options() -> AccumulatorArgs {
AccumulatorArgs {
ignore_nulls: true/ false,
has_ordering: true / false
}
}
```
or
```rust
// The same with this one, but separate each option
fn support_ignore_nulls() -> bool
fn support_ordering() -> bool
```
##########
datafusion/expr/src/function.rs:
##########
@@ -67,6 +88,24 @@ impl<'a> AccumulatorArgs<'a> {
sort_exprs,
}
}
+
+ /// Return a not yet implemented error if IGNORE NULLs is true
+ pub fn check_ignore_nulls(&self, name: &str) -> Result<()> {
+ if self.ignore_nulls {
Review Comment:
But I think there is also a problem because if they forget to add
`check_ignore_nulls/check_order_by` in the accumulator, they can still run the
function successfully. This approach does not *force* the user to check their
options because datafusion implements them, not the user.
To *enforce* they specified the options for their functions, I think we can
implement options directly in `AggregateUDFImpl`.
Either
```rust
// We will check if the AccumulatorArgs meet the requirement or not.
fn options() -> AccumulatorArgs {
AccumulatorArgs {
ignore_nulls: true/ false,
has_ordering: true / false
}
}
```
or
```rust
// The same with this one, but separate each option
fn support_ignore_nulls() -> bool
fn support_ordering() -> bool
```
--
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]
