Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
github-actions[bot] closed pull request #16136: Specialized `GroupValues` for `primitive` and `large_primitive` URL: https://github.com/apache/datafusion/pull/16136 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
github-actions[bot] commented on PR #16136: URL: https://github.com/apache/datafusion/pull/16136#issuecomment-3130331822 Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
alamb commented on PR #16136: URL: https://github.com/apache/datafusion/pull/16136#issuecomment-2917647569 🤖: Benchmark completed Details ``` Comparing HEAD and improve-primitive-group-values Benchmark clickbench_extended.json ┏━━┳┳┳━━┓ ┃ Query┃ HEAD ┃ improve-primitive-group-values ┃ Change ┃ ┡━━╇╇╇━━┩ │ QQuery 0 │ 1915.07ms │ 1925.29ms │no change │ │ QQuery 1 │ 696.08ms │ 702.78ms │no change │ │ QQuery 2 │ 1421.79ms │ 1407.15ms │no change │ │ QQuery 3 │ 693.44ms │ 689.96ms │no change │ │ QQuery 4 │ 1454.69ms │ 1447.10ms │no change │ │ QQuery 5 │ 15331.97ms │ 15156.51ms │no change │ │ QQuery 6 │ 2106.64ms │ 2044.70ms │no change │ │ QQuery 7 │ 2037.56ms │ 2207.73ms │ 1.08x slower │ │ QQuery 8 │ 818.29ms │ 826.34ms │no change │ └──┴┴┴──┘ ┏━━━┳┓ ┃ Benchmark Summary ┃┃ ┡━━━╇┩ │ Total Time (HEAD) │ 26475.53ms │ │ Total Time (improve-primitive-group-values) │ 26407.55ms │ │ Average Time (HEAD) │ 2941.73ms │ │ Average Time (improve-primitive-group-values) │ 2934.17ms │ │ Queries Faster│ 0 │ │ Queries Slower│ 1 │ │ Queries with No Change│ 8 │ └───┴┘ Benchmark clickbench_partitioned.json ┏━━┳┳┳━━┓ ┃ Query┃ HEAD ┃ improve-primitive-group-values ┃ Change ┃ ┡━━╇╇╇━━┩ │ QQuery 0 │15.44ms │15.45ms │no change │ │ QQuery 1 │32.31ms │32.50ms │no change │ │ QQuery 2 │79.82ms │81.50ms │no change │ │ QQuery 3 │96.11ms │97.19ms │no change │ │ QQuery 4 │ 576.73ms │ 582.80ms │no change │ │ QQuery 5 │ 874.26ms │ 897.85ms │no change │ │ QQuery 6 │22.21ms │23.67ms │ 1.07x slower │ │ QQuery 7 │36.56ms │38.49ms │ 1.05x slower │ │ QQuery 8 │ 876.79ms │ 901.41ms │no change │ │ QQuery 9 │ 1180.79ms │ 1184.28ms │no change │ │ QQuery 10│ 260.73ms │ 262.17ms │no change │ │ QQuery 11│ 293.15ms │ 294.16ms │no change │ │ QQuery 12│ 905.30ms │ 887.00ms │no change │ │ QQuery 13│ 1375.72ms │ 1387.10ms │no change │ │ QQuery 14│ 855.54ms │ 838.72ms │no change │ │ QQuery 15│ 819.43ms │ 824.34ms │no change │ │ QQuery 16│ 1717.88ms │ 1721.36ms │no change │ │ QQuery 17│ 1576.03ms │ 1618.82ms │no change │ │ QQuery 18│ 3021.58ms │ 3057.47ms │no change │ │ QQuery 19│81.36ms │85.15ms │no change │ │ QQuery 20│ 1149.28ms │ 1140.23ms │no change │ │ QQuery 21│ 1335.99ms │ 1327.81ms │no change │ │ QQuery 22│ 2276.34ms │ 2214.15ms │no change │ │ QQuery 23│ 8187.11ms │ 8094.83ms │no change │ │ QQuery 24│ 469.30ms │ 467.50ms │no change │ │ QQuery 25│ 401.62ms │ 387.40ms │no change │ │ QQuery 26│ 542.07ms │ 536.12ms │no change │ │ QQuery 27│ 1624.08ms │ 1595.77ms │no change │ │ QQuery 28│ 12366.86ms │ 12744.64ms │no change │ │ QQuery 29│ 537.01ms │ 524.08ms │no change │ │ QQuery 30│ 814.22ms │ 809.74ms │no change │ │ QQuery 31│ 848.16ms │ 845.50ms │
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
alamb commented on PR #16136: URL: https://github.com/apache/datafusion/pull/16136#issuecomment-2917562287 🤖 `./gh_compare_branch.sh` [Benchmark Script](https://github.com/alamb/datafusion-benchmarking/blob/main/gh_compare_branch.sh) Running Linux aal-dev 6.11.0-1013-gcp #13~24.04.1-Ubuntu SMP Wed Apr 2 16:34:16 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux Comparing improve-primitive-group-values (8e09c834544c692b5b446dcc017b01dd9d4a1c2d) to 17fe504b0b8ca4dfaaf81482557d3ff2e86df52f [diff](https://github.com/apache/datafusion/compare/17fe504b0b8ca4dfaaf81482557d3ff2e86df52f..8e09c834544c692b5b446dcc017b01dd9d4a1c2d) Benchmarks: tpch_mem clickbench_partitioned clickbench_extended Results will be posted here when complete -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Rachelint commented on PR #16136: URL: https://github.com/apache/datafusion/pull/16136#issuecomment-2917542628 > I think this PR makes things better so approving. Nice work @Rachelint. Thanks @alamb , I think still two blocked things before merging it: - Maybe we should also compare the performance of this one with what @Dandandan mentioned https://github.com/apache/datafusion/pull/16136#discussion_r2100764581 - Run the benchmark again, and see if it always reproducable (I still have concern about no improvement in q8 of clickbench_extended, it is not as expected...). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Rachelint commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2112691325
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs:
##
@@ -116,42 +122,60 @@ where
{
fn intern(&mut self, cols: &[ArrayRef], groups: &mut Vec) ->
Result<()> {
assert_eq!(cols.len(), 1);
+let col = cols[0].as_primitive::();
+
groups.clear();
+self.append_row_indices.clear();
-for v in cols[0].as_primitive::() {
+let mut num_total_groups = self.values.len();
+for (row_index, v) in col.iter().enumerate() {
let group_id = match v {
None => *self.null_group.get_or_insert_with(|| {
-let group_id = self.values.len();
-self.values.push(Default::default());
+let group_id = num_total_groups;
+self.append_row_indices.push(row_index as u32);
+num_total_groups += 1;
group_id
}),
Some(key) => {
let state = &self.random_state;
let hash = key.hash(state);
let insert = self.map.entry(
hash,
-|&(g, _)| unsafe {
self.values.get_unchecked(g).is_eq(key) },
-|&(_, h)| h,
+|&(_, v)| v.is_eq(key),
+|&(_, v)| v.hash(state),
);
match insert {
hashbrown::hash_table::Entry::Occupied(o) => o.get().0,
hashbrown::hash_table::Entry::Vacant(v) => {
-let g = self.values.len();
-v.insert((g, hash));
-self.values.push(key);
+let g = num_total_groups;
+v.insert((g, key));
+self.append_row_indices.push(row_index as u32);
+num_total_groups += 1;
g
}
}
}
};
groups.push(group_id)
}
+
+// If all are new groups, we just extend it
+if self.append_row_indices.len() == col.len() {
+self.values.extend_from_slice(col.values());
+} else {
+let col_values = col.values();
+for &row_index in self.append_row_indices.iter() {
Review Comment:
Actually... I found no obvious improvement when switching to `extend`...
The bottleneck still the `hashtable`, I think it is better to just keep the
original `push` logic because it may be simpler, and actually efficient enough.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Dandandan commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2104358004
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs:
##
@@ -116,42 +122,60 @@ where
{
fn intern(&mut self, cols: &[ArrayRef], groups: &mut Vec) ->
Result<()> {
assert_eq!(cols.len(), 1);
+let col = cols[0].as_primitive::();
+
groups.clear();
+self.append_row_indices.clear();
-for v in cols[0].as_primitive::() {
+let mut num_total_groups = self.values.len();
+for (row_index, v) in col.iter().enumerate() {
let group_id = match v {
None => *self.null_group.get_or_insert_with(|| {
-let group_id = self.values.len();
-self.values.push(Default::default());
+let group_id = num_total_groups;
+self.append_row_indices.push(row_index as u32);
+num_total_groups += 1;
group_id
}),
Some(key) => {
let state = &self.random_state;
let hash = key.hash(state);
let insert = self.map.entry(
hash,
-|&(g, _)| unsafe {
self.values.get_unchecked(g).is_eq(key) },
-|&(_, h)| h,
+|&(_, v)| v.is_eq(key),
+|&(_, v)| v.hash(state),
);
match insert {
hashbrown::hash_table::Entry::Occupied(o) => o.get().0,
hashbrown::hash_table::Entry::Vacant(v) => {
-let g = self.values.len();
-v.insert((g, hash));
-self.values.push(key);
+let g = num_total_groups;
+v.insert((g, key));
+self.append_row_indices.push(row_index as u32);
+num_total_groups += 1;
g
}
}
}
};
groups.push(group_id)
Review Comment:
this can `extend` to `groups`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Dandandan commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2104358998
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs:
##
@@ -116,42 +122,60 @@ where
{
fn intern(&mut self, cols: &[ArrayRef], groups: &mut Vec) ->
Result<()> {
assert_eq!(cols.len(), 1);
+let col = cols[0].as_primitive::();
+
groups.clear();
+self.append_row_indices.clear();
-for v in cols[0].as_primitive::() {
+let mut num_total_groups = self.values.len();
+for (row_index, v) in col.iter().enumerate() {
let group_id = match v {
None => *self.null_group.get_or_insert_with(|| {
-let group_id = self.values.len();
-self.values.push(Default::default());
+let group_id = num_total_groups;
+self.append_row_indices.push(row_index as u32);
+num_total_groups += 1;
group_id
}),
Some(key) => {
let state = &self.random_state;
let hash = key.hash(state);
let insert = self.map.entry(
hash,
-|&(g, _)| unsafe {
self.values.get_unchecked(g).is_eq(key) },
-|&(_, h)| h,
+|&(_, v)| v.is_eq(key),
+|&(_, v)| v.hash(state),
);
match insert {
hashbrown::hash_table::Entry::Occupied(o) => o.get().0,
hashbrown::hash_table::Entry::Vacant(v) => {
-let g = self.values.len();
-v.insert((g, hash));
-self.values.push(key);
+let g = num_total_groups;
+v.insert((g, key));
+self.append_row_indices.push(row_index as u32);
+num_total_groups += 1;
g
}
}
}
};
groups.push(group_id)
}
+
+// If all are new groups, we just extend it
+if self.append_row_indices.len() == col.len() {
+self.values.extend_from_slice(col.values());
+} else {
+let col_values = col.values();
+for &row_index in self.append_row_indices.iter() {
Review Comment:
can be written as
`self.values.extend(self.append_row_indices.iter().map(...)`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Dandandan commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2104357071
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/large_primitive.rs:
##
@@ -0,0 +1,139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::aggregates::group_values::single_group_by::primitive::{
+emit_internal, HashValue,
+};
+use crate::aggregates::group_values::GroupValues;
+use ahash::RandomState;
+use arrow::array::{
+cast::AsArray, ArrayRef, ArrowNativeTypeOp, ArrowPrimitiveType,
PrimitiveArray,
+};
+use arrow::datatypes::DataType;
+use arrow::record_batch::RecordBatch;
+use datafusion_common::Result;
+use datafusion_execution::memory_pool::proxy::VecAllocExt;
+use datafusion_expr::EmitTo;
+use hashbrown::hash_table::HashTable;
+use std::mem::size_of;
+
+/// A [`GroupValues`] storing a single column of large primitive values (bits
> 64)
+///
+/// This specialization is significantly faster than using the more general
+/// purpose `Row`s format
+pub struct GroupValuesLargePrimitive {
+/// The data type of the output array
+data_type: DataType,
+/// Stores the `(group_index, hash)` based on the hash of its value
+///
+/// We also store `hash` is for reducing cost of rehashing. Such cost
+/// is obvious in high cardinality group by situation.
+/// More details can see:
+///
+///
+map: HashTable<(usize, u64)>,
+/// The group index of the null value if any
+null_group: Option,
+/// The values for each group index
+values: Vec,
+/// The random state used to generate hashes
+random_state: RandomState,
+}
+
+impl GroupValuesLargePrimitive {
+pub fn new(data_type: DataType) -> Self {
+assert!(PrimitiveArrayis_compatible(&data_type));
+Self {
+data_type,
+map: HashTable::with_capacity(128),
+values: Vec::with_capacity(128),
+null_group: None,
+random_state: Default::default(),
+}
+}
+}
+
+impl GroupValues for GroupValuesLargePrimitive
+where
+T::Native: HashValue,
+{
+fn intern(&mut self, cols: &[ArrayRef], groups: &mut Vec) ->
Result<()> {
+assert_eq!(cols.len(), 1);
+groups.clear();
+
+for v in cols[0].as_primitive::() {
+let group_id = match v {
+None => *self.null_group.get_or_insert_with(|| {
+let group_id = self.values.len();
+self.values.push(Default::default());
+group_id
+}),
+Some(key) => {
+let state = &self.random_state;
+let hash = key.hash(state);
+let insert = self.map.entry(
+hash,
+|&(g, _)| unsafe {
self.values.get_unchecked(g).is_eq(key) },
+|&(_, h)| h,
+);
+
+match insert {
+hashbrown::hash_table::Entry::Occupied(o) => o.get().0,
+hashbrown::hash_table::Entry::Vacant(v) => {
+let g = self.values.len();
+v.insert((g, hash));
+self.values.push(key);
+g
+}
+}
+}
+};
+groups.push(group_id)
Review Comment:
It can `extend` to groups here by writing this as iterator.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
alamb commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2103313269
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/large_primitive.rs:
##
@@ -0,0 +1,139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::aggregates::group_values::single_group_by::primitive::{
+emit_internal, HashValue,
+};
+use crate::aggregates::group_values::GroupValues;
+use ahash::RandomState;
+use arrow::array::{
+cast::AsArray, ArrayRef, ArrowNativeTypeOp, ArrowPrimitiveType,
PrimitiveArray,
+};
+use arrow::datatypes::DataType;
+use arrow::record_batch::RecordBatch;
+use datafusion_common::Result;
+use datafusion_execution::memory_pool::proxy::VecAllocExt;
+use datafusion_expr::EmitTo;
+use hashbrown::hash_table::HashTable;
+use std::mem::size_of;
+
+/// A [`GroupValues`] storing a single column of large primitive values (bits
> 64)
+///
+/// This specialization is significantly faster than using the more general
+/// purpose `Row`s format
Review Comment:
I think it would help me a lot if you could document the rationale behind
this change and why it is different than `GroupValuesPrimitive`
```suggestion
/// This specialization is faster than [`GroupValuesPrimitive`] because it
does not store values directly
/// in the hash table, but instead stores an index into self.vales
```
Or something like that
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Rachelint commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2101922845
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/large_primitive.rs:
##
@@ -0,0 +1,139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::aggregates::group_values::single_group_by::primitive::{
+emit_internal, HashValue,
+};
+use crate::aggregates::group_values::GroupValues;
+use ahash::RandomState;
+use arrow::array::{
+cast::AsArray, ArrayRef, ArrowNativeTypeOp, ArrowPrimitiveType,
PrimitiveArray,
+};
+use arrow::datatypes::DataType;
+use arrow::record_batch::RecordBatch;
+use datafusion_common::Result;
+use datafusion_execution::memory_pool::proxy::VecAllocExt;
+use datafusion_expr::EmitTo;
+use hashbrown::hash_table::HashTable;
+use std::mem::size_of;
+
+/// A [`GroupValues`] storing a single column of large primitive values (bits
> 64)
+///
+/// This specialization is significantly faster than using the more general
+/// purpose `Row`s format
+pub struct GroupValuesLargePrimitive {
Review Comment:
> Hm I think the storage of a value inside `HashTable` will not be very
different from `Vec` besides being inline?
I think for the `value <= 64bits`, storing it inside `HashTable`:
- Can decrease the size of `HashTable` comparing to store their hashes?
- And when probing and rehashing of `HashTable` totally in-place, and
eliminting the random read to a the outside `Vec`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Rachelint commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2101922845
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/large_primitive.rs:
##
@@ -0,0 +1,139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::aggregates::group_values::single_group_by::primitive::{
+emit_internal, HashValue,
+};
+use crate::aggregates::group_values::GroupValues;
+use ahash::RandomState;
+use arrow::array::{
+cast::AsArray, ArrayRef, ArrowNativeTypeOp, ArrowPrimitiveType,
PrimitiveArray,
+};
+use arrow::datatypes::DataType;
+use arrow::record_batch::RecordBatch;
+use datafusion_common::Result;
+use datafusion_execution::memory_pool::proxy::VecAllocExt;
+use datafusion_expr::EmitTo;
+use hashbrown::hash_table::HashTable;
+use std::mem::size_of;
+
+/// A [`GroupValues`] storing a single column of large primitive values (bits
> 64)
+///
+/// This specialization is significantly faster than using the more general
+/// purpose `Row`s format
+pub struct GroupValuesLargePrimitive {
Review Comment:
> Hm I think the storage of a value inside `HashTable` will not be very
different from `Vec` besides being inline?
I think for the `value <= 64bits`, storing it inside `HashTable`:
- Can decrease the size of `HashTable` comparing to store their hashes?
- And when probing and rehashing of `HashTable` totally in-place, and
eliminting the random read to a the outside `Vec`?
But I am indeed not sure, for the large primitive, if doing this can get
benefit, because storing them inline `HashTable` will make the table larger
than storing their hashes...
Also comparing them directly is more expansive than comparing their hashes...
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Rachelint commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2101922845
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/large_primitive.rs:
##
@@ -0,0 +1,139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::aggregates::group_values::single_group_by::primitive::{
+emit_internal, HashValue,
+};
+use crate::aggregates::group_values::GroupValues;
+use ahash::RandomState;
+use arrow::array::{
+cast::AsArray, ArrayRef, ArrowNativeTypeOp, ArrowPrimitiveType,
PrimitiveArray,
+};
+use arrow::datatypes::DataType;
+use arrow::record_batch::RecordBatch;
+use datafusion_common::Result;
+use datafusion_execution::memory_pool::proxy::VecAllocExt;
+use datafusion_expr::EmitTo;
+use hashbrown::hash_table::HashTable;
+use std::mem::size_of;
+
+/// A [`GroupValues`] storing a single column of large primitive values (bits
> 64)
+///
+/// This specialization is significantly faster than using the more general
+/// purpose `Row`s format
+pub struct GroupValuesLargePrimitive {
Review Comment:
> Hm I think the storage of a value inside `HashTable` will not be very
different from `Vec` besides being inline?
I think for the `value <= 64bits`, storing it inside `HashTable`:
- Can decrease the size of `HashTable` comparing to store their hashes?
- And when probing and rehashing of `HashTable` totally in-place, and
eliminting the random read to a the outside `Vec`?
But I am indeed not sure, for the large primitive, if doing this can get
benefit, because the storing them inline `HashTable` will make the table larger
than storing their hashes...
Also comparing them directly is more expansive than comparing their hashes...
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Dandandan commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2101880510
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/large_primitive.rs:
##
@@ -0,0 +1,139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::aggregates::group_values::single_group_by::primitive::{
+emit_internal, HashValue,
+};
+use crate::aggregates::group_values::GroupValues;
+use ahash::RandomState;
+use arrow::array::{
+cast::AsArray, ArrayRef, ArrowNativeTypeOp, ArrowPrimitiveType,
PrimitiveArray,
+};
+use arrow::datatypes::DataType;
+use arrow::record_batch::RecordBatch;
+use datafusion_common::Result;
+use datafusion_execution::memory_pool::proxy::VecAllocExt;
+use datafusion_expr::EmitTo;
+use hashbrown::hash_table::HashTable;
+use std::mem::size_of;
+
+/// A [`GroupValues`] storing a single column of large primitive values (bits
> 64)
+///
+/// This specialization is significantly faster than using the more general
+/// purpose `Row`s format
+pub struct GroupValuesLargePrimitive {
Review Comment:
Hm I think the storage of a value inside `HashTable` will not be very
different from `Vec` besides being inline?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Rachelint commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2101732232
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/large_primitive.rs:
##
@@ -0,0 +1,139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::aggregates::group_values::single_group_by::primitive::{
+emit_internal, HashValue,
+};
+use crate::aggregates::group_values::GroupValues;
+use ahash::RandomState;
+use arrow::array::{
+cast::AsArray, ArrayRef, ArrowNativeTypeOp, ArrowPrimitiveType,
PrimitiveArray,
+};
+use arrow::datatypes::DataType;
+use arrow::record_batch::RecordBatch;
+use datafusion_common::Result;
+use datafusion_execution::memory_pool::proxy::VecAllocExt;
+use datafusion_expr::EmitTo;
+use hashbrown::hash_table::HashTable;
+use std::mem::size_of;
+
+/// A [`GroupValues`] storing a single column of large primitive values (bits
> 64)
+///
+/// This specialization is significantly faster than using the more general
+/// purpose `Row`s format
+pub struct GroupValuesLargePrimitive {
Review Comment:
> Why use large primitive? Is 128 bits slower otherwise?
🤔 I currently decide which type to use `GroupValuesLargePrimitive`,
according to if its bits > 64.
Because the `hash` is 64bits, when bits > 64, the `map` will get larger, and
I am not sure if it is still better to go the new way.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Rachelint commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2101741744
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs:
##
@@ -130,15 +133,15 @@ where
let hash = key.hash(state);
let insert = self.map.entry(
hash,
-|&(g, _)| unsafe {
self.values.get_unchecked(g).is_eq(key) },
Review Comment:
I think for the `large`(> 64bits), we can do that to improve.
But for `normal`(<= 64bits), the new way will be better, I am testing it.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Rachelint commented on PR #16136: URL: https://github.com/apache/datafusion/pull/16136#issuecomment-2900067762 > 🤖: Benchmark completed Thanks, q4 and q15 are the target, and it seems indeed get faster! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Rachelint commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2101735859
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs:
##
@@ -74,21 +77,21 @@ macro_rules! hash_float {
hash_float!(f16, f32, f64);
-/// A [`GroupValues`] storing a single column of primitive values
+/// A [`GroupValues`] storing a single column of normal primitive values (bits
<= 64)
///
/// This specialization is significantly faster than using the more general
/// purpose `Row`s format
pub struct GroupValuesPrimitive {
/// The data type of the output array
data_type: DataType,
-/// Stores the `(group_index, hash)` based on the hash of its value
+/// Stores the `(group_index, group_value)`
///
-/// We also store `hash` is for reducing cost of rehashing. Such cost
-/// is obvious in high cardinality group by situation.
+/// We directly store copy of `group_value` for not only efficient
+/// rehashing, but also efficient probing.
/// More details can see:
-///
+///
///
-map: HashTable<(usize, u64)>,
+map: HashTable<(usize, T::Native)>,
Review Comment:
I want to try it, too.
But I think it will lead to regression, becasue many extra random writes
exist in `emit` if we do that.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Rachelint commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2101735859
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs:
##
@@ -74,21 +77,21 @@ macro_rules! hash_float {
hash_float!(f16, f32, f64);
-/// A [`GroupValues`] storing a single column of primitive values
+/// A [`GroupValues`] storing a single column of normal primitive values (bits
<= 64)
///
/// This specialization is significantly faster than using the more general
/// purpose `Row`s format
pub struct GroupValuesPrimitive {
/// The data type of the output array
data_type: DataType,
-/// Stores the `(group_index, hash)` based on the hash of its value
+/// Stores the `(group_index, group_value)`
///
-/// We also store `hash` is for reducing cost of rehashing. Such cost
-/// is obvious in high cardinality group by situation.
+/// We directly store copy of `group_value` for not only efficient
+/// rehashing, but also efficient probing.
/// More details can see:
-///
+///
///
-map: HashTable<(usize, u64)>,
+map: HashTable<(usize, T::Native)>,
Review Comment:
I want to try it, too.
But I think it will lead to regression, becasue many extra random writes
exist in `emit`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Rachelint commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2101732218
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/large_primitive.rs:
##
@@ -0,0 +1,139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::aggregates::group_values::single_group_by::primitive::{
+emit_internal, HashValue,
+};
+use crate::aggregates::group_values::GroupValues;
+use ahash::RandomState;
+use arrow::array::{
+cast::AsArray, ArrayRef, ArrowNativeTypeOp, ArrowPrimitiveType,
PrimitiveArray,
+};
+use arrow::datatypes::DataType;
+use arrow::record_batch::RecordBatch;
+use datafusion_common::Result;
+use datafusion_execution::memory_pool::proxy::VecAllocExt;
+use datafusion_expr::EmitTo;
+use hashbrown::hash_table::HashTable;
+use std::mem::size_of;
+
+/// A [`GroupValues`] storing a single column of large primitive values (bits
> 64)
+///
+/// This specialization is significantly faster than using the more general
+/// purpose `Row`s format
+pub struct GroupValuesLargePrimitive {
Review Comment:
> Why use large primitive? Is 128 bits slower otherwise?
🤔 I currently decide which type to use `GroupValuesLargePrimitive`,
according to if its bits > 64.
Because the `hash` is 64bits, when bits > 64, the `map` will get larger, and
I am not sure if it is still better to go the new way.
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/large_primitive.rs:
##
@@ -0,0 +1,139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::aggregates::group_values::single_group_by::primitive::{
+emit_internal, HashValue,
+};
+use crate::aggregates::group_values::GroupValues;
+use ahash::RandomState;
+use arrow::array::{
+cast::AsArray, ArrayRef, ArrowNativeTypeOp, ArrowPrimitiveType,
PrimitiveArray,
+};
+use arrow::datatypes::DataType;
+use arrow::record_batch::RecordBatch;
+use datafusion_common::Result;
+use datafusion_execution::memory_pool::proxy::VecAllocExt;
+use datafusion_expr::EmitTo;
+use hashbrown::hash_table::HashTable;
+use std::mem::size_of;
+
+/// A [`GroupValues`] storing a single column of large primitive values (bits
> 64)
+///
+/// This specialization is significantly faster than using the more general
+/// purpose `Row`s format
+pub struct GroupValuesLargePrimitive {
Review Comment:
> Why use large primitive? Is 128 bits slower otherwise?
🤔 I currently decide which type to use `GroupValuesLargePrimitive`,
according to if its bits > 64.
Because the `hash` is 64bits, when bits > 64, the `map` will get larger, and
I am not sure if it is still better to go the new way.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Dandandan commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2100975676
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs:
##
@@ -74,21 +77,21 @@ macro_rules! hash_float {
hash_float!(f16, f32, f64);
-/// A [`GroupValues`] storing a single column of primitive values
+/// A [`GroupValues`] storing a single column of normal primitive values (bits
<= 64)
///
/// This specialization is significantly faster than using the more general
/// purpose `Row`s format
pub struct GroupValuesPrimitive {
/// The data type of the output array
data_type: DataType,
-/// Stores the `(group_index, hash)` based on the hash of its value
+/// Stores the `(group_index, group_value)`
///
-/// We also store `hash` is for reducing cost of rehashing. Such cost
-/// is obvious in high cardinality group by situation.
+/// We directly store copy of `group_value` for not only efficient
+/// rehashing, but also efficient probing.
/// More details can see:
-///
+///
///
-map: HashTable<(usize, u64)>,
+map: HashTable<(usize, T::Native)>,
Review Comment:
Something like:
```diff
diff --git
i/datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs
w/datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs
index 693cc997fa..600287486d 100644
---
i/datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs
+++
w/datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs
@@ -25,7 +25,6 @@ use arrow::array::{
use arrow::datatypes::{i256, DataType};
use arrow::record_batch::RecordBatch;
use datafusion_common::Result;
-use datafusion_execution::memory_pool::proxy::VecAllocExt;
use datafusion_expr::EmitTo;
use half::f16;
use hashbrown::hash_table::HashTable;
@@ -33,7 +32,6 @@ use std::mem::size_of;
use std::sync::Arc;
mod large_primitive;
-pub use large_primitive::GroupValuesLargePrimitive;
/// A trait to allow hashing of floating point numbers
pub(crate) trait HashValue {
@@ -94,8 +92,6 @@ pub struct GroupValuesPrimitive {
map: HashTable<(usize, T::Native)>,
/// The group index of the null value if any
null_group: Option,
-/// The values for each group index
-values: Vec,
/// The random state used to generate hashes
random_state: RandomState,
}
@@ -106,7 +102,6 @@ impl GroupValuesPrimitive {
Self {
data_type,
map: HashTable::with_capacity(128),
-values: Vec::with_capacity(128),
null_group: None,
random_state: Default::default(),
}
@@ -124,13 +119,14 @@ where
for v in cols[0].as_primitive::() {
let group_id = match v {
None => *self.null_group.get_or_insert_with(|| {
-let group_id = self.values.len();
-self.values.push(Default::default());
+let group_id = self.map.len();
group_id
}),
Some(key) => {
let state = &self.random_state;
let hash = key.hash(state);
+let group_id = self.map.len();
+
let insert = self.map.entry(
hash,
|&(_, v)| v.is_eq(key),
@@ -140,10 +136,8 @@ where
match insert {
hashbrown::hash_table::Entry::Occupied(o) =>
o.get().0,
hashbrown::hash_table::Entry::Vacant(v) => {
-let g = self.values.len();
-v.insert((g, key));
-self.values.push(key);
-g
+v.insert((group_id, key));
+group_id
}
}
}
@@ -155,21 +149,19 @@ where
fn size(&self) -> usize {
self.map.capacity() * size_of::<(usize, T::Native)>()
-+ self.values.allocated_size()
}
fn is_empty(&self) -> bool {
-self.values.is_empty()
+self.map.is_empty()
}
fn len(&self) -> usize {
-self.values.len()
+self.map.len()
}
fn emit(&mut self, emit_to: EmitTo) -> Result> {
-emit_internal::(
+emit_internal::(
emit_to,
-&mut self.values,
&mut self.null_group,
&mu
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
alamb commented on PR #16136: URL: https://github.com/apache/datafusion/pull/16136#issuecomment-2898865038 🤖 `./gh_compare_branch.sh` [Benchmark Script](https://github.com/alamb/datafusion-benchmarking/blob/main/gh_compare_branch.sh) Running Linux aal-dev 6.11.0-1013-gcp #13~24.04.1-Ubuntu SMP Wed Apr 2 16:34:16 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux Comparing improve-primitive-group-values (8c05f694d6849a364bfb78400f06b5b4ca14c02c) to 07fe23f03dcb53b2715d3f3b3e0c476d54202c00 [diff](https://github.com/apache/datafusion/compare/07fe23f03dcb53b2715d3f3b3e0c476d54202c00..8c05f694d6849a364bfb78400f06b5b4ca14c02c) Benchmarks: tpch_mem clickbench_partitioned clickbench_extended Results will be posted here when complete -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Dandandan commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2100975676
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs:
##
@@ -74,21 +77,21 @@ macro_rules! hash_float {
hash_float!(f16, f32, f64);
-/// A [`GroupValues`] storing a single column of primitive values
+/// A [`GroupValues`] storing a single column of normal primitive values (bits
<= 64)
///
/// This specialization is significantly faster than using the more general
/// purpose `Row`s format
pub struct GroupValuesPrimitive {
/// The data type of the output array
data_type: DataType,
-/// Stores the `(group_index, hash)` based on the hash of its value
+/// Stores the `(group_index, group_value)`
///
-/// We also store `hash` is for reducing cost of rehashing. Such cost
-/// is obvious in high cardinality group by situation.
+/// We directly store copy of `group_value` for not only efficient
+/// rehashing, but also efficient probing.
/// More details can see:
-///
+///
///
-map: HashTable<(usize, u64)>,
+map: HashTable<(usize, T::Native)>,
Review Comment:
Something like:
```diff
diff --git
i/datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs
w/datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs
index 693cc997fa..c166c16f8e 100644
---
i/datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs
+++
w/datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs
@@ -25,7 +25,6 @@ use arrow::array::{
use arrow::datatypes::{i256, DataType};
use arrow::record_batch::RecordBatch;
use datafusion_common::Result;
-use datafusion_execution::memory_pool::proxy::VecAllocExt;
use datafusion_expr::EmitTo;
use half::f16;
use hashbrown::hash_table::HashTable;
@@ -33,7 +32,6 @@ use std::mem::size_of;
use std::sync::Arc;
mod large_primitive;
-pub use large_primitive::GroupValuesLargePrimitive;
/// A trait to allow hashing of floating point numbers
pub(crate) trait HashValue {
@@ -94,8 +92,6 @@ pub struct GroupValuesPrimitive {
map: HashTable<(usize, T::Native)>,
/// The group index of the null value if any
null_group: Option,
-/// The values for each group index
-values: Vec,
/// The random state used to generate hashes
random_state: RandomState,
}
@@ -106,7 +102,6 @@ impl GroupValuesPrimitive {
Self {
data_type,
map: HashTable::with_capacity(128),
-values: Vec::with_capacity(128),
null_group: None,
random_state: Default::default(),
}
@@ -124,13 +119,14 @@ where
for v in cols[0].as_primitive::() {
let group_id = match v {
None => *self.null_group.get_or_insert_with(|| {
-let group_id = self.values.len();
-self.values.push(Default::default());
+let group_id = self.map.len();
group_id
}),
Some(key) => {
let state = &self.random_state;
let hash = key.hash(state);
+let group_id = self.map.len();
+
let insert = self.map.entry(
hash,
|&(_, v)| v.is_eq(key),
@@ -140,10 +136,8 @@ where
match insert {
hashbrown::hash_table::Entry::Occupied(o) =>
o.get().0,
hashbrown::hash_table::Entry::Vacant(v) => {
-let g = self.values.len();
-v.insert((g, key));
-self.values.push(key);
-g
+v.insert((group_id, key));
+group_id
}
}
}
@@ -155,21 +149,19 @@ where
fn size(&self) -> usize {
self.map.capacity() * size_of::<(usize, T::Native)>()
-+ self.values.allocated_size()
}
fn is_empty(&self) -> bool {
-self.values.is_empty()
+self.map.is_empty()
}
fn len(&self) -> usize {
-self.values.len()
+self.map.len()
}
fn emit(&mut self, emit_to: EmitTo) -> Result> {
-emit_internal::(
+emit_internal::(
emit_to,
-&mut self.values,
&mut self.null_group,
&mu
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
alamb commented on PR #16136: URL: https://github.com/apache/datafusion/pull/16136#issuecomment-2898960798 🤖: Benchmark completed Details ``` Comparing HEAD and improve-primitive-group-values Benchmark clickbench_extended.json ┏━━┳┳┳━━━┓ ┃ Query┃ HEAD ┃ improve-primitive-group-values ┃Change ┃ ┡━━╇╇╇━━━┩ │ QQuery 0 │ 2121.49ms │ 1919.65ms │ +1.11x faster │ │ QQuery 1 │ 740.28ms │ 713.71ms │ no change │ │ QQuery 2 │ 1498.69ms │ 1414.88ms │ +1.06x faster │ │ QQuery 3 │ 695.23ms │ 717.43ms │ no change │ │ QQuery 4 │ 1471.01ms │ 1474.66ms │ no change │ │ QQuery 5 │ 15267.11ms │ 15054.97ms │ no change │ │ QQuery 6 │ 2081.23ms │ 2106.01ms │ no change │ │ QQuery 7 │ 2141.68ms │ 2088.73ms │ no change │ │ QQuery 8 │ 2008.35ms │ 2070.93ms │ no change │ └──┴┴┴───┘ ┏━━━┳┓ ┃ Benchmark Summary ┃┃ ┡━━━╇┩ │ Total Time (HEAD) │ 28025.07ms │ │ Total Time (improve-primitive-group-values) │ 27560.97ms │ │ Average Time (HEAD) │ 3113.90ms │ │ Average Time (improve-primitive-group-values) │ 3062.33ms │ │ Queries Faster│ 2 │ │ Queries Slower│ 0 │ │ Queries with No Change│ 7 │ └───┴┘ Benchmark clickbench_partitioned.json ┏━━┳┳┳━━━┓ ┃ Query┃ HEAD ┃ improve-primitive-group-values ┃Change ┃ ┡━━╇╇╇━━━┩ │ QQuery 0 │ 2.69ms │ 2.18ms │ +1.23x faster │ │ QQuery 1 │37.02ms │37.08ms │ no change │ │ QQuery 2 │88.75ms │93.11ms │ no change │ │ QQuery 3 │98.86ms │ 101.96ms │ no change │ │ QQuery 4 │ 578.77ms │ 546.64ms │ +1.06x faster │ │ QQuery 5 │ 847.22ms │ 889.08ms │ no change │ │ QQuery 6 │ 2.32ms │ 2.25ms │ no change │ │ QQuery 7 │40.97ms │41.98ms │ no change │ │ QQuery 8 │ 897.48ms │ 914.82ms │ no change │ │ QQuery 9 │ 1197.23ms │ 1208.45ms │ no change │ │ QQuery 10│ 271.49ms │ 266.82ms │ no change │ │ QQuery 11│ 302.44ms │ 300.03ms │ no change │ │ QQuery 12│ 890.12ms │ 920.98ms │ no change │ │ QQuery 13│ 1356.21ms │ 1390.50ms │ no change │ │ QQuery 14│ 829.07ms │ 867.85ms │ no change │ │ QQuery 15│ 819.53ms │ 745.34ms │ +1.10x faster │ │ QQuery 16│ 1703.44ms │ 1735.34ms │ no change │ │ QQuery 17│ 1586.44ms │ 1598.57ms │ no change │ │ QQuery 18│ 3046.16ms │ 3127.48ms │ no change │ │ QQuery 19│84.86ms │89.24ms │ 1.05x slower │ │ QQuery 20│ 1154.87ms │ 1172.21ms │ no change │ │ QQuery 21│ 1348.35ms │ 1369.51ms │ no change │ │ QQuery 22│ 2274.86ms │ 2274.24ms │ no change │ │ QQuery 23│ 8334.33ms │ 8396.84ms │ no change │ │ QQuery 24│ 468.77ms │ 471.73ms │ no change │ │ QQuery 25│ 407.86ms │ 400.50ms │ no change │ │ QQuery 26│ 531.96ms │ 536.71ms │ no change │ │ QQuery 27│ 1594.67ms │ 1618.85ms │ no change │ │ QQuery 28│ 12418.63ms │ 12463.37ms │ no change │ │ QQuery 29│ 543.60ms │ 533.05ms │ no change │ │ QQuery 30│ 820.78ms │ 82
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Dandandan commented on code in PR #16136: URL: https://github.com/apache/datafusion/pull/16136#discussion_r2100966267 ## datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs: ## @@ -222,3 +184,61 @@ where self.map.shrink_to(count, |_| 0); // hasher does not matter since the map is cleared } } + +pub(crate) fn emit_internal( Review Comment: K is unneeded here, it can take `emit_internal` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Dandandan commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2100975676
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs:
##
@@ -74,21 +77,21 @@ macro_rules! hash_float {
hash_float!(f16, f32, f64);
-/// A [`GroupValues`] storing a single column of primitive values
+/// A [`GroupValues`] storing a single column of normal primitive values (bits
<= 64)
///
/// This specialization is significantly faster than using the more general
/// purpose `Row`s format
pub struct GroupValuesPrimitive {
/// The data type of the output array
data_type: DataType,
-/// Stores the `(group_index, hash)` based on the hash of its value
+/// Stores the `(group_index, group_value)`
///
-/// We also store `hash` is for reducing cost of rehashing. Such cost
-/// is obvious in high cardinality group by situation.
+/// We directly store copy of `group_value` for not only efficient
+/// rehashing, but also efficient probing.
/// More details can see:
-///
+///
///
-map: HashTable<(usize, u64)>,
+map: HashTable<(usize, T::Native)>,
Review Comment:
Something like:
```diff
diff --git
i/datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs
w/datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs
index 693cc997fa..600287486d 100644
---
i/datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs
+++
w/datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs
@@ -25,7 +25,6 @@ use arrow::array::{
use arrow::datatypes::{i256, DataType};
use arrow::record_batch::RecordBatch;
use datafusion_common::Result;
-use datafusion_execution::memory_pool::proxy::VecAllocExt;
use datafusion_expr::EmitTo;
use half::f16;
use hashbrown::hash_table::HashTable;
@@ -33,7 +32,6 @@ use std::mem::size_of;
use std::sync::Arc;
mod large_primitive;
-pub use large_primitive::GroupValuesLargePrimitive;
/// A trait to allow hashing of floating point numbers
pub(crate) trait HashValue {
@@ -94,8 +92,6 @@ pub struct GroupValuesPrimitive {
map: HashTable<(usize, T::Native)>,
/// The group index of the null value if any
null_group: Option,
-/// The values for each group index
-values: Vec,
/// The random state used to generate hashes
random_state: RandomState,
}
@@ -106,7 +102,6 @@ impl GroupValuesPrimitive {
Self {
data_type,
map: HashTable::with_capacity(128),
-values: Vec::with_capacity(128),
null_group: None,
random_state: Default::default(),
}
@@ -124,13 +119,14 @@ where
for v in cols[0].as_primitive::() {
let group_id = match v {
None => *self.null_group.get_or_insert_with(|| {
-let group_id = self.values.len();
-self.values.push(Default::default());
+let group_id = self.map.len();
group_id
}),
Some(key) => {
let state = &self.random_state;
let hash = key.hash(state);
+let group_id = self.map.len();
+
let insert = self.map.entry(
hash,
|&(_, v)| v.is_eq(key),
@@ -140,10 +136,8 @@ where
match insert {
hashbrown::hash_table::Entry::Occupied(o) =>
o.get().0,
hashbrown::hash_table::Entry::Vacant(v) => {
-let g = self.values.len();
-v.insert((g, key));
-self.values.push(key);
-g
+v.insert((group_id, key));
+group_id
}
}
}
@@ -155,21 +149,19 @@ where
fn size(&self) -> usize {
self.map.capacity() * size_of::<(usize, T::Native)>()
-+ self.values.allocated_size()
}
fn is_empty(&self) -> bool {
-self.values.is_empty()
+self.map.is_empty()
}
fn len(&self) -> usize {
-self.values.len()
+self.map.len()
}
fn emit(&mut self, emit_to: EmitTo) -> Result> {
-emit_internal::(
+emit_internal::(
emit_to,
-&mut self.values,
&mut self.null_group,
&mu
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Dandandan commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2100975676
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs:
##
@@ -74,21 +77,21 @@ macro_rules! hash_float {
hash_float!(f16, f32, f64);
-/// A [`GroupValues`] storing a single column of primitive values
+/// A [`GroupValues`] storing a single column of normal primitive values (bits
<= 64)
///
/// This specialization is significantly faster than using the more general
/// purpose `Row`s format
pub struct GroupValuesPrimitive {
/// The data type of the output array
data_type: DataType,
-/// Stores the `(group_index, hash)` based on the hash of its value
+/// Stores the `(group_index, group_value)`
///
-/// We also store `hash` is for reducing cost of rehashing. Such cost
-/// is obvious in high cardinality group by situation.
+/// We directly store copy of `group_value` for not only efficient
+/// rehashing, but also efficient probing.
/// More details can see:
-///
+///
///
-map: HashTable<(usize, u64)>,
+map: HashTable<(usize, T::Native)>,
Review Comment:
Something like:
```diff
diff --git
i/datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs
w/datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs
index 693cc997fa..600287486d 100644
---
i/datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs
+++
w/datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs
@@ -25,7 +25,6 @@ use arrow::array::{
use arrow::datatypes::{i256, DataType};
use arrow::record_batch::RecordBatch;
use datafusion_common::Result;
-use datafusion_execution::memory_pool::proxy::VecAllocExt;
use datafusion_expr::EmitTo;
use half::f16;
use hashbrown::hash_table::HashTable;
@@ -33,7 +32,6 @@ use std::mem::size_of;
use std::sync::Arc;
mod large_primitive;
-pub use large_primitive::GroupValuesLargePrimitive;
/// A trait to allow hashing of floating point numbers
pub(crate) trait HashValue {
@@ -94,8 +92,6 @@ pub struct GroupValuesPrimitive {
map: HashTable<(usize, T::Native)>,
/// The group index of the null value if any
null_group: Option,
-/// The values for each group index
-values: Vec,
/// The random state used to generate hashes
random_state: RandomState,
}
@@ -106,7 +102,6 @@ impl GroupValuesPrimitive {
Self {
data_type,
map: HashTable::with_capacity(128),
-values: Vec::with_capacity(128),
null_group: None,
random_state: Default::default(),
}
@@ -124,13 +119,14 @@ where
for v in cols[0].as_primitive::() {
let group_id = match v {
None => *self.null_group.get_or_insert_with(|| {
-let group_id = self.values.len();
-self.values.push(Default::default());
+let group_id = self.map.len();
group_id
}),
Some(key) => {
let state = &self.random_state;
let hash = key.hash(state);
+let group_id = self.map.len();
+
let insert = self.map.entry(
hash,
|&(_, v)| v.is_eq(key),
@@ -140,10 +136,8 @@ where
match insert {
hashbrown::hash_table::Entry::Occupied(o) =>
o.get().0,
hashbrown::hash_table::Entry::Vacant(v) => {
-let g = self.values.len();
-v.insert((g, key));
-self.values.push(key);
-g
+v.insert((group_id, key));
+group_id
}
}
}
@@ -155,21 +149,19 @@ where
fn size(&self) -> usize {
self.map.capacity() * size_of::<(usize, T::Native)>()
-+ self.values.allocated_size()
}
fn is_empty(&self) -> bool {
-self.values.is_empty()
+self.map.is_empty()
}
fn len(&self) -> usize {
-self.values.len()
+self.map.len()
}
fn emit(&mut self, emit_to: EmitTo) -> Result> {
-emit_internal::(
+emit_internal::(
emit_to,
-&mut self.values,
&mut self.null_group,
&mu
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Dandandan commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2100768086
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/large_primitive.rs:
##
@@ -0,0 +1,139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::aggregates::group_values::single_group_by::primitive::{
+emit_internal, HashValue,
+};
+use crate::aggregates::group_values::GroupValues;
+use ahash::RandomState;
+use arrow::array::{
+cast::AsArray, ArrayRef, ArrowNativeTypeOp, ArrowPrimitiveType,
PrimitiveArray,
+};
+use arrow::datatypes::DataType;
+use arrow::record_batch::RecordBatch;
+use datafusion_common::Result;
+use datafusion_execution::memory_pool::proxy::VecAllocExt;
+use datafusion_expr::EmitTo;
+use hashbrown::hash_table::HashTable;
+use std::mem::size_of;
+
+/// A [`GroupValues`] storing a single column of large primitive values (bits
> 64)
+///
+/// This specialization is significantly faster than using the more general
+/// purpose `Row`s format
+pub struct GroupValuesLargePrimitive {
Review Comment:
Why use large primitive? Is 128 bits slower otherwise?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Dandandan commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2100764581
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs:
##
@@ -130,15 +133,15 @@ where
let hash = key.hash(state);
let insert = self.map.entry(
hash,
-|&(g, _)| unsafe {
self.values.get_unchecked(g).is_eq(key) },
Review Comment:
I wonder if the existing performance couldn't already be improved by doing
`h == hash && unsafe { self.values.get_unchecked(g).is_eq(key) }` (i.e.
avoiding the fetch if hash equality doesn't hold), like we do in other places.
This already filters out most of them as hashes are almost never the same.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Dandandan commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2100764581
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs:
##
@@ -130,15 +133,15 @@ where
let hash = key.hash(state);
let insert = self.map.entry(
hash,
-|&(g, _)| unsafe {
self.values.get_unchecked(g).is_eq(key) },
Review Comment:
I wonder if the existing performance couldn't already be improved by doing
`h == hash && unsafe { self.values.get_unchecked(g).is_eq(key) }` (i.e.
avoiding fetching the value if hash equality doesn't hold), like we do in other
places. This already filters out most of them as hashes are almost never the
same.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Dandandan commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2100764581
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs:
##
@@ -130,15 +133,15 @@ where
let hash = key.hash(state);
let insert = self.map.entry(
hash,
-|&(g, _)| unsafe {
self.values.get_unchecked(g).is_eq(key) },
Review Comment:
I wonder if the existing performance couldn't already be improved by doing
`h == hash && self.values.get_unchecked(g).is_eq(key)` (i.e. avoiding the fetch
if hash equality doesn't hold), like we do in other places. This already
filters out most of them as hashes are almost never the same.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Dandandan commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2100719962
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs:
##
@@ -74,21 +77,21 @@ macro_rules! hash_float {
hash_float!(f16, f32, f64);
-/// A [`GroupValues`] storing a single column of primitive values
+/// A [`GroupValues`] storing a single column of normal primitive values (bits
<= 64)
///
/// This specialization is significantly faster than using the more general
/// purpose `Row`s format
pub struct GroupValuesPrimitive {
/// The data type of the output array
data_type: DataType,
-/// Stores the `(group_index, hash)` based on the hash of its value
+/// Stores the `(group_index, group_value)`
///
-/// We also store `hash` is for reducing cost of rehashing. Such cost
-/// is obvious in high cardinality group by situation.
+/// We directly store copy of `group_value` for not only efficient
+/// rehashing, but also efficient probing.
/// More details can see:
-///
+///
///
-map: HashTable<(usize, u64)>,
+map: HashTable<(usize, T::Native)>,
Review Comment:
Is storing `values: Vec` in this case necessary?
It could be rebuild in `emit_internal` by traversing the map items I think
(create a `Vec` and update by group index). This avoids doing that while
inserting.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Specialized `GroupValues` for `primitive` and `large_primitive` [datafusion]
Dandandan commented on code in PR #16136:
URL: https://github.com/apache/datafusion/pull/16136#discussion_r2100719962
##
datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive/mod.rs:
##
@@ -74,21 +77,21 @@ macro_rules! hash_float {
hash_float!(f16, f32, f64);
-/// A [`GroupValues`] storing a single column of primitive values
+/// A [`GroupValues`] storing a single column of normal primitive values (bits
<= 64)
///
/// This specialization is significantly faster than using the more general
/// purpose `Row`s format
pub struct GroupValuesPrimitive {
/// The data type of the output array
data_type: DataType,
-/// Stores the `(group_index, hash)` based on the hash of its value
+/// Stores the `(group_index, group_value)`
///
-/// We also store `hash` is for reducing cost of rehashing. Such cost
-/// is obvious in high cardinality group by situation.
+/// We directly store copy of `group_value` for not only efficient
+/// rehashing, but also efficient probing.
/// More details can see:
-///
+///
///
-map: HashTable<(usize, u64)>,
+map: HashTable<(usize, T::Native)>,
Review Comment:
Is storing `values: Vec` in this case necessary?
It could be rebuild in `emit_internal` by traversing the map items I think
(create a `Vec` and update by group index). This avoids doing that while
inserting / updating.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
