dwsmith1983 commented on code in PR #26228:
URL: https://github.com/apache/datafusion/pull/26228#discussion_r4385712781
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -5014,6 +4998,123 @@ mod tests {
TestMemoryExec::try_new_exec(&[vec![batch.clone(), batch]], schema,
None).unwrap()
}
+ /// Unmatched build rows of a `LeftAnti` join are emitted in build index
+ /// order, so the concatenated build batch must keep the input batch order.
Review Comment:
Done in 4ae0b04bc with your wording. I also ran it against main: it fails on
all five hash map configs and passes on the array map ones, so it is still the
regression test for #26226.
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -5014,6 +4998,123 @@ mod tests {
TestMemoryExec::try_new_exec(&[vec![batch.clone(), batch]], schema,
None).unwrap()
}
+ /// Unmatched build rows of a `LeftAnti` join are emitted in build index
+ /// order, so the concatenated build batch must keep the input batch order.
+ #[apply(hash_join_exec_configs)]
+ #[tokio::test]
+ async fn join_left_anti_keeps_build_order_across_batches(
+ batch_size: usize,
+ use_perfect_hash_join_as_possible: bool,
+ ) -> Result<()> {
+ let task_ctx = prepare_task_ctx(batch_size,
use_perfect_hash_join_as_possible);
+ let batch1 = build_table_i32(
+ ("a1", &vec![1, 2]),
+ ("b1", &vec![10, 20]),
+ ("c1", &vec![7, 8]),
+ );
+ let batch2 = build_table_i32(
+ ("a1", &vec![3, 4]),
+ ("b1", &vec![3000, 4000]),
+ ("c1", &vec![9, 10]),
+ );
+ let schema = batch1.schema();
+ let left =
+ TestMemoryExec::try_new_exec(&[vec![batch1, batch2]], schema,
None).unwrap();
+ let right = build_table(("a2", &vec![1]), ("b1", &vec![1]), ("c2",
&vec![1]));
+ let on = vec![(
+ Arc::new(Column::new_with_schema("b1", &left.schema())?) as _,
+ Arc::new(Column::new_with_schema("b1", &right.schema())?) as _,
+ )];
+
+ let (columns, batches, metrics) = join_collect(
+ left,
+ right,
+ on,
+ &JoinType::LeftAnti,
+ NullEquality::NullEqualsNothing,
+ task_ctx,
+ )
+ .await?;
+
+ assert_eq!(columns, vec!["a1", "b1", "c1"]);
+ allow_duplicates! {
+ assert_snapshot!(batches_to_string(&batches), @r"
+ +----+------+----+
+ | a1 | b1 | c1 |
+ +----+------+----+
+ | 1 | 10 | 7 |
+ | 2 | 20 | 8 |
+ | 3 | 3000 | 9 |
+ | 4 | 4000 | 10 |
+ +----+------+----+
+ ");
+ }
+ assert_phj_used(&metrics, use_perfect_hash_join_as_possible);
+
+ Ok(())
+ }
+
+ /// Build rows sharing a key across build batches are matched in build
+ /// order, which relies on the hash map chains putting smaller build
+ /// indices first.
+ #[apply(hash_join_exec_configs)]
+ #[tokio::test]
+ async fn join_inner_keeps_build_order_for_duplicate_keys_across_batches(
Review Comment:
Agreed. I removed it and parameterized
`join_inner_one_two_parts_left_randomly_ordered` with `hash_join_exec_configs`
instead. You were right that the dropped test passed on main as well: the
reverse walk with matching offsets already kept chain order, and only the scan
by build index was off.
##########
datafusion/physical-plan/src/joins/hash_join/exec/prepared/tests.rs:
##########
@@ -689,6 +689,56 @@ async fn prepared_build_matches_collect_left() ->
Result<()> {
Ok(())
}
+/// A prepared hash-map build keeps its rows in input batch order, and each
+/// hash-map index still points at the row with its key.
+#[tokio::test]
+async fn prepared_build_keeps_build_order_across_batches() -> Result<()> {
Review Comment:
Removed. The `[None, 0, 0, -4096, 4096]` case in
`prepared_build_matches_collect_left` covers it, thanks.
--
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]
