-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid RowConverter for multi column grouping #12269
base: main
Are you sure you want to change the base?
Conversation
@alamb The approach in this PR is to replace RowConverter and check the equality of the group by values by accessing the certain row and iterate all the group by expressions. The downside is that we need to have type-specific implementation but we could see it outperform Rows by eliminating the cost of I'm thinking of support only primitive, string, datetime those non-nested type. For other less common nested types maybe we just fallback to |
} | ||
|
||
impl<T: ArrowPrimitiveType> ArrayEq for PrimitiveGroupValueBuilder<T> { | ||
fn equal_to(&self, lhs_row: usize, array: &ArrayRef, rhs_row: usize) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
equal_to
and append_val
are two core functions.
equal_to
is to compare the incoming row with the row in group value builder
append_val
is to add row into group value builder
Thanks @jayzhan211 -- I will try and review this over the next day or two (I am catching up from being out last week and I am not back full time until this Thursday) |
} | ||
|
||
for (i, group_val) in group_values_v2.iter().enumerate() { | ||
if !compare_equal(group_val.as_ref(), *group_idx, &cols[i], row) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is called in a loop, this can be optimized/specialized for certain cases like: do the arrays have any nulls or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get it how could I further optimize the loop based on nulls 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea would be change compare_equal to take advantage of cases when, for example, it was known the values couldn't be null (so checking Option
isn't needed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, indeed 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLDR is I think this is a really neat idea @jayzhan211 -- in essence it seems to me this PR basically changes from Row comparison to Column by column comparison.
The theory with using RowCoverter at first I believe is that:
- It handled all possible types and combinations
- The theory was that time spent creating the Row would be paid back by faster comparisons by avoiding dynamic dispatch.
Your benchmark numbers seem to show different results 👌
I thought about how the performance could be so good and I suppose it does make sense because for most aggregate queries, many of the rows will go into an existing group -- so the cost of copying the input, just to find it isn't needed is outweighted
Also, this doesn't make sense to upstream to Arrow for me, it is group by specific implementation, so we need to maintain this in Datafusion. Would like an early feedback on this approach!
I looked at this and I think we could potentially reuse a lot of what is upstream in arrow-rs 's builders. I left comments
I am running the clickbench benchmarks to see if I can confirm the results. If so, I suggest we try and reuse the builders from arrow-rs as much as possible and see how elegant we can make this PR.
But all in all, really nicely done 👏
let mut group_values_v2 = self | ||
.group_values_v2 | ||
.take() | ||
.expect("Can not emit from empty rows"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a neat optimization as well -- as it saves a copy of the intermediate group values 👍
// } | ||
// } | ||
|
||
pub struct ByteGroupValueBuilderNaive<O> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to my comment above, this looks very similar to the GenericBinaryBuilder in arrow -- it would be great if we could simply reuse that instead of a significant amount of copying 🤔
fn build(self: Box<Self>) -> ArrayRef; | ||
} | ||
|
||
pub struct PrimitiveGroupValueBuilder<T: ArrowPrimitiveType>(Vec<Option<T::Native>>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very similar to PrimitiveBuilder
in arrow-rs to me https://docs.rs/arrow/latest/arrow/array/struct.PrimitiveBuilder.html (though I think PrimitiveBuilder
is likely faster / handles nulls better)
I wonder if you could implement ArrayEq
for PrimitiveBuilder
using the methods like https://docs.rs/arrow/latest/arrow/array/struct.PrimitiveBuilder.html#method.values_slice
If so I think you would have a very compelling PR here
} | ||
|
||
for (i, group_val) in group_values_v2.iter().enumerate() { | ||
if !compare_equal(group_val.as_ref(), *group_idx, &cols[i], row) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea would be change compare_equal to take advantage of cases when, for example, it was known the values couldn't be null (so checking Option
isn't needed)
@@ -35,9 +35,4 @@ SELECT "URL", COUNT(*) AS c FROM hits GROUP BY "URL" ORDER BY c DESC LIMIT 10; | |||
SELECT 1, "URL", COUNT(*) AS c FROM hits GROUP BY 1, "URL" ORDER BY c DESC LIMIT 10; | |||
SELECT "ClientIP", "ClientIP" - 1, "ClientIP" - 2, "ClientIP" - 3, COUNT(*) AS c FROM hits GROUP BY "ClientIP", "ClientIP" - 1, "ClientIP" - 2, "ClientIP" - 3 ORDER BY c DESC LIMIT 10; | |||
SELECT "URL", COUNT(*) AS PageViews FROM hits WHERE "CounterID" = 62 AND "EventDate"::INT::DATE >= '2013-07-01' AND "EventDate"::INT::DATE <= '2013-07-31' AND "DontCountHits" = 0 AND "IsRefresh" = 0 AND "URL" <> '' GROUP BY "URL" ORDER BY PageViews DESC LIMIT 10; | |||
SELECT "Title", COUNT(*) AS PageViews FROM hits WHERE "CounterID" = 62 AND "EventDate"::INT::DATE >= '2013-07-01' AND "EventDate"::INT::DATE <= '2013-07-31' AND "DontCountHits" = 0 AND "IsRefresh" = 0 AND "Title" <> '' GROUP BY "Title" ORDER BY PageViews DESC LIMIT 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these queries removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I haven't implement DateTime builder, so couldn't pass the test
TLDR is I ran the benchmarks and it does appear to make a measurable performance improvement on several queries 👍
|
|
9d8dbea
to
5d904a3
Compare
@alamb I found Or maybe we should revert to the previous vector implementation since the performance doesn't differ a lot and it is easier for |
I think using Vec, if possible, is likely to be the best idea as Vec is well understood and highly optimized in Rust (and also has a fast conversion to Arrow arrays) |
@jayzhan211 BTW the results in #12269 (comment) are quite impressive This is a great example of using data driven decisions to come to a better internal architecture / approach (e.g showing with data that using single row comparisons is actually better than using RowCoverter). Very nice 👌 |
Query that compute with Datetime are slower 🤔 |
array: &ArrayRef, | ||
rhs_row: usize, | ||
) -> bool { | ||
array_row.equal_to(lhs_row, array, rhs_row) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nulls are handled in the equal_to
, is there any other place we could further optimize?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One idea I had is that you could defer actually copying the new rows into group_values so rather than calling the function once for each new group, you could call it once per batch, and it could insert all the new values in one function call
That would save some function call overhead as well as the downcasting of arrays and maybe would vectorize better
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
b80dde1
to
07ed966
Compare
@@ -1380,24 +1380,16 @@ mod tests { | |||
"+---+-----+-----------------+", | |||
"| a | b | COUNT(1)[count] |", | |||
"+---+-----+-----------------+", | |||
"| | 1.0 | 1 |", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change is consistent with the one that run in slt in main
.
Not sure which one is the expected behaviour 🤔
The only difference is that we use GroupValuesRowLike
instead GroupValuesRows
for Float
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When in doubt, I always run in postgres to see what it says
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked both postgres and duckdb, the spill
one is the expected result. But why the non-spill
version has the different result
@jayzhan211 what is the status of this PR? Is it ready for another look? Do you need some help? |
The biggest challenge now is that the grouping sets result changed after I add the Float type support, and it seems that the result is not consistent with postgres. It is also not consistent for slt test on main branch. (the result in slt and rust test seems different on main branch and both postgres and duckdb has the result that is different than my current result) Specifically, for query
My current branch will group the values that are the same together without considering grouping sets. |
Given the comment here #7400 (comment)
|
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
…lues Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Thanks @jayzhan211 -- I am running the benchmarks on this PR and plan to review it carefully either later today or tomorrow 🚀 |
I tried to run the clickbench benchmarks and they failed like this:
|
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
this is next on my list |
|
||
pub struct PrimitiveGroupValueBuilder<T: ArrowPrimitiveType> { | ||
group_values: Vec<T::Native>, | ||
nulls: Vec<bool>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably make this faster using a BooleanBufferBuilder or something similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLDR in my opinion this PR is amazing @jayzhan211 -- I found it a pleasure to read and I think the engineering displayed getting here is 🦾 very impressive
(I seem to be using this a lot recently 😁 )
I think this shows that storing multi-column group values also in columns is significantly faster than converting to row. Very very cool. I also think there are several other potential improvements here for improving performance, but all the better if this works well as is
I am rerunning benchmarks and will post the results here but assuming they still show an improvement I think this PR could be merged it as is (I have a few suggestions) but I think it is ready
Some nice to haves / follow ons would be:
-
Improve documentation (I have some ideas, but this PR is already very well commented)
-
Add some additional testing / fuzz testing (especially for nulls/ non null values) for cases (int a, string b) and (string a, int b)
-
Implement a ByteGroupValueBuilder for
StringView
and the other primitive based types (e.g. DecimalArray, the date/time stuff, etc)
|
||
fn append_val(&mut self, array: &ArrayRef, row: usize) { | ||
// non-null fast path | ||
if !self.nullable || !array.is_null(row) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be
if !self.nullable || !array.is_null(row) { | |
if !self.nullable && !array.is_null(row) { |
To catch the first row that is nullable?
// Sanity array type | ||
match self.output_type { | ||
OutputType::Binary => { | ||
assert!(matches!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can make this go even faster with debug_assert or something.
// SAFETY: | ||
// 1. the offsets were constructed safely | ||
// | ||
// 2. we asserted the input arrays were all the correct type and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
/// The actual group by values, stored column-wise. Compare from | ||
/// the left to right, each column is stored as `ArrayRowEq`. | ||
/// This is shown faster than the row format | ||
group_values: Option<Vec<Box<dyn ArrayRowEq>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain the difference between what None
and Some(vec![])
is?
I wonder if you can make this simpler by just using a Vec and checking for empty vec?
group_values: Option<Vec<Box<dyn ArrayRowEq>>>, | |
group_values: Vec<Box<dyn ArrayRowEq>>, |
fn take_n(&mut self, n: usize) -> ArrayRef { | ||
assert!(self.len() >= n); | ||
|
||
let mut nulls_count = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null / shifting logic here feels like it should be unit tested to me (maybe fuzz testing would be enough)
Also, I am reminded of @kazuyukitanimura 's PR in arrow to improve set_bits
apache/arrow-rs#6288 which I think could be used here as well and now even more optimized
fn take_n(&mut self, n: usize) -> ArrayRef; | ||
} | ||
|
||
pub struct PrimitiveGroupValueBuilder<T: ArrowPrimitiveType> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a high level, I feel like this builder and the ByteGroupValueBuilder
feel like they are very similar to builders in arrow-rs. I don't know if we could actually upstream any of this / use the upstream builders, but if we could we could probably save some significant time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, when @wiedld and I were discussing this PR, we realized this is quite similar to
values: Vec<T::Native>, |
I wonder if we could potentially use that (i.e. impl ArrayRowEq
for GroupValuesPrimitive
) 🤔 -- that code is already pretty well tested and optimized 🤔
Though maybe null handling has to be different
@@ -0,0 +1,393 @@ | |||
// Licensed to the Apache Software Foundation (ASF) under one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module feels like it is an implementation detail of datafusion/physical-plan/src/aggregates/group_values/row_like.rs, so I suggest putting it in that module (physical-plan
rather than physical-expr-common
)
array: &ArrayRef, | ||
rhs_row: usize, | ||
) -> bool { | ||
array_row.equal_to(lhs_row, array, rhs_row) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One idea I had is that you could defer actually copying the new rows into group_values so rather than calling the function once for each new group, you could call it once per batch, and it could insert all the new values in one function call
That would save some function call overhead as well as the downcasting of arrays and maybe would vectorize better
.map(|f| f.data_type()) | ||
.all(has_row_like_feature) | ||
{ | ||
Ok(Box::new(GroupValuesRowLike::try_new(schema)?)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading this from afar, I would say the name GroupValuesRowLike
is confusing to me as it seems the key is that the group values are stored in columns rather than rows 🤔
Perhaps renaming GroupValuesRowLike
to GroupValuesColumn
would make this clearer
/// Trait for group values column-wise row comparison | ||
pub trait ArrayRowEq: Send + Sync { | ||
fn equal_to(&self, lhs_row: usize, array: &ArrayRef, rhs_row: usize) -> bool; | ||
fn append_val(&mut self, array: &ArrayRef, row: usize); | ||
fn len(&self) -> usize; | ||
fn is_empty(&self) -> bool; | ||
fn build(self: Box<Self>) -> ArrayRef; | ||
fn take_n(&mut self, n: usize) -> ArrayRef; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some doc suggestions:
/// Trait for group values column-wise row comparison | |
pub trait ArrayRowEq: Send + Sync { | |
fn equal_to(&self, lhs_row: usize, array: &ArrayRef, rhs_row: usize) -> bool; | |
fn append_val(&mut self, array: &ArrayRef, row: usize); | |
fn len(&self) -> usize; | |
fn is_empty(&self) -> bool; | |
fn build(self: Box<Self>) -> ArrayRef; | |
fn take_n(&mut self, n: usize) -> ArrayRef; | |
} | |
/// Trait for group values column-wise row comparison | |
/// | |
/// Implementations of this trait store a in-progress collection of group values | |
/// (similar to various builders in Arrow-rs) that allow for quick comparison to | |
/// incoming rows. | |
/// | |
pub trait ArrayRowEq: Send + Sync { | |
/// Returns equal if the row stored in this builder at `lhs_row` is equal to | |
/// the row in `array` at `rhs_row` | |
fn equal_to(&self, lhs_row: usize, array: &ArrayRef, rhs_row: usize) -> bool; | |
/// Appends the row at `row` in `array` to this builder | |
fn append_val(&mut self, array: &ArrayRef, row: usize); | |
/// Returns the number of rows stored in this builder | |
fn len(&self) -> usize; | |
/// Returns true if this builder is empty | |
fn is_empty(&self) -> bool; | |
/// Builds a new array from all of the stored rows | |
fn build(self: Box<Self>) -> ArrayRef; | |
/// Builds a new array from the first `n` stored rows, shifting the | |
/// remaining rows to the start of the builder | |
fn take_n(&mut self, n: usize) -> ArrayRef; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Nice work @jayzhan211
I think we can continue to improve / optimize this code, but the benchmark results speak for themselves. Really nice work
--------------------
Benchmark clickbench_1.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query ┃ main_base ┃ row-group ┃ Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━╇━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 0 │ 0.65ms │ 0.67ms │ no change │
│ QQuery 1 │ 71.17ms │ 71.11ms │ no change │
│ QQuery 2 │ 123.47ms │ 125.05ms │ no change │
│ QQuery 3 │ 135.88ms │ 129.59ms │ no change │
│ QQuery 4 │ 977.05ms │ 969.01ms │ no change │
│ QQuery 5 │ 1097.08ms │ 1103.84ms │ no change │
│ QQuery 6 │ 64.50ms │ 66.47ms │ no change │
│ QQuery 7 │ 83.28ms │ 81.94ms │ no change │
│ QQuery 8 │ 1471.54ms │ 1394.71ms │ +1.06x faster │
│ QQuery 9 │ 1391.33ms │ 1362.55ms │ no change │
│ QQuery 10 │ 471.01ms │ 455.89ms │ no change │
│ QQuery 11 │ 518.11ms │ 495.06ms │ no change │
│ QQuery 12 │ 1226.53ms │ 1228.29ms │ no change │
│ QQuery 13 │ 2239.79ms │ 1867.38ms │ +1.20x faster │
│ QQuery 14 │ 1679.84ms │ 1336.33ms │ +1.26x faster │
│ QQuery 15 │ 1147.96ms │ 1140.62ms │ no change │
│ QQuery 16 │ 3027.32ms │ 2674.68ms │ +1.13x faster │
│ QQuery 17 │ 2811.21ms │ 2472.53ms │ +1.14x faster │
│ QQuery 18 │ 5971.43ms │ 5117.77ms │ +1.17x faster │
│ QQuery 19 │ 119.98ms │ 119.23ms │ no change │
│ QQuery 20 │ 1675.42ms │ 1683.73ms │ no change │
│ QQuery 21 │ 2088.10ms │ 2130.67ms │ no change │
│ QQuery 22 │ 5067.34ms │ 5097.95ms │ no change │
│ QQuery 23 │ 12066.88ms │ 11898.76ms │ no change │
│ QQuery 24 │ 798.95ms │ 769.36ms │ no change │
│ QQuery 25 │ 724.90ms │ 689.24ms │ no change │
│ QQuery 26 │ 867.08ms │ 862.14ms │ no change │
│ QQuery 27 │ 2609.24ms │ 2600.77ms │ no change │
│ QQuery 28 │ 15367.14ms │ 16229.38ms │ 1.06x slower │
│ QQuery 29 │ 563.78ms │ 574.95ms │ no change │
│ QQuery 30 │ 1287.56ms │ 1237.79ms │ no change │
│ QQuery 31 │ 1364.54ms │ 1282.52ms │ +1.06x faster │
│ QQuery 32 │ 4735.13ms │ 4309.81ms │ +1.10x faster │
│ QQuery 33 │ 5307.06ms │ 5354.96ms │ no change │
│ QQuery 34 │ 5252.21ms │ 5283.19ms │ no change │
│ QQuery 35 │ 1850.47ms │ 1877.84ms │ no change │
│ QQuery 36 │ 317.13ms │ 325.94ms │ no change │
│ QQuery 37 │ 215.65ms │ 223.00ms │ no change │
│ QQuery 38 │ 202.05ms │ 191.70ms │ +1.05x faster │
│ QQuery 39 │ 1077.51ms │ 847.19ms │ +1.27x faster │
│ QQuery 40 │ 93.08ms │ 86.30ms │ +1.08x faster │
│ QQuery 41 │ 81.67ms │ 77.70ms │ no change │
│ QQuery 42 │ 98.51ms │ 93.09ms │ +1.06x faster │
└──────────────┴────────────┴────────────┴───────────────┘
--------------------
Benchmark clickbench_partitioned.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query ┃ main_base ┃ row-group ┃ Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━╇━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 0 │ 2.22ms │ 2.22ms │ no change │
│ QQuery 1 │ 38.52ms │ 38.60ms │ no change │
│ QQuery 2 │ 93.93ms │ 92.83ms │ no change │
│ QQuery 3 │ 100.91ms │ 99.54ms │ no change │
│ QQuery 4 │ 935.01ms │ 928.03ms │ no change │
│ QQuery 5 │ 971.85ms │ 977.00ms │ no change │
│ QQuery 6 │ 33.86ms │ 33.53ms │ no change │
│ QQuery 7 │ 42.08ms │ 41.74ms │ no change │
│ QQuery 8 │ 1444.01ms │ 1412.74ms │ no change │
│ QQuery 9 │ 1319.34ms │ 1355.49ms │ no change │
│ QQuery 10 │ 364.25ms │ 345.23ms │ +1.06x faster │
│ QQuery 11 │ 397.19ms │ 388.98ms │ no change │
│ QQuery 12 │ 1097.11ms │ 1078.08ms │ no change │
│ QQuery 13 │ 1929.01ms │ 1732.75ms │ +1.11x faster │
│ QQuery 14 │ 1545.16ms │ 1258.11ms │ +1.23x faster │
│ QQuery 15 │ 1087.34ms │ 1081.88ms │ no change │
│ QQuery 16 │ 2953.26ms │ 2597.77ms │ +1.14x faster │
│ QQuery 17 │ 2741.90ms │ 2378.29ms │ +1.15x faster │
│ QQuery 18 │ 5821.55ms │ 5101.19ms │ +1.14x faster │
│ QQuery 19 │ 92.23ms │ 92.97ms │ no change │
│ QQuery 20 │ 1734.66ms │ 1771.89ms │ no change │
│ QQuery 21 │ 2058.21ms │ 2044.49ms │ no change │
│ QQuery 22 │ 5208.74ms │ 5197.48ms │ no change │
│ QQuery 23 │ 10475.98ms │ 10422.24ms │ no change │
│ QQuery 24 │ 601.41ms │ 572.15ms │ no change │
│ QQuery 25 │ 498.67ms │ 497.77ms │ no change │
│ QQuery 26 │ 660.25ms │ 656.65ms │ no change │
│ QQuery 27 │ 2599.62ms │ 2577.07ms │ no change │
│ QQuery 28 │ 14563.07ms │ 15297.20ms │ 1.05x slower │
│ QQuery 29 │ 517.92ms │ 524.98ms │ no change │
│ QQuery 30 │ 1123.10ms │ 1052.28ms │ +1.07x faster │
│ QQuery 31 │ 1169.77ms │ 1119.14ms │ no change │
│ QQuery 32 │ 4853.42ms │ 4234.03ms │ +1.15x faster │
│ QQuery 33 │ 5231.77ms │ 5175.85ms │ no change │
│ QQuery 34 │ 5142.16ms │ 5106.57ms │ no change │
│ QQuery 35 │ 1818.27ms │ 2074.98ms │ 1.14x slower │
│ QQuery 36 │ 268.04ms │ 275.51ms │ no change │
│ QQuery 37 │ 125.96ms │ 120.91ms │ no change │
│ QQuery 38 │ 140.83ms │ 146.25ms │ no change │
│ QQuery 39 │ 951.76ms │ 749.69ms │ +1.27x faster │
│ QQuery 40 │ 52.12ms │ 55.88ms │ 1.07x slower │
│ QQuery 41 │ 47.74ms │ 48.89ms │ no change │
│ QQuery 42 │ 63.78ms │ 61.94ms │ no change │
└──────────────┴────────────┴────────────┴───────────────┘
--------------------
Benchmark tpch_sf1.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query ┃ main_base ┃ row-group ┃ Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 1 │ 252.62ms │ 196.18ms │ +1.29x faster │
│ QQuery 2 │ 131.31ms │ 111.47ms │ +1.18x faster │
│ QQuery 3 │ 138.74ms │ 131.76ms │ +1.05x faster │
│ QQuery 4 │ 92.29ms │ 90.21ms │ no change │
│ QQuery 5 │ 180.88ms │ 157.42ms │ +1.15x faster │
│ QQuery 6 │ 60.26ms │ 53.12ms │ +1.13x faster │
│ QQuery 7 │ 223.37ms │ 202.25ms │ +1.10x faster │
│ QQuery 8 │ 165.38ms │ 169.47ms │ no change │
│ QQuery 9 │ 259.49ms │ 254.33ms │ no change │
│ QQuery 10 │ 244.94ms │ 223.46ms │ +1.10x faster │
│ QQuery 11 │ 104.23ms │ 93.90ms │ +1.11x faster │
│ QQuery 12 │ 135.08ms │ 128.36ms │ no change │
│ QQuery 13 │ 304.56ms │ 211.85ms │ +1.44x faster │
│ QQuery 14 │ 97.08ms │ 73.28ms │ +1.32x faster │
│ QQuery 15 │ 128.14ms │ 103.56ms │ +1.24x faster │
│ QQuery 16 │ 85.69ms │ 80.47ms │ +1.06x faster │
│ QQuery 17 │ 246.47ms │ 218.66ms │ +1.13x faster │
│ QQuery 18 │ 344.79ms │ 314.93ms │ +1.09x faster │
│ QQuery 19 │ 162.78ms │ 122.87ms │ +1.32x faster │
│ QQuery 20 │ 145.63ms │ 142.05ms │ no change │
│ QQuery 21 │ 276.08ms │ 265.45ms │ no change │
│ QQuery 22 │ 65.63ms │ 62.79ms │ no change │
└──────────────┴───────────┴───────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━┓
┃ Benchmark Summary ┃ ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━┩
│ Total Time (main_base) │ 3845.43ms │
│ Total Time (row-group) │ 3407.84ms │
│ Average Time (main_base) │ 174.79ms │
│ Average Time (row-group) │ 154.90ms │
│ Queries Faster │ 15 │
│ Queries Slower │ 0 │
│ Queries with No Change │ 7 │
└──────────────────────────┴───────────┘
Which issue does this PR close?
Closes #.
Rationale for this change
To avoid Row converter in multi group by clause, we add equality check for group values Array.
We can see a improvement on group by query (much more for string types). The downside is that this is type-specific design unlike Rows that covers all the types
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?
Benchmark
Query after 37 is removed since DateTime is not supported