Skip to content

Commit b63bb3e

Browse files
adriangbclaude
andcommitted
Fix RandomState mismatch in progressive hash join filtering
This commit addresses a critical correctness issue identified in PR review where the hash function used in filter expressions had different RandomState seeds than the RepartitionExec operator, potentially causing incorrect partition assignments and filtering. ## Key Fixes **RandomState Consistency**: - Use RepartitionExec's RandomState (0,0,0,0) instead of custom seeds ('H','A','S','H') - Ensures `hash(row) % num_partitions` produces same partition assignments as actual partitioning - Prevents false negatives in progressive filtering logic **Performance Optimization**: - Cache ConfigOptions in SharedBoundsAccumulator to avoid repeated allocations - Single Arc<ConfigOptions> shared across all hash expression creations **Code Quality**: - Improved comment clarity replacing outdated "HEAD" references - Better documentation of deduplication logic ## Why This Matters Without matching RandomState seeds, our progressive filter expressions could: - Incorrectly filter out valid join candidates (correctness issue) - Allow through data that doesn't belong to a partition (performance issue) - Break the fundamental assumption that hash-based filtering matches partitioning Resolves: apache#17632 (comment) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent fadb4f6 commit b63bb3e

2 files changed

Lines changed: 18 additions & 5 deletions

File tree

datafusion/physical-plan/src/joins/hash_join/shared_bounds.rs

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,12 @@ use datafusion_physical_expr::{PhysicalExpr, PhysicalExprRef, ScalarFunctionExpr
3737
use itertools::Itertools;
3838
use parking_lot::Mutex;
3939
use std::collections::HashSet;
40+
use ahash::RandomState;
41+
42+
/// RandomState used by RepartitionExec for consistent hash partitioning
43+
/// This must match the seeds used in RepartitionExec to ensure our hash-based
44+
/// filter expressions compute the same partition assignments as the actual partitioning
45+
const REPARTITION_RANDOM_STATE: RandomState = RandomState::with_seeds(0, 0, 0, 0);
4046

4147
/// Represents the minimum and maximum values for a specific column.
4248
/// Used in dynamic filter pushdown to establish value boundaries.
@@ -113,6 +119,8 @@ pub(crate) struct SharedBoundsAccumulator {
113119
dynamic_filter: Arc<DynamicFilterPhysicalExpr>,
114120
/// Right side join expressions needed for creating filter bounds
115121
on_right: Vec<PhysicalExprRef>,
122+
/// Cached ConfigOptions to avoid repeated allocations
123+
config_options: Arc<ConfigOptions>,
116124
}
117125

118126
/// State protected by SharedBoundsAccumulator's mutex
@@ -182,21 +190,26 @@ impl SharedBoundsAccumulator {
182190
total_partitions,
183191
dynamic_filter,
184192
on_right,
193+
config_options: Arc::new(ConfigOptions::default()),
185194
}
186195
}
187196

188197
/// Create hash expression for the join keys: hash(col1, col2, ...)
189198
fn create_hash_expression(&self) -> Result<Arc<dyn PhysicalExpr>> {
190-
// Use the hash function with the same random state as hash joins for consistency
191-
let hash_udf = Arc::new(ScalarUDF::from(Hash::new()));
199+
// Use the same random state as RepartitionExec for consistent partitioning
200+
// This ensures hash(row) % num_partitions produces the same partition assignment
201+
// as the original repartitioning operation
202+
let hash_udf = Arc::new(ScalarUDF::from(Hash::new_with_random_state(
203+
REPARTITION_RANDOM_STATE,
204+
)));
192205

193206
// Create the hash expression using ScalarFunctionExpr
194207
Ok(Arc::new(ScalarFunctionExpr::new(
195208
"hash",
196209
hash_udf,
197210
self.on_right.clone(),
198211
Field::new("hash_result", DataType::UInt64, false).into(),
199-
Arc::new(ConfigOptions::default()),
212+
Arc::clone(&self.config_options),
200213
)))
201214
}
202215

@@ -355,7 +368,7 @@ impl SharedBoundsAccumulator {
355368
) -> Result<()> {
356369
let mut inner = self.inner.lock();
357370

358-
// Skip if this partition already reported (using improved deduplication logic from HEAD)
371+
// Skip processing if this partition has already reported its bounds to prevent duplicate updates
359372
if inner.completed_partitions.contains(&left_side_partition_id) {
360373
return Ok(());
361374
}

0 commit comments

Comments
 (0)