gene-bordegaray commented on code in PR #23854:
URL: https://github.com/apache/datafusion/pull/23854#discussion_r3743682884
##########
datafusion/core/tests/physical_optimizer/filter_pushdown.rs:
##########
@@ -1191,6 +1192,256 @@ async fn
test_hashjoin_dynamic_filter_pushdown_partitioned() {
);
}
+#[tokio::test]
+async fn test_hashjoin_dynamic_filter_pushdown_range_partitioned() {
+ use datafusion_common::JoinType;
Review Comment:
since it seems we are repeating imports, would probably be good to add these
to module level, thaknk you 🙇
##########
datafusion/core/tests/physical_optimizer/filter_pushdown.rs:
##########
@@ -943,6 +944,67 @@ async fn
test_topk_filter_passes_through_coalesce_partitions() {
);
}
+fn hashjoin_pushdown_scans() -> (
Review Comment:
really great thank you ❤️
##########
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:
Well I think we can achieve this if we redesign the statement a bit.
Right now we do:
```sql
CASE
WHEN key <range split[0] THEN F0
WHEN key <range split[1] THEN F1
...
ELSE Fn
END
```
and the floating point comparisns here are cuasing us to suffer.
Range partitioning guarentees that a row will be routed to a particular
partition number based on the split points and the row values itself. So we can
modify this statement to reflect that rather that comparing floating points
directly in the case, with someting like this:
```sql
CASE range_partition(key)
WHEN 0 THEN F0
WHEN 1 THEN F1
...
ELSE Fn
END
```
We can extract the code that determines where a row will fall form the
repartition code into a comon helper:
https://github.com/peterxcli/datafusion/blob/18f9db399331ef1c2163c8d5a230b7169d073e50/datafusion/physical-plan/src/repartition/mod.rs#L974-L992
Then we can use something similar to what the Hash case is doing with
`HashExpr` here by adding a `RangeExpr` that will return the partition rows
belong in. Then both the repartition and this code can call it to ensure
behavior is the same.
I don't believe this will have any pruning regression from the prior form
since the page-stats pruner doesnt understand `CASE` statements at all.
For CPU overhead we now have to extract a `ScalarValue` per key in every row
but this was something we did in the repartitioner knowing we can improve in
later iterations as benchmarks showed proof of needing better perf here. We can
file an issue to benchmark this and samply it to find where / how we can
improve. One idea is to turn the keys into Arrow arrays and do this once per
batch.
@jayshrivastava @LiaCastaneda also lmk what you guys this
--
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]
