You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Rachelint (via GitHub)" <gi...@apache.org> on 2024/08/05 18:30:42 UTC
[PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Rachelint opened a new pull request, #11827:
URL: https://github.com/apache/datafusion/pull/11827
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #11819
## Rationale for this change
See #11819
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
Impl `convert_to_state` for `GroupsAccumulatorAdapter`.
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
Working.
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
No.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter` (faster median for high cardinality aggregates) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#discussion_r1759390202
##########
datafusion/functions-aggregate-common/src/aggregate/groups_accumulator.rs:
##########
@@ -348,6 +352,46 @@ impl GroupsAccumulator for GroupsAccumulatorAdapter {
fn size(&self) -> usize {
self.allocation_bytes
}
+
+ fn convert_to_state(
+ &self,
+ values: &[ArrayRef],
+ opt_filter: Option<&BooleanArray>,
+ ) -> Result<Vec<ArrayRef>> {
+ let num_rows = values[0].len();
+
+ // Each row has its respective group
+ let mut results = vec![];
+ for row_idx in 0..num_rows {
+ // Create the empty accumulator for converting
+ let mut converted_accumulator = (self.factory)()?;
Review Comment:
as a follow on PR I wonder if we could potentially to improve performance by adding a `clear()` or `reset()` type function to each accumulator to avoid having to create a new accumulator for each 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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#discussion_r1709282866
##########
datafusion/physical-expr/src/aggregate/groups_accumulator/adapter.rs:
##########
@@ -342,6 +374,50 @@ impl GroupsAccumulator for GroupsAccumulatorAdapter {
fn size(&self) -> usize {
self.allocation_bytes
}
+
+ fn convert_to_state(
Review Comment:
Yeah, it takes some getting used to thinking in terms of `Array`s and masks, etc
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter` (faster median for high cardinality aggregates) [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on code in PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#discussion_r1759647832
##########
datafusion/functions-aggregate-common/src/aggregate/groups_accumulator.rs:
##########
@@ -348,6 +352,46 @@ impl GroupsAccumulator for GroupsAccumulatorAdapter {
fn size(&self) -> usize {
self.allocation_bytes
}
+
+ fn convert_to_state(
+ &self,
+ values: &[ArrayRef],
+ opt_filter: Option<&BooleanArray>,
+ ) -> Result<Vec<ArrayRef>> {
+ let num_rows = values[0].len();
+
+ // Each row has its respective group
+ let mut results = vec![];
+ for row_idx in 0..num_rows {
+ // Create the empty accumulator for converting
+ let mut converted_accumulator = (self.factory)()?;
Review Comment:
Yes, I want to reuse the `converted_accumulator` at the beginning, but it is not ensure that the state will be reset after calling `state`.
It is clever to add such function to do the reset work explicitly.
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter` (faster median for high cardinality aggregates) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #11827:
URL: https://github.com/apache/datafusion/pull/11827
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on code in PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#discussion_r1704792650
##########
datafusion/physical-expr/src/aggregate/groups_accumulator/adapter.rs:
##########
@@ -342,6 +374,50 @@ impl GroupsAccumulator for GroupsAccumulatorAdapter {
fn size(&self) -> usize {
self.allocation_bytes
}
+
+ fn convert_to_state(
Review Comment:
> thanks @Rachelint
>
> This might be interesting to you: #11825
Seems interesting, learning this pr (still not so familiar with arrow ops).
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#issuecomment-2344159428
> > I created a proposal in #12438
> > I tested a little locally on the PR here and it seems like this PR does not improve the performance much.
>
> Ok, I will check it in my local soon.
It may be that the query doesn't show the correct pattern of high cardinality intermediate aggregates, btw. I am not sure
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#issuecomment-2344061391
> Thank you @Rachelint -- this looks very cool . I am sorry for the delay in the review
>
> It is my understanding that this will allow aggregates that do not yet implement GroupsAccumulator to benefit from the intermediate aggregate state.
>
> Thus the primary benefit of this code is to make aggregates on such queries faster.
>
> Unfortunately I don't think we have any examples of such aggregates in the benchmarks (e.g. calculating median or approx_median). I will make a PR to add some to see if we can measure improvement of this PR
>
> cc @korowa
Sounds great! And we can continue to improve the performance after having such benchmarks.
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#issuecomment-2344108637
I created a proposal in https://github.com/apache/datafusion/pull/12438
I tested a little locally on the PR here and it seems like this PR does not improve the performance much.
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#discussion_r1704538507
##########
datafusion/physical-expr/src/aggregate/groups_accumulator/adapter.rs:
##########
@@ -342,6 +374,50 @@ impl GroupsAccumulator for GroupsAccumulatorAdapter {
fn size(&self) -> usize {
self.allocation_bytes
}
+
+ fn convert_to_state(
Review Comment:
thanks @Rachelint
This might be interesting to you: https://github.com/apache/datafusion/pull/11825
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#issuecomment-2350011912
I just ran this with some new queries on https://github.com/apache/datafusion/pull/12438 and this branch goes about 2x faster
* Elapsed 9.777 seconds. --> Elapsed 5.361 seconds.
* Elapsed 6.919 seconds. --> Elapsed 3.952 seconds.
```shell
./datafusion-cli-intermediate-state -f tq1.sql
DataFusion CLI v41.0.0
0 row(s) fetched.
Elapsed 0.019 seconds.
+-------------+---------------------+---+------+------+------+
| ClientIP | WatchID | c | tmin | tp95 | tmax |
+-------------+---------------------+---+------+------+------+
| 1611957945 | 6655575552203051303 | 2 | 0 | 0 | 0 |
| -1402644643 | 8566928176839891583 | 2 | 0 | 0 | 0 |
+-------------+---------------------+---+------+------+------+
2 row(s) fetched.
Elapsed 5.361 seconds.
+-------------+---------------------+---+------+------+------+
| ClientIP | WatchID | c | tmin | tmed | tmax |
+-------------+---------------------+---+------+------+------+
| 1611957945 | 6655575552203051303 | 2 | 0 | 0 | 0 |
| -1402644643 | 8566928176839891583 | 2 | 0 | 0 | 0 |
+-------------+---------------------+---+------+------+------+
2 row(s) fetched.
Elapsed 3.952 seconds.
andrewlamb@Andrews-MacBook-Pro-2 Downloads % datafusion-cli -f tq1.sql
datafusion-cli -f tq1.sql
DataFusion CLI v41.0.0
0 row(s) fetched.
Elapsed 0.020 seconds.
+-------------+---------------------+---+------+------+------+
| ClientIP | WatchID | c | tmin | tp95 | tmax |
+-------------+---------------------+---+------+------+------+
| 1611957945 | 6655575552203051303 | 2 | 0 | 0 | 0 |
| -1402644643 | 8566928176839891583 | 2 | 0 | 0 | 0 |
+-------------+---------------------+---+------+------+------+
2 row(s) fetched.
Elapsed 9.777 seconds.
+-------------+---------------------+---+------+------+------+
| ClientIP | WatchID | c | tmin | tmed | tmax |
+-------------+---------------------+---+------+------+------+
| 1611957945 | 6655575552203051303 | 2 | 0 | 0 | 0 |
| -1402644643 | 8566928176839891583 | 2 | 0 | 0 | 0 |
+-------------+---------------------+---+------+------+------+
2 row(s) fetched.
Elapsed 6.919 seconds.
```
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter` (faster median for high cardinality aggregates) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#issuecomment-2351560576
The 42.0.0 release has been cut -- let's start the code flowing!
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#issuecomment-2346894196
> I am trying to find the reason.
It is probably because there are 17M groups which each is holding multiple aggregates each with non trivial state 🤔
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on code in PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#discussion_r1745201830
##########
datafusion/physical-expr/src/aggregate/groups_accumulator/adapter.rs:
##########
@@ -342,6 +374,50 @@ impl GroupsAccumulator for GroupsAccumulatorAdapter {
fn size(&self) -> usize {
self.allocation_bytes
}
+
+ fn convert_to_state(
Review Comment:
Found the quick way using `filtered_null_mask` + `set_nulls` will just set the filtered row to be null, but not change the row num.
```rust
// `filtered_null_mask` + `set_nulls`
left: PrimitiveArray<Int32>
[
null,
null,
6,
null,
null,
null,
]
// `compute::filter`
right: PrimitiveArray<Int32>
[
null,
6,
]
```
Maybe this will make difference to the correctness for some accumulators? For example, an udf count which thinks a null row as 1?
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#issuecomment-2346985605
> > I am trying to find the reason.
>
> It is probably because there are 17M groups which each is holding multiple aggregates each with non trivial state 🤔
Yes... I found state is much larger than the simple accumulators(e.g. `count`, `sum`, `average`).
For example, the digest for `approx_percentile_cont`:
```
pub struct TDigest {
centroids: Vec<Centroid>,
max_size: usize,
sum: f64,
count: u64,
max: f64,
min: f64,
}
```
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#issuecomment-2347046502
I create a subset of `hits_partitioned`, and using this smaller dataset, it can success to run the q32 like high cardinality query now.
```sql
SELECT "WatchID", "ClientIP", COUNT(*) AS c, approx_median("ResponseStartTiming") tmed FROM hits GROUP BY "WatchID", "ClientIP" ORDER BY c DESC LIMIT 10;
```
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on code in PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#discussion_r1745201830
##########
datafusion/physical-expr/src/aggregate/groups_accumulator/adapter.rs:
##########
@@ -342,6 +374,50 @@ impl GroupsAccumulator for GroupsAccumulatorAdapter {
fn size(&self) -> usize {
self.allocation_bytes
}
+
+ fn convert_to_state(
Review Comment:
Found the quick way using `filtered_null_mask` + `set_nulls` will just set the filtered row to be null, but now change the row num.
```rust
// `filtered_null_mask` + `set_nulls`
left: PrimitiveArray<Int32>
[
null,
null,
6,
null,
null,
null,
]
// `compute::filter`
right: PrimitiveArray<Int32>
[
null,
6,
]
```
Maybe this will make difference to the correctness for some accumulators? For example, an udf count which thinks a null row as 1?
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on code in PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#discussion_r1744783828
##########
datafusion/physical-expr/src/aggregate/groups_accumulator/adapter.rs:
##########
@@ -342,6 +374,50 @@ impl GroupsAccumulator for GroupsAccumulatorAdapter {
fn size(&self) -> usize {
self.allocation_bytes
}
+
+ fn convert_to_state(
Review Comment:
Sorry for delay, fixed.
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#issuecomment-2347092811
This is the number from my local test with a subset(15%) of `hits_partitioned`:
```
// Main(3ece7a736193)
Q5: SELECT "WatchID", "ClientIP", COUNT(*) AS c, approx_median("ResponseStartTiming") tmed FROM hits GROUP BY "WatchID", "ClientIP" ORDER BY c DESC LIMIT 10;
Query 5 iteration 0 took 17567.8 ms and returned 10 rows
Query 5 iteration 1 took 17462.8 ms and returned 10 rows
Query 5 iteration 2 took 17442.4 ms and returned 10 rows
Query 5 iteration 3 took 17569.0 ms and returned 10 rows
Query 5 iteration 4 took 17527.8 ms and returned 10 rows
// This pr's branch(have rebased 3ece7a736193)
Q5: SELECT "WatchID", "ClientIP", COUNT(*) AS c, approx_median("ResponseStartTiming") tmed FROM hits GROUP BY "WatchID", "ClientIP" ORDER BY c DESC LIMIT 10;
Query 5 iteration 0 took 12093.5 ms and returned 10 rows
Query 5 iteration 1 took 12234.2 ms and returned 10 rows
Query 5 iteration 2 took 11951.3 ms and returned 10 rows
Query 5 iteration 3 took 12066.6 ms and returned 10 rows
Query 5 iteration 4 took 11963.1 ms and returned 10 rows
```
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter` (faster median for high cardinality aggregates) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#issuecomment-2351560623
Thanks again @Rachelint
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on code in PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#discussion_r1745201830
##########
datafusion/physical-expr/src/aggregate/groups_accumulator/adapter.rs:
##########
@@ -342,6 +374,50 @@ impl GroupsAccumulator for GroupsAccumulatorAdapter {
fn size(&self) -> usize {
self.allocation_bytes
}
+
+ fn convert_to_state(
Review Comment:
Found the quick way using `filtered_null_mask` + `set_nulls` will just set the filtered row to be null, but now change the row num.
```rust
// `filtered_null_mask` + `set_nulls`
left: PrimitiveArray<Int32>
[
null,
null,
6,
null,
null,
null,
]
// `compute::filter`
right: PrimitiveArray<Int32>
[
null,
6,
]
```
Maybe this will make difference to the correctness for some accumulators? For example, an udf count which will thinks a null row as 1?
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#issuecomment-2346885589
I found I can' t run it successfully in my local...
The memory usage get higher and higher, and swap seems to be triggered, and make it even more slower... Finally, it almost can't finish forever...
I am trying to find the reason.
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#issuecomment-2344127128
> I created a proposal in #12438
>
> I tested a little locally on the PR here and it seems like this PR does not improve the performance much.
Ok, I will check it in my local soon.
--
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: [PR] Impl `convert_to_state` for `GroupsAccumulatorAdapter`. [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on PR #11827:
URL: https://github.com/apache/datafusion/pull/11827#issuecomment-2344127134
> I created a proposal in #12438
>
> I tested a little locally on the PR here and it seems like this PR does not improve the performance much.
Ok, I will check it in my local soon.
--
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