jayshrivastava commented on code in PR #23854:
URL: https://github.com/apache/datafusion/pull/23854#discussion_r3715199119
##########
datafusion/core/tests/physical_optimizer/filter_pushdown.rs:
##########
@@ -1191,6 +1192,260 @@ async fn
test_hashjoin_dynamic_filter_pushdown_partitioned() {
);
}
+// Not portable to sqllogictest: this test pins `PartitionMode::Partitioned`
+// by hand-wiring matching Range repartitioning on both join sides, which the
+// SQL planner does not currently produce.
+#[tokio::test]
Review Comment:
I don't think you need this comment. It's totally fine to construct the plan
manually. The Hash partitioned test in this file does the same.
For the record, I think there should be a way to get range partitioning
working in sqllogictest? See here:
https://github.com/apache/datafusion/blob/096012e08467ca2457b6a3ebe115bc9229005ef1/datafusion/sqllogictest/test_files/range_partitioning.slt?plain=1#L1
##########
datafusion/physical-plan/src/joins/hash_join/shared_bounds.rs:
##########
@@ -1137,6 +1138,101 @@ mod tests {
Ok(())
}
+ #[test]
+ fn partitioned_range_dynamic_filter_routes_compound_nullable_keys() ->
Result<()> {
+ let probe_schema = Arc::new(Schema::new(vec![
+ Field::new("probe_key", DataType::Int32, true),
+ Field::new("probe_tie", DataType::Int32, true),
+ ]));
+ let on_right: Vec<PhysicalExprRef> = vec![
+ Arc::new(Column::new("probe_key", 0)),
+ Arc::new(Column::new("probe_tie", 1)),
+ ];
+ let mut acc = make_accumulator_for_test(
+ AccumulatedBuildData::Partitioned {
+ partitions: vec![PartitionStatus::Pending; 4],
+ completed_partitions: 0,
+ },
+ on_right,
+ );
+ acc.probe_schema = Arc::clone(&probe_schema);
+ acc.probe_range_partitioning = Some(RangePartitioning::try_new(
+ [
+ PhysicalSortExpr::new(
+ Arc::clone(&acc.on_right[0]),
+ SortOptions::new(false, true),
+ ),
+ PhysicalSortExpr::new(
+ Arc::clone(&acc.on_right[1]),
+ SortOptions::new(false, false),
+ ),
+ ]
+ .into(),
+ vec![
+ SplitPoint::new(vec![
+ ScalarValue::Int32(None),
+ ScalarValue::Int32(Some(10)),
+ ]),
+ SplitPoint::new(vec![ScalarValue::Int32(None),
ScalarValue::Int32(None)]),
+ SplitPoint::new(vec![
+ ScalarValue::Int32(Some(10)),
+ ScalarValue::Int32(None),
+ ]),
+ ],
+ )?);
+
+ acc.build_filter(FinalizeInput::Partitioned(vec![
+ reported(PushdownStrategy::Empty, no_bounds()),
+ PartitionStatus::CanceledUnknown,
+ reported(PushdownStrategy::Empty, no_bounds()),
+ PartitionStatus::CanceledUnknown,
+ ]))?;
+
+ let expr = current_expr(&acc);
+ let case = case_expr(&expr);
+ assert!(case.expr().is_none());
+ assert_eq!(case.when_then_expr().len(), 3);
+
+ let batch = RecordBatch::try_new(
+ probe_schema,
+ vec![
+ Arc::new(Int32Array::from(vec![
+ None,
+ None,
+ None,
+ None,
+ Some(9),
+ Some(10),
+ Some(10),
+ Some(11),
+ ])),
+ Arc::new(Int32Array::from(vec![
+ Some(9),
+ Some(10),
+ Some(11),
+ None,
+ None,
+ Some(9),
+ None,
+ None,
+ ])),
Review Comment:
Nice.
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -7093,11 +7093,28 @@ mod tests {
)?;
assert!(join.allow_join_dynamic_filter_pushdown(session_config.options()));
+
+ let hash_join = join
Review Comment:
nit: change the test name to not be so specific to range partitioning
##########
datafusion/core/tests/physical_optimizer/filter_pushdown.rs:
##########
@@ -1191,6 +1192,260 @@ async fn
test_hashjoin_dynamic_filter_pushdown_partitioned() {
);
}
+// Not portable to sqllogictest: this test pins `PartitionMode::Partitioned`
+// by hand-wiring matching Range repartitioning on both join sides, which the
+// SQL planner does not currently produce.
+#[tokio::test]
+async fn test_hashjoin_dynamic_filter_pushdown_range_partitioned() {
+ use datafusion_common::JoinType;
+ use datafusion_physical_plan::joins::{HashJoinExec, PartitionMode};
+
+ // Rough sketch of the Range-partitioned MRE we're trying to recreate. The
+ // test hand-wires identical Range repartitioning because SQL planning does
+ // not currently derive the split points:
Review Comment:
I don't think we need "because SQL planning does not currently derive the
split points:" in this comment
Great test though!
--
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]
