jayzhan211 commented on code in PR #25386:
URL: https://github.com/apache/datafusion/pull/25386#discussion_r4036831206
##########
benchmarks/sql_benchmarks/null_aware_join/benchmarks/q04.benchmark:
##########
@@ -0,0 +1,18 @@
+name Q04
+group null_aware_join
+
+load sql_benchmarks/null_aware_join/init/load.sql
+
+expect_plan HashJoinExec
Review Comment:
`expect_plan` matches the `{:#?}` output, and `HashJoinExec`'s `Debug` impl
doesn't print `null_aware`, so this guard also passes for a plain
(non-null-aware) join — Q08 on `main` is exactly that. Worth pinning, since the
whole suite is about that flag:
```diff
.field("mode", &self.mode)
+ .field("null_aware", &self.null_aware)
.field("metrics", &self.metrics)
```
then on Q02–Q08:
```diff
-expect_plan HashJoinExec
+expect_plan null_aware: true
```
Fine as a follow-up
##########
benchmarks/sql_benchmarks/null_aware_join/benchmarks/q05.benchmark:
##########
@@ -0,0 +1,18 @@
+name Q05
+group null_aware_join
+
+load sql_benchmarks/null_aware_join/init/load.sql
+
+expect_plan HashJoinExec
+
+run
+-- Q5: non-equality-correlated NOT IN, 1% NULL on the outer side.
+-- With no correlation scope keys the join filter is evaluated over every
+-- (NULL outer row x probe row) pair, so the work scales with the NULL count
+-- times the subquery size. At 1% NULL that product is still small; Q06 is the
+-- same query at 50%.
+SELECT count(*)
+FROM small_outer o
+WHERE o.id_n1 NOT IN (SELECT i.id_n0 FROM small_inner i WHERE i.z < o.z);
Review Comment:
On `main` this suite measures wrong answers for Q05–Q08, so the "base"
column in the table is the cost of skipping the work, not a slower/faster
comparison. Checked against DuckDB at default sizes:
| Query | correct | `main` |
|---|---|---|
| Q05 | 7460 | 7450 |
| Q06 | 5010 | 5000 |
| Q07 | 10 | 0 |
| Q08 | 5530 | 10000 |
`parquet_row_filter_skip` already uses `assert` as a correctness canary for
the same reason. Please add one per query (in #25339 if they need to stay red
on `main` until it lands), and note in this PR's description that the base
numbers for Q05–Q08 come from incorrect results. Example for Q07:
```diff
expect_plan HashJoinExec
+assert I
+SELECT count(*)
+FROM small_outer o
+WHERE o.id_n0 NOT IN (SELECT i.id_n50 FROM small_inner i WHERE i.z < o.z);
+----
+10
+
run
```
Fine as a follow-up. We can revisit the result later to figure out whether
there's a bug in DataFusion, or whether the expected result legitimately
differs from DuckDB.
--
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]
