Copilot commented on code in PR #21820: URL: https://github.com/apache/datafusion/pull/21820#discussion_r3634728246
########## datafusion/common/src/hash_utils/build_hasher.rs: ########## @@ -0,0 +1,489 @@ +// 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 super::{AsDynArray, HASH_BUFFER, MAX_BUFFER_SIZE}; +#[cfg(not(feature = "force_hash_collisions"))] +use super::{ + ChildHashing, combine_hashes, hash_dictionary_with_child_hashing, + hash_fixed_list_array, hash_list_array, hash_list_view_array, hash_map_array, + hash_run_array, hash_struct_array, hash_union_array, +}; +#[cfg(not(feature = "force_hash_collisions"))] +use crate::cast::{ + as_binary_view_array, as_boolean_array, as_fixed_size_list_array, + as_generic_binary_array, as_large_list_array, as_large_list_view_array, + as_list_array, as_list_view_array, as_map_array, as_string_array, + as_string_view_array, as_struct_array, as_union_array, +}; +use crate::error::Result; +use crate::error::{_internal_datafusion_err, _internal_err}; +#[cfg(feature = "force_hash_collisions")] +use arrow::array::Array; +#[cfg(not(feature = "force_hash_collisions"))] +use arrow::array::types::{IntervalDayTime, IntervalMonthDayNano}; +#[cfg(not(feature = "force_hash_collisions"))] +use arrow::array::*; +#[cfg(not(feature = "force_hash_collisions"))] +use arrow::datatypes::*; +#[cfg(not(feature = "force_hash_collisions"))] +use arrow::{downcast_dictionary_array, downcast_primitive_array}; +use std::hash::BuildHasher; + +pub(super) fn with_hashes_with_hasher<I, T, F, R, S>( + arrays: I, + hash_builder: &S, + callback: F, +) -> Result<R> +where + I: IntoIterator<Item = T>, + T: AsDynArray, + F: FnOnce(&[u64]) -> Result<R>, + S: BuildHasher, +{ + let mut iter = arrays.into_iter().peekable(); + + let required_size = match iter.peek() { + Some(arr) => arr.as_dyn_array().len(), + None => return _internal_err!("with_hashes requires at least one array"), + }; Review Comment: The empty-input error message references `with_hashes`, but this code path is for `with_hashes_with_hasher`, which can be confusing when surfaced to users. This issue also appears in the following locations of the same file: - line 65 - line 84 ########## datafusion/common/src/hash_utils/build_hasher.rs: ########## @@ -0,0 +1,489 @@ +// 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 super::{AsDynArray, HASH_BUFFER, MAX_BUFFER_SIZE}; +#[cfg(not(feature = "force_hash_collisions"))] +use super::{ + ChildHashing, combine_hashes, hash_dictionary_with_child_hashing, + hash_fixed_list_array, hash_list_array, hash_list_view_array, hash_map_array, + hash_run_array, hash_struct_array, hash_union_array, +}; +#[cfg(not(feature = "force_hash_collisions"))] +use crate::cast::{ + as_binary_view_array, as_boolean_array, as_fixed_size_list_array, + as_generic_binary_array, as_large_list_array, as_large_list_view_array, + as_list_array, as_list_view_array, as_map_array, as_string_array, + as_string_view_array, as_struct_array, as_union_array, +}; +use crate::error::Result; +use crate::error::{_internal_datafusion_err, _internal_err}; +#[cfg(feature = "force_hash_collisions")] +use arrow::array::Array; +#[cfg(not(feature = "force_hash_collisions"))] +use arrow::array::types::{IntervalDayTime, IntervalMonthDayNano}; +#[cfg(not(feature = "force_hash_collisions"))] +use arrow::array::*; +#[cfg(not(feature = "force_hash_collisions"))] +use arrow::datatypes::*; +#[cfg(not(feature = "force_hash_collisions"))] +use arrow::{downcast_dictionary_array, downcast_primitive_array}; +use std::hash::BuildHasher; + +pub(super) fn with_hashes_with_hasher<I, T, F, R, S>( + arrays: I, + hash_builder: &S, + callback: F, +) -> Result<R> +where + I: IntoIterator<Item = T>, + T: AsDynArray, + F: FnOnce(&[u64]) -> Result<R>, + S: BuildHasher, +{ + let mut iter = arrays.into_iter().peekable(); + + let required_size = match iter.peek() { + Some(arr) => arr.as_dyn_array().len(), + None => return _internal_err!("with_hashes requires at least one array"), + }; + + HASH_BUFFER.try_with(|cell| { + let mut buffer = cell.try_borrow_mut().map_err(|_| { + _internal_datafusion_err!( + "with_hashes cannot be called reentrantly on the same thread" + ) + })?; + + buffer.clear(); + buffer.resize(required_size, 0); + + create_hashes_with_hasher_impl(iter, hash_builder, &mut buffer[..required_size])?; + + let result = callback(&buffer[..required_size])?; + + if buffer.capacity() > MAX_BUFFER_SIZE { + buffer.truncate(MAX_BUFFER_SIZE); + buffer.shrink_to_fit(); + } + + Ok(result) + }).map_err(|_| { + _internal_datafusion_err!( + "with_hashes cannot access thread-local storage during or after thread destruction" + ) + })? +} + +pub(super) fn create_hashes_with_hasher<'a, I, T, S>( + arrays: I, + hash_builder: &S, + hashes_buffer: &'a mut [u64], +) -> Result<&'a mut [u64]> +where + I: IntoIterator<Item = T>, + T: AsDynArray, + S: BuildHasher, +{ + create_hashes_with_hasher_impl(arrays, hash_builder, hashes_buffer) +} + +fn create_hashes_with_hasher_impl<'a, I, T, S>( + arrays: I, + hash_builder: &S, + hashes_buffer: &'a mut [u64], +) -> Result<&'a mut [u64]> +where + I: IntoIterator<Item = T>, + T: AsDynArray, + S: BuildHasher, +{ + for (i, array) in arrays.into_iter().enumerate() { + let rehash = i >= 1; + hash_single_array_with_hasher( + array.as_dyn_array(), + hash_builder, + hashes_buffer, + rehash, + )?; + } + Ok(hashes_buffer) +} + +#[cfg(not(feature = "force_hash_collisions"))] +struct BuildHasherChildHashing<'a, S> { + hash_builder: &'a S, +} + +#[cfg(not(feature = "force_hash_collisions"))] +impl<S: BuildHasher> ChildHashing for BuildHasherChildHashing<'_, S> { + fn create_hashes<I, T>(&self, arrays: I, hashes_buffer: &mut [u64]) -> Result<()> + where + I: IntoIterator<Item = T>, + T: AsDynArray, + { + create_hashes_with_hasher_impl(arrays, self.hash_builder, hashes_buffer) + .map(|_| ()) + } +} + +#[cfg(not(feature = "force_hash_collisions"))] +trait BuildHasherHashValue { + fn hash_one_with_hasher<S: BuildHasher>(&self, state: &S) -> u64; +} + +#[cfg(not(feature = "force_hash_collisions"))] +impl<T: BuildHasherHashValue + ?Sized> BuildHasherHashValue for &T { + fn hash_one_with_hasher<S: BuildHasher>(&self, state: &S) -> u64 { + T::hash_one_with_hasher(self, state) + } +} + +macro_rules! build_hasher_hash_value { + ($($t:ty),+) => { + $(#[cfg(not(feature = "force_hash_collisions"))] + impl BuildHasherHashValue for $t { + fn hash_one_with_hasher<S: BuildHasher>(&self, state: &S) -> u64 { + state.hash_one(self) + } + })+ + }; +} +build_hasher_hash_value!(i8, i16, i32, i64, i128, i256, u8, u16, u32, u64, u128); +build_hasher_hash_value!(bool, str, [u8], IntervalDayTime, IntervalMonthDayNano); + +macro_rules! build_hasher_hash_float_value { + ($(($t:ty, $i:ty)),+) => { + $(#[cfg(not(feature = "force_hash_collisions"))] + impl BuildHasherHashValue for $t { + fn hash_one_with_hasher<S: BuildHasher>(&self, state: &S) -> u64 { + let bits = <$i>::from_ne_bytes(self.to_ne_bytes()); + let bits = if bits << 1 == 0 { 0 } else { bits }; + state.hash_one(bits) + } + })+ + }; +} +build_hasher_hash_float_value!((half::f16, u16), (f32, u32), (f64, u64)); + +#[cfg(not(feature = "force_hash_collisions"))] +fn hash_null_with_hasher<S: BuildHasher>( + hash_builder: &S, + hashes_buffer: &mut [u64], + multi_col: bool, +) { + if multi_col { + hashes_buffer.iter_mut().for_each(|hash| { + *hash = combine_hashes(hash_builder.hash_one(1), *hash); + }) + } else { + hashes_buffer.iter_mut().for_each(|hash| { + *hash = hash_builder.hash_one(1); + }) + } +} Review Comment: `hash_null_with_hasher` recomputes `hash_builder.hash_one(1)` for every row, which unnecessarily rebuilds a hasher N times for a constant value. Compute it once and reuse 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]
