You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2025/04/10 17:43:12 UTC
[I] Possible regression in aggregate query correctness [datafusion]
andygrove opened a new issue, #15676:
URL: https://github.com/apache/datafusion/issues/15676
### Describe the bug
In the Comet subproject, we have a PR where we are upgrading to use the latest DataFusion revision from main in preparation for upgrading to DataFusion 47 (https://github.com/apache/datafusion-comet/pull/1563).
After bumping the revision from `784df33f8930f91eada0d67aa5acc25a4c25cea2` to `b22e4d26480202247022bf2c94ba629a07e3275b`, we see some tests start to fail, such as:
`multiple group-by columns + single aggregate column, with nulls`
```
2025-04-10T17:13:49.0990050Z == Results ==
2025-04-10T17:13:49.0990580Z !== Correct Answer - 2 == == Spark Answer - 2 ==
2025-04-10T17:13:49.0991090Z struct<_g1:tinyint,_g2:smallint,last(_3):int> struct<_g1:tinyint,_g2:smallint,last(_3):int>
2025-04-10T17:13:49.0991750Z [null,null,null] [null,null,null]
2025-04-10T17:13:49.0992100Z ![0,0,9] [0,0,0] (QueryTest.scala:244)
```
It is also possible that the error is due to other changes in Comet and not related to DataFusion. I will update this issue once I know more.
### To Reproduce
_No response_
### Expected behavior
_No response_
### Additional context
_No response_
--
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-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "UBarney (via GitHub)" <gi...@apache.org>.
UBarney commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2796526975
> I think the undeterministic is based on the input not order-preserved, so if the input is order-preserved, we can have deterministic result for first/last.
Just to clarify, what does "order-preserved" mean exactly? Does it only refer to a single partition with ordered input, or can the order be determined by ORDER BY fields?
> If ordering is specified, it returns the first value among those "last values" with the same ordering keys, due to the performance reason
I don't understand why we *should* "return the first value among those "last values" with the same ordering keys". If the ordering keys are the same, the result may still be non-deterministic unless the input is ordered and in a single partition.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "UBarney (via GitHub)" <gi...@apache.org>.
UBarney commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2795784149
> > I implemented it according to the behavior defined in this document
> > > Returns the last element in an aggregation group according to the requested ordering. **If no ordering is given, returns an arbitrary** element from the group.
> >
> >
> > https://datafusion.apache.org/user-guide/sql/aggregate_functions.html#last-value
>
> The doc isn't correct, we should return the last value in the group
As an aside, I'm not sure if the return value of `last_value` (as implemented previously) is stable if no ordering is specified, even if the input is in order.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2795016049
> I wonder, why do we even support LAST_VALUE / FIRST_VALUE as plain aggregate functions? I think hey don't make any sense as aggregate functions (but only as window functions).
They are valid in Spark as plain aggregates. I'm not sure if any other query engines support this use.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2797612697
> Rewrite our tests for LAST to stop comparing to Spark and implement some other means to determine that the behavior is correct, and also document that Comet is not compatible with Spark in some cases
Perhaps we can add order by for the tests?
> Fork the LAST implementation and maintain it in Comet
What is the way for forking? Any downside like performance to make it deterministic? If we can do this in the forked version without downside, we can also do the same change in DataFusion itself so that we can keep the previous behavior?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Possible regression in aggregate query correctness [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2794874385
The following query previously produced different values for `FIRST` and `LAST` but now produces the same value for both.
```sql
SELECT _g1, _g2, FIRST(_3), LAST(_3) FROM v GROUP BY _g1, _g2 ORDER BY _g1, _g2
```
Before:
```
[0,0,0,9]
```
After
```
[0,0,0,0]
```
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2795612502
> I implemented it according to the behavior defined in this document
>
> > Returns the last element in an aggregation group according to the requested ordering. **If no ordering is given, returns an arbitrary** element from the group.
>
> https://datafusion.apache.org/user-guide/sql/aggregate_functions.html#last-value
The doc isn't correct, we should return the last value in the group
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2797833569
> Perhaps we can add order by for the tests?
Spark SQL doesn't seem to support an `ORDER BY` clause in this context.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2796011842
> Is it possible to maintain order in group case? We read the batch in order so the ordering is maintained.
https://github.com/apache/datafusion/pull/14232 reminds me that when we have multiple partitions the order might not be preserved, therefore we can't always guarantee the deterministic result unless there is no ordering + single partition. The most simple case.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "UBarney (via GitHub)" <gi...@apache.org>.
UBarney commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2796030831
> can try to get the deterministic result in non-order + single partition case
That may be slow since there's no parallelism (only one thread/coroutine does the grouping).
If we want a deterministic result, I think we should add `ORDER BY`. Just like when we want deterministic output with `LIMIT`, we need to add `ORDER BY`.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2797840426
I'm ok with closing this issue since the behavior of first/last aggregates without explicit ordering is not generally deterministic. We will figure out an approach in Comet. Thanks for all of the feedback and discussion.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "UBarney (via GitHub)" <gi...@apache.org>.
UBarney commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2795786634
https://github.com/apache/datafusion/pull/15681 may fix this regression
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2796042803
> Is it possible to maintain order in group case
The point I was trying to make is `FIRST` and `LAST` are not deterministic, so users can not trust on the behavior to be consistent in future versions.
[Spark](https://spark.apache.org/docs/latest/api/python/reference/pyspark.sql/api/pyspark.sql.functions.first.html)) says for example "The function is non-deterministic because its results depends on the order of the rows which may be non-deterministic after a shuffle" )
The aggregation function doesn't give **any promise at all** you'll be getting really the first and last values of something. E.g. for `SELECT FIRST(x) GROUP BY t` there is just nothing in the query specifying that `t` should be ordered in a way or should maintain order.
Based on how `t` is scanned (it might read it sequentially from parquet or in some order from a database...`) and the order in which the values are grouped and tasks are being scheduled, nothing prevents it from returning the rows in a different order..
Based on the semantics of the function, the engine could convert it to a function like `ANY_VALUE` and don't break any contract.
IMO we should at least clearly document this behaviour and/or maybe either deprecate these functions (users can include said function into contrib themselves) or move them to a contrib crate.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2796351960
I think the undeterministic is based on the input not order-preserved, so if the input is order-preserved, we can have deterministic result for first/last.
> Returns the last element in an aggregation group according to the requested ordering. If no ordering is given, returns an arbitrary element from the group.
LastValue:
Returns the last element in an aggregation group (if any), based on the specified ordering.
- If the input is not order-preserved, the result may be non-deterministic.
- If no ordering is specified, the function returns the last value in the group.
- If ordering is specified, it returns the first value among those with the same ordering keys, due to the performance reason, See https://github.com/apache/datafusion/pull/14232
FirstValue:
Returns the first element in an aggregation group (if any), based on the specified ordering.
- If the input is not order-preserved, the result may be non-deterministic.
- If no ordering is specified, the function returns the first value in the group.
- If ordering is specified, it returns the first value.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2797856807
Sorry to have caused so much discussion.
I'm totally in favor of keeping this open and have the function (without `order by`) for now matching the expectation of "first value based on what is provided to the function" or "last value based on what is provided to the function" while acknowledging it might change in the future based on implementation details.
We can have a discussion for future direction elsewhere.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Possible regression in aggregate query correctness [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2794808743
The issue starts happening with https://github.com/apache/datafusion/pull/15542, which seems to have changed the behavior of the `last_value` aggregate. I am still investigating.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove closed issue #15676: Regression in `last_value` functionality
URL: https://github.com/apache/datafusion/issues/15676
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "UBarney (via GitHub)" <gi...@apache.org>.
UBarney commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2796150236
> FIRST_VALUE and LAST_VALUE aggregation functions are not deterministic
If we include an `ORDER BY` clause, and the column used for ordering contains no duplicate values, then the results of `FIRST_VALUE` and `LAST_VALUE` can be considered deterministic.
https://github.com/apache/datafusion/blob/221df48da43eca71a65d7baeb43ae82a166862e9/datafusion/sqllogictest/test_files/group_by.slt#L2687-L2700
> IMO we should at least clearly document this behaviour
I believe the current documentation already makes this clear—these functions are not deterministic when no `ORDER BY` is specified:
> Returns the last element in an aggregation group according to the requested ordering. If no ordering is given, returns an *arbitrary* element from the group.
https://datafusion.apache.org/user-guide/sql/aggregate_functions.html#last-value
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2794913290
@UBarney fyi
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2795627706
```rust
fn should_update_state(
&self,
group_idx: usize,
new_ordering_values: &[ScalarValue],
) -> Result<bool> {
if !self.is_sets.get_bit(group_idx) {
return Ok(true);
}
assert!(new_ordering_values.len() == self.ordering_req.len());
let current_ordering = &self.orderings[group_idx];
compare_rows(current_ordering, new_ordering_values, &self.sort_options).map(|x| {
if self.pick_first_in_group {
x.is_gt()
} else {
x.is_lt()
}
})
}
```
For last value, even the ordering is empty we still need to update the value.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2796363319
I believe the same issue applies to `nth_value` and `array_agg` order-sensitive aggregation
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2794950825
I wonder, why do we even support LAST_VALUE / FIRST_VALUE as plain aggregate functions?
I think hey don't make any sense as aggregate functions.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2794967384
(But I think we should of course fix this regression).
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "UBarney (via GitHub)" <gi...@apache.org>.
UBarney commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2795568566
I implemented it according to the behavior defined in this document
> Returns the last element in an aggregation group according to the requested ordering. **If no ordering is given, returns an arbitrary** element from the group.
https://datafusion.apache.org/user-guide/sql/aggregate_functions.html#last-value
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2796056565
> If we want a deterministic result, I think we should add ORDER BY. Just like when we want deterministic output with LIMIT, we need to add ORDER BY.
Yes, but in that case it would become equivalent to a window function `FIRST_VALUE`, which is clearly defined (and has a sane execution path in DataFusion). [`SELECT FIRST_VALUE(x) OVER (PARTITION BY x ORDER BY )` ](https://datafusion.apache.org/user-guide/sql/window_functions.html#first-value)
Sorry for hijacking this issue @andygrove for this discussion but I think it's important to not have features like these limit the progression of DataFusion in the future.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2796540520
> IMO we should at least clearly document this behaviour and/or maybe either deprecate these functions (users can include said function into contrib themselves) or move them to a contrib crate.
That's a very aggressive action, IMO
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2796621637
> Just to clarify, what does "order-preserved" mean exactly? Does it only refer to a single partition with ordered input, or can the order be determined by ORDER BY fields?
The input of first/last is the same as the data in data source.
> I don't understand why we should "return the first value among those "last values" with the same ordering keys". If the ordering keys are the same, the result may still be non-deterministic unless the input is ordered and in a single partition.
This case is single partition (order-preserved) + ordering.
To be more specifically, whether the output is deterministic depends on the sort algorithm, if we have stable sort where the order is preserved for same keys, then we have deterministic output.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2796920220
Sorry to cause so much work for everyone discussing this 😞
For order-preserved input, such as single-partition & single-thread, DataFusion has implemented the same behavior as Spark for the past ~1 year. In the past week, the behavior changed. It isn't necessarily "wrong," but it is a breaking change for some downstream users. Comet's test suites run the same query against Spark and Comet/DataFusion and compare the results. The tests are mostly deterministic. Upgrading to the soon-to-be DataFusion 47 causes test failures, hence I reported this issue.
In the Comet project, we now have the following choices:
- 1. Rewrite our tests for `LAST` to stop comparing to Spark and implement some other means to determine that the behavior is correct, and also document that Comet is not compatible with Spark in some cases
- 2. Fork the `LAST` implementation and maintain it in Comet
- 3. See if there are options for DataFusion to support the order-preserved case
Any one of these options can work.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2795994676
> when given no ordering that will make results deterministic will be not deterministic (depending on how te function is executed)
Is it possible to maintain order in group case? We read the batch in order so the ordering is maintained.
If there is some case that we can't maintain ordering then I think it makes sense to have a non-deterministic output.
I only now that when the order is given, we might not get the "true" last value. See https://github.com/apache/datafusion/pull/14232
when we have `last(a order by b)` and `b` are all the same, we don't return the "true" last value.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2794909458
I confirmed that the behavior in DataFusion also changed when #15542 was merged.
## Before
```
query III
SELECT y, FIRST_VALUE(x), LAST_VALUE(x)
FROM FOO
GROUP BY y
ORDER BY y;
----
2 1 1
3 2 1
```
## After
```
query III
SELECT y, FIRST_VALUE(x), LAST_VALUE(x)
FROM FOO
GROUP BY y
ORDER BY y;
----
2 1 1
3 2 2
```
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2796224178
Thanks @UBarney, nice to see it is already documented.
So to give another example, test with queries like the following:
https://github.com/apache/datafusion-comet/blob/fdaec64fd313954ae788413ec02cf652cfa570b9/spark/src/test/scala/org/apache/comet/exec/CometAggregateSuite.scala#L1052
```
SELECT FIRST(col1), LAST(col1) FROM t
SELECT FIRST(col1), LAST(col1), col3 FROM t GROUP BY col3
```
**Will** break in the future whenever we change some internals that changes the order of batches / rows to appear in an aggregation function (not only depending on the order of which t is scanned.
So a query `SELECT FIRST(col1), LAST(col1) FROM t`
Can basically be seen as semantically equivalent to queries like the following queries (just giving some extreme examples to make a point):
```
SELECT ANY_VALUE(col1), ANY_VALUE(col1) FROM t
SELECT MIN(col1), MAX(col1) FROM t
SELECT FIRST(col1), LAST(col1) FROM t ORDER BY RANDOM()
SELECT FIRST(col1), LAST(col1) FROM t LIMIT 1
```
> Returns the last element in an aggregation group according to the requested ordering. If no ordering is given, returns an arbitrary element from the group.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2796537078
I think these should work in the most performant way if no order specified. We are not required to provide a deterministic result, and we can change the result comparison to a version which compares the results against the inclusion of the values in the ARRAY_AGG() of the same column)
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2795614284
> I wonder, why do we even support LAST_VALUE / FIRST_VALUE as plain aggregate functions? I think hey don't make any sense as aggregate functions (but only as window functions).
DuckDb has `first` `last` too, I guess it would be useful in some cases? 🤔
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Regression in `last_value` functionality [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on issue #15676:
URL: https://github.com/apache/datafusion/issues/15676#issuecomment-2795881552
> > I wonder, why do we even support LAST_VALUE / FIRST_VALUE as plain aggregate functions? I think hey don't make any sense as aggregate functions (but only as window functions).
>
> DuckDb has `first` `last` too, I guess it would be useful in some cases? 🤔
The reaction I have is because the result of `first_value` and `last_value` when given no ordering that will make results deterministic will be not deterministic (depending on how te function is executed). If DataFusion would switch to another mode of execution or grouping implementation that doesn't maintain same order, functions like these would break (based on what users might expect from them).
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org