peterxcli commented on code in PR #23854:
URL: https://github.com/apache/datafusion/pull/23854#discussion_r3723522567
##########
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:
I followed how `test_hashjoin_hash_table_pushdown_partitioned`'s pattern
https://github.com/apache/datafusion/blob/18f9db399331ef1c2163c8d5a230b7169d073e50/datafusion/core/tests/physical_optimizer/filter_pushdown.rs#L948-L949
it also import the similar things at the top of test function.
should I also move their `use` to module level, or just follow their
convention?
##########
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:
IMO, because now we're using `CASE WHEN` expression to do the partition
routing, so I'm not sure that 1:1 replicate from
https://github.com/apache/datafusion/blob/18f9db399331ef1c2163c8d5a230b7169d073e50/datafusion/common/src/utils/mod.rs#L119-L146
is practical.
Maybe we should just reject this case in
https://github.com/apache/datafusion/blob/4990a577654f25cb1ed226c03a80208b4a33bc7b/datafusion/physical-plan/src/joins/hash_join/exec.rs#L860
--
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]
