Skip to content

WIP Optimize hash_aggregate when there are no null group keys #922

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.

Sign up for GitHub

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

Closed
novemberkilo wants to merge 2 commits into from
Closed

WIP Optimize hash_aggregate when there are no null group keys #922

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
857bd84
Interim commit for feedback.
novemberkilo Aug 22, 2021
ffb11e0
Follow up commit for feedback.
novemberkilo Aug 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions datafusion/src/physical_plan/hash_aggregate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,10 +365,16 @@ fn group_aggregate_batch(
// actually the same key value as the group in
// existing_idx (aka group_values @ row)
let group_state = &group_states[*group_idx];
group_values
.iter()
.zip(group_state.group_by_values.iter())
.all(|(array, scalar)| scalar.eq_array(array, row))
group_values
.iter()
.zip(group_state.group_by_values.iter())
.all(|(array, scalar)| {
if array.null_count() > 0 {
scalar.eq_array(array, row)
Comment thread
novemberkilo marked this conversation as resolved.
Outdated
} else {
scalar.eq_array_no_nulls(array, row)
}
})
});

match entry {
Expand Down
82 changes: 82 additions & 0 deletions datafusion/src/scalar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,16 @@ macro_rules! eq_array_primitive {
}};
}

macro_rules! eq_array_no_nulls_primitive {
($array:expr, $index:expr, $ARRAYTYPE:ident, $VALUE:expr) => {{
let array = $array.as_any().downcast_ref::<$ARRAYTYPE>().unwrap();
match $VALUE {
Some(val) => &array.value($index) == val,
None => false,
}
}};
}

impl ScalarValue {
/// Getter for the `DataType` of the value
pub fn get_datatype(&self) -> DataType {
Expand Down Expand Up @@ -1095,6 +1105,78 @@ impl ScalarValue {
}
}


/// an unsafe version of `eq_array` optimised for the situation
/// where we know in advance that the array has no nulls
#[inline]
pub fn eq_array_no_nulls(&self, array: &ArrayRef, index:usize) -> bool {
if let DataType::Dictionary(key_type, _) = array.data_type() {
return self.eq_array_dictionary(array, index, key_type);

@alamb alamb Aug 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should perhaps also be self.eq_array_dictionary_no_nulls and have a corresponding dictionary special case.

}

match self {
ScalarValue::Boolean(val) => {
eq_array_no_nulls_primitive!(array, index, BooleanArray, val)
}
ScalarValue::Float32(val) => {
eq_array_no_nulls_primitive!(array, index, Float32Array, val)
}
ScalarValue::Float64(val) => {
eq_array_no_nulls_primitive!(array, index, Float64Array, val)
}
ScalarValue::Int8(val) => eq_array_no_nulls_primitive!(array, index, Int8Array, val),
ScalarValue::Int16(val) => eq_array_no_nulls_primitive!(array, index, Int16Array, val),
ScalarValue::Int32(val) => eq_array_no_nulls_primitive!(array, index, Int32Array, val),
ScalarValue::Int64(val) => eq_array_no_nulls_primitive!(array, index, Int64Array, val),
ScalarValue::UInt8(val) => eq_array_no_nulls_primitive!(array, index, UInt8Array, val),
ScalarValue::UInt16(val) => {
eq_array_no_nulls_primitive!(array, index, UInt16Array, val)
}
ScalarValue::UInt32(val) => {
eq_array_no_nulls_primitive!(array, index, UInt32Array, val)
}
ScalarValue::UInt64(val) => {
eq_array_no_nulls_primitive!(array, index, UInt64Array, val)
}
ScalarValue::Utf8(val) => eq_array_no_nulls_primitive!(array, index, StringArray, val),
ScalarValue::LargeUtf8(val) => {
eq_array_no_nulls_primitive!(array, index, LargeStringArray, val)
}
ScalarValue::Binary(val) => {
eq_array_no_nulls_primitive!(array, index, BinaryArray, val)
}
ScalarValue::LargeBinary(val) => {
eq_array_no_nulls_primitive!(array, index, LargeBinaryArray, val)
}
ScalarValue::List(_, _) => unimplemented!(),
ScalarValue::Date32(val) => {
eq_array_no_nulls_primitive!(array, index, Date32Array, val)
}
ScalarValue::Date64(val) => {
eq_array_no_nulls_primitive!(array, index, Date64Array, val)
}
ScalarValue::TimestampSecond(val) => {
eq_array_no_nulls_primitive!(array, index, TimestampSecondArray, val)
}
ScalarValue::TimestampMillisecond(val) => {
eq_array_no_nulls_primitive!(array, index, TimestampMillisecondArray, val)
}
ScalarValue::TimestampMicrosecond(val) => {
eq_array_no_nulls_primitive!(array, index, TimestampMicrosecondArray, val)
}
ScalarValue::TimestampNanosecond(val) => {
eq_array_no_nulls_primitive!(array, index, TimestampNanosecondArray, val)
}
ScalarValue::IntervalYearMonth(val) => {
eq_array_no_nulls_primitive!(array, index, IntervalYearMonthArray, val)
}
ScalarValue::IntervalDayTime(val) => {
eq_array_no_nulls_primitive!(array, index, IntervalDayTimeArray, val)
}
}
}


/// Compares a dictionary array with indexes of type `key_type`
/// with the array @ index for equality with self
fn eq_array_dictionary(
Expand Down