Implement fast min/max accumulator for binary / strings (now it uses the slower path)

Is your feature request related to a problem or challenge?

#6904 introduces some fancy new hashing and ways to implement aggregates

See related blog post: https://datafusion.apache.org/blog/2023/08/05/datafusion_fast_grouping/

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 #6800 (comment) for more details

Additional context

No response

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 🤔 )

@alamb

@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.

@alamb

@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)

It actually turns out that Min / Max on string/binary columns are in several ClickBench queries:

datafusion/benchmarks/queries/clickbench/queries.sql

Lines 22 to 23 in a08f923

    
           SELECT "SearchPhrase", MIN("URL"), COUNT(*) AS c FROM hits WHERE "URL" LIKE '%google%' AND "SearchPhrase" <> '' GROUP BY "SearchPhrase" ORDER BY c DESC LIMIT 10; 
        
           SELECT "SearchPhrase", MIN("URL"), MIN("Title"), COUNT(*) AS c, COUNT(DISTINCT "UserID") FROM hits WHERE "Title" LIKE '%Google%' AND "URL" NOT LIKE '%.google.%' AND "SearchPhrase" <> '' GROUP BY "SearchPhrase" ORDER BY c DESC LIMIT 10;

datafusion/benchmarks/queries/clickbench/queries.sql

Line 29 in a08f923

    
           SELECT REGEXP_REPLACE("Referer", '^https?://(?:www\.)?([^/]+)/.*$', '\1') AS k, AVG(length("Referer")) AS l, COUNT(*) AS c, MIN("Referer") FROM hits WHERE "Referer" <> '' GROUP BY k HAVING COUNT(*) > 100000 ORDER BY l DESC LIMIT 25;

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 #12092 it turns out that Min / Max on BinaryView and StringView are suuuuper slow. We can likely restore their speed to something similar to apache/arrow-rs#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

Background

(I will make a PR shortly to add this to the actual datafusion docs)

GroupsAccumulator 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 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

Potential Design

One high level idea is to build a data structure that uses the same internal format (views/buffers) as StringViewArray 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)

a fixed size u128 view
a variable length part with the string data

As new batches are updated, each View is updated if necessary

Benefits of design:

Hopefully use the same code as in arrow-rs
Allows Zero copy conversion to StringView / BinaryView at output
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 Strings 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 run occasionally

Random Thoughts

Thoughts: maybe this structure (MutableStringViewBuilder??) could be upstreamed eventually

@alamb

@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 🤔

@alamb

@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 😆

take

In terms of implementation, what I suggest is:

Do a POC implementaiton: wire up just enough `StringView, don't worry about GC, basic unit tests
Verify it makes the clickbench query faster
Flesh out testing, documentation, add support for StringArrary, etc
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`:

cd benchmarks
./bench.sh data clickbench_partitioned

Step 2: Prepare a script with reproducer 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;

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`):

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, you can see most of the time is spent doing GroupsAccumulator

@alamb

@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.

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

@alamb

I think what @alamb means is that just simply using Vec 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.

💯

@Rachelint

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

datafusion/datafusion/functions-aggregate-common/src/aggregate/groups_accumulator.rs

Line 475 in a35d007

pub(crate) fn slice_and_maybe_filter(

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 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 where to start, the expected outcome and a path to get there.

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. It is currently only implemented for MIN will need to work on MAX next but I foresee the implementation being almost identical.

Without view types:

25 row(s) fetched.
Elapsed 12.293 seconds.

With view types:

25 row(s) fetched.
Elapsed 12.135 seconds.

A 1.29% performance improvement 😎

Nice! If you want to put up a POC / draft PR I would be more than happy to help work on it with you

Nice! If you want to put up a POC / draft PR I would be more than happy to help work on it with you

Sounds good I pushed up what I have so far as a draft. There are a few things I would like to do including making the accumulator more generic so that I use a single method for min or max and pass an aggregate fn similar to how other UDFs do it.

I will check out #12677 -- I hoped to do it today but I appear to be almost out of time

@alamb

I think we can store (len, prefix, index) for string/binary view cases. We then compare the value based on len + inline or prefix first, if they are the same we look up the actual value with index. In this way, we don't have the garbage collect issue, since we store the index instead of the whole string.

@alamb @Rachelint @devanbenz

@alamb

I think we can store (len, prefix, index) for string/binary view cases. We then compare the value based on len + inline or prefix first, if they are the same we look up the actual value with index. In this way, we don't have the garbage collect issue, since we store the index instead of the whole string.

@alamb @Rachelint @devanbenz

I am a bit confused about where we store actual string values?

We still store it using Vec<String>, and through copying way when constructing the StringView array like #6906 (comment) ?

I think what @Dandandan memtioned in #6800 (comment) seems to be performance best impl?
But it is a bit hard to make it... we need to impl something like memory allocator...

I think we can store (len, prefix, index) for string/binary view cases. We then compare the value based on len + inline or prefix first, if they are the same we look up the actual value with index. In this way, we don't have the garbage collect issue, since we store the index instead of the whole string.

I think the garbage collection issue happens when the current "min" for a group is updated to a new value. We have to copy the new min value into the buffer and adjust the offset, but the old min value is still in the string view buffer

My approach only works within batch not across batch. The garbage collection issue here is that whenever we got a better "min", we need to append the bytes and the old bytes is now "garbage". But given a batch, we can get the string value by the row index of the StringArray, therefore we don't need to update the possible long bytes but update only (len, prefix, row - index) . We only store this "min" (which creates garbage) after computing the whole batch and update it to the accumulator. The more values are in the same group within the batch, the less "garbage" we created compare to the original proposed design. The best case is that the whole batch has the same group value so we just do 1 update to the accumulator, the worst case is that every group value is distinct so we need n (n is batch size and the group size of the batch) updates to the accumulator. Maybe repartition prior to the aggregation helps a lot since the same group stays together in a batch.

The more values are in the same group within the batch, the less "garbage" we created compare to the original proposed design.

this makes a lot of sense (and is similar to what the current non groups accumulator does -- finds the min per batch and then updates the current overall min from that, rather than updating the min for each input row).

I started hacking on this today: #12792. I am fighting with the generics, but I think I am pretty close.

Ok, I think #12792 is ready to review. It has a nice 10% improvement on Q28 and avoids the slowdown when string view is enabled

Navigation Menu

Repository navigation

Implement fast min/max accumulator for binary / strings (now it uses the slower path) #6906

Is your feature request related to a problem or challenge?

Describe the solution you'd like

Describe alternatives you've considered

Additional context

Background

Potential Design

Potential challenges

Random Thoughts

Step 1. Get `hits_partitioned` using `bench.sh`:

Step 2: Prepare a script with reproducer query:

Step 3: Run script (with release build of `datafusion-cli`):

17 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	SELECT "SearchPhrase", MIN("URL"), COUNT(*) AS c FROM hits WHERE "URL" LIKE '%google%' AND "SearchPhrase" <> '' GROUP BY "SearchPhrase" ORDER BY c DESC LIMIT 10;
	SELECT "SearchPhrase", MIN("URL"), MIN("Title"), COUNT(*) AS c, COUNT(DISTINCT "UserID") FROM hits WHERE "Title" LIKE '%Google%' AND "URL" NOT LIKE '%.google.%' AND "SearchPhrase" <> '' GROUP BY "SearchPhrase" ORDER BY c DESC LIMIT 10;

Implement fast min/max accumulator for binary / strings (now it uses the slower path) #6906

Description

Is your feature request related to a problem or challenge?

Describe the solution you'd like

Describe alternatives you've considered

Additional context

Activity

alamb commented on Jan 8, 2024

devanbenz commented on Aug 26, 2024

alamb commented on Aug 26, 2024

alamb commented on Sep 17, 2024

alamb commented on Sep 17, 2024

Background

alamb commented on Sep 17, 2024

Potential Design

Potential challenges

Random Thoughts

alamb commented on Sep 17, 2024

devanbenz commented on Sep 17, 2024

devanbenz commented on Sep 17, 2024

alamb commented on Sep 17, 2024

Step 1. Get hits_partitioned using bench.sh:

Step 2: Prepare a script with reproducer query:

Step 3: Run script (with release build of datafusion-cli):

Rachelint commented on Sep 17, 2024

alamb commented on Sep 17, 2024

17 remaining items

alamb commented on Sep 23, 2024

devanbenz commented on Sep 23, 2024

devanbenz commented on Sep 26, 2024

alamb commented on Sep 29, 2024

devanbenz commented on Sep 30, 2024

alamb commented on Sep 30, 2024

jayzhan211 commented on Oct 4, 2024

Rachelint commented on Oct 4, 2024

alamb commented on Oct 4, 2024

jayzhan211 commented on Oct 5, 2024

alamb commented on Oct 6, 2024

alamb commented on Oct 7, 2024

alamb commented on Oct 10, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Step 1. Get `hits_partitioned` using `bench.sh`:

Step 3: Run script (with release build of `datafusion-cli`):