You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/07/10 18:06:41 UTC
[GitHub] [arrow-datafusion] alamb opened a new issue, #6906: Implement fast min/max accumulator for strings (now it uses the slower path)
alamb opened a new issue, #6906:
URL: https://github.com/apache/arrow-datafusion/issues/6906
### Is your feature request related to a problem or challenge?
https://github.com/apache/arrow-datafusion/pull/6904 introduces some fancy new hashing and ways to implement aggregates
min/max for strings (`StringArray` / `LargeStringArray`, etc) now uses the slower `Accumulator` implementation which could be made much faster
### Describe the solution you'd like
I would like to implement a fast `GroupsAccumulator` for Min/Max
### Describe alternatives you've considered
here is one potential way to implement it:
We could store the current minimum for all groups in the same Rows π€ and track an index into that Rows for the current minimum for each group.
This would require an extra copy of the input values, but it could probably be vectorized pretty well, as shown in the following diagram.
Sorry what I meant was something like the following where the accumulator only stored the current minimum values.
This approach would potentially end up with `min_storage` being full of "garbage" if many batches had new minumums, but I think we could heuristically "compact" `min_storage` (if it had `2*num_groups`, for example) if it got too large
```
β β β β β β β β β β β β β β β β β β β
β Accumulator β
state
βββββββββββ βββββββββββ β βββββββββββ βββββββββββ β
β βββββββ β β βββββββ β β βββββββ β β βββββββ β
β β A β β β β A βββΌβββββ β β β D β β βββββββΌββ 1 β β β
β βββββββ€ β β βββββββ€ β β β βββββββ€ β β β βββββββ€ β
β β B β β β β B β β ββββΌββΌβΆβ A βββΌβββββ β β 0 β β β
β βββββββ€ β β βββββββ€ β β βββββββ β β βββββββ β
β β A β β β β A β β β β β β β β
β βββββββ€ β β βββββββ€ β β β β β
β β A β β β β A β β β β β β β β
β βββββββ€ β β βββββββ€ β β β β β
β β C β β β β C β β β β β β β β
β βββββββ β β βββββββ β β β β β
βββββββββββ βββββββββββ β βββββββββββ βββββββββββ β
input input β min_storage: min_values β
values values Rows
(Array) (Rows) β β β β β β β β β β β β β β β β β β β
step 1: step 2: for
convert any value step 3: min value
arguments to that is a new (per group) is
Row format group tracked as an
minimum, copy index into
it to a min_storage `Rows`
second `Rows`
```
See https://github.com/apache/arrow-datafusion/pull/6800#issuecomment-1622290981 for more details
### 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@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2368855897
> I think what @alamb means is that just simply using Vec<String> to store the states will be at least not worse than StringArray + GroupsAccumulatorAdapter, and it is easy to start from.
Yes indeed. Thank you
> I guess the current goal is to remove the calling of slice, and get an at least not worse performance than StringArray + GroupsAccumulatorAdapter as mentioned above.
π―
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2366854617
> @Rachelint your implicit idea of using `Vec<String>` to store the state I think is actually quite interesting and maybe we should try that one first:
>
>
>
> It would at least avoid calling `Array::slice` and likely be better than using `GroupsAccumulatorAdapter`, even if we could improve it later with more explciit memory management π€
>
>
So this would effectively be using a Vec<String> for state instead of your proposal of using something akin to StringView above @alamb?
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2368191429
> > @Rachelint your implicit idea of using `Vec<String>` to store the state I think is actually quite interesting and maybe we should try that one first:
> > It would at least avoid calling `Array::slice` and likely be better than using `GroupsAccumulatorAdapter`, even if we could improve it later with more explciit memory management π€
>
> So this would effectively be using a `Vec<String>` for state instead of your proposal of using something akin to StringView notated here [#6906 (comment)](https://github.com/apache/datafusion/issues/6906#issuecomment-2355629355) @alamb?
I think what @alamb means is that just simply using `Vec<String>` to store the store will be at least not worse than `StringArray + GroupsAccumulatorAdapter`, and it is easy to start from.
And after improve it to be not worse than `StringArray + GroupsAccumulatorAdapter`, we can continue to push forward enabling string view by default.
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2356207431
The challenge of `String` seems that?
- If we just simply use a `Vec<String>` like `primitives` to keep the min/max values, it is too expensive to convert them to `StringArray`/`StringViewArray`(many many copy)
- But if we use `StringArray` like approach to keep the values, we can't update the min/max values.
- So Finally we need to use a `StringViewArray` like approach to make it, but still have the new challenge about gc?
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2355629355
# Potential Design
One high level idea is to build a data structure that uses the same internal format (views/buffers) as [`StringViewArray`](https://docs.rs/arrow/latest/arrow/array/struct.GenericByteViewArray.html) in Arrow:
```
βββββββββββββββββββββββββββββββββββββββββββββ
β Stored in Vec<u8> β
β Stored in a βββββββββββββββββββ β
β Vec<u128> β βββββββββββββ β β
β βββββββββββββββ β βββΆβsome value β β β
βββββββ β β βββββββββββ β β β βββββββββββββ β β
β 0 ββββββββββΌββΌβΆβ View β β β β β
βββββββ€ β β βββββββββββ€ β β β β β
β 1 ββββββββββΌββΌβΆβ View ββββ β β β ... β β
βββββββ β β βββββββββββ β β β β β
... β β ... β β β β β
βββββββ β β βββββββββββ β β β β β
β N-2 β β β β View ββββ β β β βββββββββββββββ β
βββββββ€ β β βββββββββββ€ β β βΌ ββΆβother value ββ β
β N-1 ββββββββββΌββΌβΆβ View β β β βββββββββββββββ β
βββββββ β β βββββββββββ β βββββββββββββββββββ β
β βββββββββββββββ String values are stored β
β inline or in extra byte β
Logical group β buffer β
number βββββββββββββββββββββββββββββββββββββββββββββ
New structure: MutableStringViewBuilder
Current Min/Max value for that group stored in
same format as StringViewArray
```
In this design, the current value for each group is stored in two parts (as described on [arrow docs](https://docs.rs/arrow/latest/arrow/array/struct.GenericByteViewArray.html))
1. a fixed size `u128` view
2. a variable length part with the string data
As new batches are updated, each `View` is updated if necessary
Benefits of design:
1. Hopefully use the same code as in arrow-rs
4. Allows Zero copy conversion to StringView / BinaryView at output
5. Use inlined values for quick min/max comparison
I believe (though we will have to verify it) that the conversion from MutableStringViewBuilder to just StringArray (not StringViewArray) should also be better than the current `GroupsAccumulatorAdapter`. Both conversions need to copy the string bytes again into the packed `StringArray` format, but the `GroupsAccumulatorAdapter` also has to allocate/free owned `String`s as well
## Potential challenges
I think the trickiest part of this code, other the low level code optimizations is that as min/max values are replaced, data in the variable length buffer will become "garbage" (not reachable) thus consuming more memory than necessary:
```
Stored in Vec<u8>
ββββββββββββββββββββββ
β ββββββββββββββββββ β
βββββββββββββββ β βprev max value 1β β
β βββββββββββ β β ββββββββββββββββββ β
β β View ββββ β β ... β
β βββββββββββ β β ββββββββββββββββββ β
β ... β β β βprev max value mβ β
β β β ββββββββββββββββββ β
β β β β ββββββββββββββββββ β
β β β βββΆβprev max value mβ β
β β β ββββββββββββββββββ β
β β β ... β
β β ββββββββββββββββββββββ
β β
βββββββββββββββ Previous min/max values
are not pointed to
anymore and need to be
cleaned up
```
I think this means the code will need something [`GenericByteViewArray::gc`](https://docs.rs/arrow/latest/arrow/array/struct.GenericByteViewArray.html#method.gc) run occasionally
## Random Thoughts
Thoughts: maybe this structure (MutableStringViewBuilder??) could be upstreamed eventually
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2365379616
> > > while working on #12092 it turns out that Min / Max on BinaryView and StringView are suuuuper slow.
> >
> >
> > Fixed in #12575, now min/max on `StringViewArray` should be slightly faster than `StringArray`
>
> I mean, fixed for that particular clickbench query which does not leverage stats to prune parquet access.
Cool, I'm assuming this change didn't impact the `GroupsAccumulatorAdapter` call in anyway since I'm still seeing a similar performance + flamegraph with the following query:
```
set datafusion.execution.parquet.schema_force_view_types = true;
SELECT REGEXP_REPLACE("Referer", '^https?://(?:www\\.)?([^/]+)/.*$', '\\1') AS k, AVG(length("Referer")) AS l, COUNT(*) AS c, MIN("Referer")
FROM hits_partitioned
WHERE "Referer" <> '' GROUP BY k HAVING COUNT(*) > 100000 ORDER BY l DESC LIMIT 25;
```
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2366846595
> BTW I was thinking more about this issue -- while a native Min/Max for strings / stringview will help, I have an idea that might make it simply it an added bonus: pushing the cast down into the parquet reader https://github.com/apache/datafusion/issues/12509#issuecomment-2366775581
This would only work for parquet correct? It would effectively be apart of the TableScan within the logical plan?
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2309105124
@alamb is there anyone working on this + is this issue still relevant? I would love to tackle it as it seems like an interesting feature/optimization.
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2356331232
@Rachelint your implicit idea of using `Vec<String>` to store the state I think is actually quite interesting and maybe we should try that one first:
It would at least avoid calling `Array::slice` and likely be better than using `GroupsAccumulatorAdapter`, even if we could improve it later with more explciit memory management π€
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2355638858
> @alamb is there anyone working on this + is this issue still relevant? I would love to tackle it as it seems like an interesting feature/optimization.
@devanbenz I think this would be a fun and interesting project as well as valuable to DataFusion. However, I also think it is pretty advanced -- I would enjoy helping with it, but also maybe @Rachelint or @jayzhan211 are interested in helping out π€
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2355935498
BTW in case anyone is interested, I recorded a short video on how to make these flamegraphs: https://youtu.be/2z11xtYw_xs
I will add a link to that in the docs later
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "XiangpengHao (via GitHub)" <gi...@apache.org>.
XiangpengHao commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2365332711
> > while working on #12092 it turns out that Min / Max on BinaryView and StringView are suuuuper slow.
>
> Fixed in #12575, now min/max on `StringViewArray` should be slightly faster than `StringArray`
I mean, fixed for that particular clickbench query which does not leverage stats to prune parquet access.
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6906:
URL: https://github.com/apache/arrow-datafusion/issues/6906#issuecomment-1880882560
One observation here is that `min` and `max` on strings is not that common of an operation from what it seems -- grouping on strings is more common.
Maybe there is some binary usecase where it is important (e.g. embeddings π€ )
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "XiangpengHao (via GitHub)" <gi...@apache.org>.
XiangpengHao commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2365331618
> while working on https://github.com/apache/datafusion/pull/12092 it turns out that Min / Max on BinaryView and StringView are suuuuper slow.
Fixed in #12575, now min/max on `StringViewArray` should be slightly faster than `StringArray`
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2355799279
> > @alamb is there anyone working on this + is this issue still relevant? I would love to tackle it as it seems like an interesting feature/optimization.
>
> @devanbenz I think this would be a fun and interesting project as well as valuable to DataFusion. However, I also think it is pretty advanced -- I would enjoy helping with it, but also maybe @Rachelint or @jayzhan211 are interested in helping out π€
Great! I'll get started on this later in the week/over the weekend :) Will likely bug folks as I require help haha π
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2355935432
> > @alamb is there anyone working on this + is this issue still relevant? I would love to tackle it as it seems like an interesting feature/optimization.
>
> @devanbenz I think this would be a fun and interesting project as well as valuable to DataFusion. However, I also think it is pretty advanced -- I would enjoy helping with it, but also maybe @Rachelint or @jayzhan211 are interested in helping out π€
Seems really interesting, I am reading the related disscusions.
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2310920383
> @alamb is there anyone working on this + is this issue still relevant? I would love to tackle it as it seems like an interesting feature/optimization.
I dont know of anyone working on this @devanbenz -- but I also don't know of any benchmarks or actual queries that use `min` / `max` on string columns. The place it shows up is when computing statistics when writing parquet, but I think parquet is already pretty good at this (and has its own code to compute min/max)
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2356326589
> The challenge of `String` seems that?
>
> * If we just simply use a `Vec<String>` like `primitives` to keep the min/max values, it is too expensive to convert them to `StringArray`/`StringViewArray`(many many copy)
I think the overhead is actually mostly that there is an additional (small) allocation for each `String`. For queries with a small numer of groups (like 100) an extra 100 allocations isn't all that bad. For queries with millions of groups the overhad is substantial
> * But if we use `StringArray` like approach to keep the values, we can't update the min/max values.
I suppose we could potentially update the values as long as the new strings were shorter :thinking:
> * So Finally we need to use a `StringViewArray` like approach to make it, but still have the new challenge about gc?
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2356392519
> I am not familiar enough with StringViewArray, is it ok to do that? And will it lead to a extremely bad performance?
I think using a single `Buffer` for each string will be bad for performance (likely worse than storing as `String` and copying them at the end. `StringViewArray` is really optimized for a small number of buffers (even though in theory it could have 2B of them as it is indexed on `i32`)
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2368094869
I have had more time to take a look at this and sort of just wrap my head around how `GroupsAccumulatorAdapter` works a bit. I'm seeing that the performance impact is happening here https://github.com/apache/datafusion/blob/a35d0075744a058f81bd9ebed747e2e597434019/datafusion/functions-aggregate-common/src/aggregate/groups_accumulator.rs#L475 in `slice_and_maybe_filter` I'm under the assumption that this is happening due to the difference between the `BinaryView` and the `Binary` Scalar values. Taking a look at https://www.influxdata.com/blog/faster-queries-with-stringview-part-one-influxdb/ I understand that `BinaryView` is effectively a non-contiguous structure where as `Binary` is contiguous so it is easily sliced. So the idea here is to effectively change the underlying state in which the accumulator structure receives that data thus making it either easier to call slice or remove the calling of slice entirely?
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2368222219
> I have had more time to take a look at this and sort of just wrap my head around how `GroupsAccumulatorAdapter` works a bit. I'm seeing that the performance impact is happening here
>
> https://github.com/apache/datafusion/blob/a35d0075744a058f81bd9ebed747e2e597434019/datafusion/functions-aggregate-common/src/aggregate/groups_accumulator.rs#L475
> in `slice_and_maybe_filter` I'm under the assumption that this is happening due to the difference between the `BinaryView` and the `Binary` Scalar values. Taking a look at https://www.influxdata.com/blog/faster-queries-with-stringview-part-one-influxdb/ I understand that `BinaryView` is effectively a non-contiguous structure where as `Binary` is contiguous so it is easily sliced. So the idea here is to effectively change the underlying state in which the accumulator structure receives that data thus making it either easier to call slice or remove the calling of slice entirely?
I guess the current goal is to remove the calling of slice, and get an at least not worse performance than `StringArray + GroupsAccumulatorAdapter` as mentioned above.
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2378010992
Finally got around to trying out a little POC locally. I'm getting much better results by just storing the state as `Vec<String>` it's very much not polished at this moment in time but I will have some more time on Sunday + early next week to continue working on this.
<img width="1716" alt="Screenshot 2024-09-26 at 4 56 25β―PM" src="https://github.com/user-attachments/assets/6ad1aac2-61c4-48cc-9bb1-02d74a47a01e">
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2355356541
It actually turns out that `Min` / `Max` on string/binary columns are in several ClickBench queries:
https://github.com/apache/datafusion/blob/a08f923c2acb1a46614970231d9a672c36ce3ad2/benchmarks/queries/clickbench/queries.sql#L22-L23
https://github.com/apache/datafusion/blob/a08f923c2acb1a46614970231d9a672c36ce3ad2/benchmarks/queries/clickbench/queries.sql#L29
I don't think `min`/`max` have appeared as priorities in benchmarking before because the queries in question are doing other string heavy operations that tend to dominate. Thus the use of `GroupsAccumulatorAdaptor` for Min/Max on strings, while bad, is overshadowed by other things
However, while working on https://github.com/apache/datafusion/pull/12092 it turns out that `Min` / `Max` on `BinaryView` and `StringView` are *suuuuper* slow. We can likely restore their speed to something similar to https://github.com/apache/arrow-rs/issues/6408 but I also think this is a good time to actually make `Min` / `Max` on strings faster. I will write up some ideas on how to do this
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2356370758
Maybe it is not a good way, but can we
> @Rachelint your implicit idea of using `Vec<String>` to store the state I think is actually quite interesting and maybe we should try that one first:
>
> It would at least avoid calling `Array::slice` and likely be better than using `GroupsAccumulatorAdapter`, even if we could improve it later with more explciit memory management π€
π€ Can we still use the string view like approach to store states https://github.com/apache/datafusion/issues/6906#issuecomment-2355604402, but for the uninlined state, we use a single `String` to store it?
And when we output them to `StringViewArray`, we make convert this single `String` to a tiny buffer.
For example, if all states are uninlined(len > 12), the output `StringViewArray` may be like:
```
row1: view1 buffer1(with only one string)
row2: view2 buffer2(with only one string)
...
rown: viewn buffern
```
I am not familiar enough with `StringViewArray`, is it ok to do that? And will it lead to a extremely bad performance?
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2366824707
BTW I was thinking more about this issue -- while a native Min/Max for strings / stringview will help, I have an idea that might make it simply it an added bonus: pushing the cast down into the parquet reader https://github.com/apache/datafusion/issues/12509#issuecomment-2366775581
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed issue #6906: Implement fast min/max accumulator for binary / strings (now it uses the slower path)
URL: https://github.com/apache/datafusion/issues/6906
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2356428850
I think
https://github.com/apache/datafusion/pull/6800#issuecomment-1622236683
> > I am not familiar enough with StringViewArray, is it ok to do that? And will it lead to a extremely bad performance?
>
> I think using a single `Buffer` for each string will be bad for performance (likely worse than storing as `String` and copying them at the end. `StringViewArray` is really optimized for a small number of buffers (even though in theory it could have 2B of them as it is indexed on `i32`)
Ok, for `StringView min/max`, seems we can just start with using `Vec<u128>(views)` to store the inlined state(<= 12), use `Vec<String>` to store the unlined.
And when conerting it to `StringViewArray`, we just copy the `Vec<String>` to create the buffer (`GroupsAccumulatorAdapter` copy the states too).
For the short strings(<=12), it can avoid allocating `String`, and for the long ones, it just do the same thing as `GroupsAccumulatorAdapter`. Seems it can have a better performance(due optimization for shorts)?
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2355857689
In terms of implementation, what I suggest is:
1. Do a POC implementaiton: wire up just enough `StringView, don't worry about GC, basic unit tests
2. Verify it makes the clickbench query faster
3. Flesh out testing, documentation, add support for StringArrary, etc
4. Merge and profit (bonus points for blogging about it)
For the POC here is the reproducer I recommend:
### Step 1. Get `hits_partitioned` using `bench.sh`:
```shell
cd benchmarks
./bench.sh data clickbench_partitioned
```
### Step 2: Prepare a script with reproducer query:
```sql
set datafusion.execution.parquet.schema_force_view_types = true;
SELECT REGEXP_REPLACE("Referer", '^https?://(?:www\\.)?([^/]+)/.*$', '\\1') AS k, AVG(length("Referer")) AS l, COUNT(*) AS c, MIN("Referer")
FROM hits_partitioned
WHERE "Referer" <> '' GROUP BY k HAVING COUNT(*) > 100000 ORDER BY l DESC LIMIT 25;
```
```shell
andrewlamb@Andrews-MacBook-Pro-2:~/Software/datafusion2/benchmarks/data$ cat q28.sql
set datafusion.execution.parquet.schema_force_view_types = true;
SELECT REGEXP_REPLACE("Referer", '^https?://(?:www\\.)?([^/]+)/.*$', '\\1') AS k, AVG(length("Referer")) AS l, COUNT(*) AS c, MIN("Referer")
FROM hits_partitioned
WHERE "Referer" <> '' GROUP BY k HAVING COUNT(*) > 100000 ORDER BY l DESC LIMIT 25;
```
### Step 3: Run script (with release build of `datafusion-cli`):
```shell
datafusion-cli -f q28.sql
```
* `set datafusion.execution.parquet.schema_force_view_types = true;` --> `Elapsed 18.431 seconds.`
* `set datafusion.execution.parquet.schema_force_view_types = false;` --> `Elapsed 6.427 seconds.`
The goal is to get `set datafusion.execution.parquet.schema_force_view_types = true;` to be the same (or better) than when it is false
If you look at the [flamegraph-string-view.svg](https://github.com/user-attachments/assets/e89c5566-c635-467b-9b28-24b7d2d91bfa), you can see most of the time is spent doing GroupsAccumulator
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2356374728
> @Rachelint your implicit idea of using `Vec<String>` to store the state I think is actually quite interesting and maybe we should try that one first:
>
> It would at least avoid calling `Array::slice` and likely be better than using `GroupsAccumulatorAdapter`, even if we could improve it later with more explciit memory management π€
Yes... At least it will be better than now, even we just use `Vec<String>` to impl a specific `GroupsAccumulator` for `String` type...
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "Rachelint (via GitHub)" <gi...@apache.org>.
Rachelint commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2368191455
> > @Rachelint your implicit idea of using `Vec<String>` to store the state I think is actually quite interesting and maybe we should try that one first:
> > It would at least avoid calling `Array::slice` and likely be better than using `GroupsAccumulatorAdapter`, even if we could improve it later with more explciit memory management π€
>
> So this would effectively be using a `Vec<String>` for state instead of your proposal of using something akin to StringView notated here [#6906 (comment)](https://github.com/apache/datafusion/issues/6906#issuecomment-2355629355) @alamb?
I think what @alamb means is that just simply using `Vec<String>` to store the store will be at least not worse than `StringArray + GroupsAccumulatorAdapter`, and it is easy to start from.
And after improve it to be not worse than `StringArray + GroupsAccumulatorAdapter`, we can continue to push forward enabling string view by default.
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2355604402
## Background
(I will make a PR shortly to add this to the actual datafusion docs)
[`GroupsAccumulator`](https://docs.rs/datafusion/latest/datafusion/logical_expr/trait.GroupsAccumulator.html) logically does this:
```
βββββββ
β 0 βββββββββββββΆ "A"
βββββββ€
β 1 βββββββββββββΆ "Z"
βββββββ
... ...
βββββββ
β N-2 β "A"
βββββββ€
β N-1 βββββββββββββΆ "Q"
βββββββ
Logical group Current Min/Max
number value for that
group
GroupsAccumulator to store N aggregate
values: logically keepa a mapping from
each group index to the current value
```
Today, String / Binary min/max values are implemented using [`GroupsAccumulatorAdapter`](https://docs.rs/datafusion/latest/datafusion/physical_expr/struct.GroupsAccumulatorAdapter.html) which results in
```
Individual String
(separate
allocation)
βββββββ ββββββββββββββββββββββββββββ
β 0 βββββββββββββΆβ ScalarValue::Utf8("A") ββββββββββββΆ "A"
βββββββ€ ββββββββββββββββββββββββββββ€
β 1 βββββββββββββΆβ ScalarValue::Utf8("Z") ββββββββββββΆ "Z"
βββββββ ββββββββββββββββββββββββββββ
... ... ...
βββββββ ββββββββββββββββββββββββββββ
β N-2 β β ScalarValue::Utf8("A") ββββββββββββΆ "A"
βββββββ€ ββββββββββββββββββββββββββββ€
β N-1 βββββββββββββΆβ ScalarValue::Utf8("Q") ββββββββββββΆ "Q"
βββββββ ββββββββββββββββββββββββββββ
Logical group Current Min/Max value for that group stored
number as a ScalarValue which points to an
indivdually allocated String
How GroupsAccumulatorAdaptor works today:
stores each current min/max as a
ScalarValue
```
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2355799486
take
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2356554263
> > > I am not familiar enough with StringViewArray, is it ok to do that? And will it lead to a extremely bad performance?
> >
> >
> > I think using a single `Buffer` for each string will be bad for performance (likely worse than storing as `String` and copying them at the end. `StringViewArray` is really optimized for a small number of buffers (even though in theory it could have 2B of them as it is indexed on `i32`)
>
> Ok, for `StringView min/max`, seems we can just start with using `Vec<u128>(views)` to store the inlined state(<= 12), use `Vec<String>` to store the unlined.
>
> And when converting it to `StringViewArray`, we just copy the `Vec<String>` to create the buffer (`GroupsAccumulatorAdapter` copy the states too).
>
> For the short strings(<=12), it can avoid allocating `String`, and for the long ones, it just do the same thing as `GroupsAccumulatorAdapter`. Seems it can have a better performance(due optimization for shorts)?
Seems like a reasonable place to start in my opinion. If we want to get more sophisticated at a later time we can try something more exotic. I suspect there will be times when individual allocations will be faster and times when a buffer will be faster and there will be memory consumption tradeoffs as well.
TLDR we should implement something and as long as it is better than what is currently going on that will be good.
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2405013793
Ok, I think https://github.com/apache/datafusion/pull/12792 is ready to review. It has a nice 10% improvement on Q28 and avoids the slowdown when string view is enabled
--
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] Implement fast min/max accumulator for binary / strings (now it uses the slower path) [datafusion]
Posted by "devanbenz (via GitHub)" <gi...@apache.org>.
devanbenz commented on issue #6906:
URL: https://github.com/apache/datafusion/issues/6906#issuecomment-2369040854
> > I have had more time to take a look at this and sort of just wrap my head around how `GroupsAccumulatorAdapter` works a bit. I'm seeing that the performance impact is happening here
> > https://github.com/apache/datafusion/blob/a35d0075744a058f81bd9ebed747e2e597434019/datafusion/functions-aggregate-common/src/aggregate/groups_accumulator.rs#L475
> >
> > in `slice_and_maybe_filter` I'm under the assumption that this is happening due to the difference between the `BinaryView` and the `Binary` Scalar values. Taking a look at [influxdata.com/blog/faster-queries-with-stringview-part-one-influxdb](https://www.influxdata.com/blog/faster-queries-with-stringview-part-one-influxdb/) I understand that `BinaryView` is effectively a non-contiguous structure where as `Binary` is contiguous so it is easily sliced. So the idea here is to effectively change the underlying state in which the accumulator structure receives that data thus making it either easier to call slice or remove the calling of slice entirely?
>
> I guess the current goal is to remove the calling of slice, and get an at least not worse performance than `StringArray + GroupsAccumulatorAdapter` as mentioned above.
Awesome thank you @Rachelint.
I've begun working on a small POC locally with this information. It took me a bit of reading up on context to get going for sure. After reading through https://docs.rs/datafusion/latest/datafusion/physical_plan/trait.Accumulator.html and https://docs.rs/datafusion/latest/datafusion/logical_expr/trait.GroupsAccumulator.html, tracing through the code locally, and reading through the discussion a few times I now have a better understanding of whats going on and where to start.
--
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