jayshrivastava commented on code in PR #23854:
URL: https://github.com/apache/datafusion/pull/23854#discussion_r3750150996
##########
datafusion/physical-plan/src/joins/hash_join/shared_bounds.rs:
##########
@@ -656,38 +661,99 @@ impl SharedBuildAccumulator {
}
}
- let filter_expr = if has_canceled_unknown {
- let mut when_then_branches = empty_partition_ids
- .into_iter()
- .map(|partition_id| {
- (
- lit(ScalarValue::UInt64(Some(partition_id as
u64))),
- lit(false),
- )
- })
- .collect::<Vec<_>>();
- when_then_branches.extend(real_branches);
-
- if when_then_branches.is_empty() {
- lit(true)
- } else {
- Arc::new(CaseExpr::try_new(
- Some(modulo_expr),
- when_then_branches,
- Some(lit(true)),
- )?) as Arc<dyn PhysicalExpr>
- }
- } else if real_branches.is_empty() {
+ let filter_expr = if has_canceled_unknown
+ && real_partition_ids.is_empty()
+ && empty_partition_ids.is_empty()
+ {
+ lit(true)
+ } else if !has_canceled_unknown &&
real_partition_ids.is_empty() {
lit(false)
- } else if real_branches.len() == 1
+ } else if !has_canceled_unknown
+ && real_partition_ids.len() == 1
&& empty_partition_ids.len() + 1 == num_partitions
{
- Arc::clone(&real_branches[0].1)
+ Arc::clone(&partition_filters[real_partition_ids[0]])
+ } else if let Some(range_partitioning) =
&self.probe_range_partitioning {
+ // Range partitioning
+ assert_eq!(
+ partition_filters.len(),
+ range_partitioning.partition_count()
+ );
+ assert_eq!(self.on_right.len(),
range_partitioning.ordering().len());
+ let sort_exprs = self
+ .on_right
+ .iter()
+ .zip(range_partitioning.ordering())
+ .map(|(expr, sort_expr)| {
+ PhysicalSortExpr::new(Arc::clone(expr),
sort_expr.options)
+ })
+ .collect::<Vec<_>>();
+ let else_expr = partition_filters
+ .pop()
+ .expect("Range partitioning always has at least one
partition");
+ // CASE evaluates in order
+ //
+ // CASE
+ // WHEN key <range split[0] THEN F0
+ // WHEN key <range split[1] THEN F1
+ // ...
+ // ELSE Fn
+ // END
+ let when_then_expr = range_partitioning
+ .split_points()
+ .iter()
+ .zip(partition_filters)
+ .map(|(split_point, then_expr)| {
+ let when_expr = build_lexicographic_filter(
Review Comment:
In the interest of time, should we just merge and file an issue to track the
refactor? The refactor would be a non-breaking change.
> The original idea behind using WHEN key < range_split[0...n-2] was to
avoid adding a new UDF, with the expectation that we could later optimize CASE
WHEN with a binary search on this kind of "monotonic function."
FWIW, I think this can be optimized with binary search as well
```
CASE range_partition(key)
WHEN 0 THEN F0
WHEN 1 THEN F1
...
ELSE Fn
END
```
`range_partition(key)` needs to be implemented as a `PhysicalExpr` like hash
is here:
https://github.com/apache/datafusion/blob/1ab576e95e55f72e562b33b70a09478bfffd79a4/datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs?plain=1#L158
--
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]
