jayzhan211 commented on code in PR #27111:
URL: https://github.com/apache/datafusion/pull/27111#discussion_r4480603481
##########
datafusion/physical-plan/src/joins/hash_join/not_in.rs:
##########
@@ -99,10 +100,22 @@ impl NotIn {
join_type: JoinType,
partition_mode: PartitionMode,
num_keys: usize,
- num_value_keys: usize,
+ num_value_keys: Option<NonZeroUsize>,
has_filter: bool,
) -> Result<Self> {
- if num_value_keys == 0 || num_value_keys > num_keys {
+ // An unset count is only unambiguous with a single key: a scalar
+ // `NOT IN`. With more keys, any default would silently pick which of
+ // them are `NOT IN` value keys and which are correlation scope keys.
+ let num_value_keys = match num_value_keys {
+ Some(num_value_keys) => num_value_keys.get(),
+ None if num_keys == 1 => 1,
+ None => {
Review Comment:
+1 to @kumarUjjawal. I confirmed it with a probe: a null-aware Inner join
and a Partitioned RightAnti both ask for `with_null_aware_value_keys`, and
setting it only leads to the join-type error. An empty `on` gets "join with 0
keys needs…". Checking the join type and mode before resolving the count, and
checking `on` before `not_in()`, gives the right cause in all three cases, and
all `hash_join` tests still pass:
```diff
--- a/datafusion/physical-plan/src/joins/hash_join/exec.rs
+++ b/datafusion/physical-plan/src/joins/hash_join/exec.rs
@@ impl HashJoinExecBuilder
+ if exec.on.is_empty() {
+ return plan_err!("On constraints in HashJoinExec should be
non-empty");
+ }
// Validate null_aware flag
exec.not_in()?;
--- a/datafusion/physical-plan/src/joins/hash_join/not_in.rs
+++ b/datafusion/physical-plan/src/joins/hash_join/not_in.rs
@@ impl NotIn
) -> Result<Self> {
+ let output = match (join_type, partition_mode) {
+ (JoinType::RightAnti, PartitionMode::CollectLeft) => None,
+ (JoinType::LeftAnti | JoinType::LeftMark,
PartitionMode::Partitioned) => {
+ return plan_err!(
+ "null_aware joins require PartitionMode::CollectLeft,
got PartitionMode::Partitioned"
+ );
+ }
+ (JoinType::LeftAnti, _) => Some(NotInOutput::Rows),
+ (JoinType::LeftMark, _) => Some(NotInOutput::Mark),
+ _ => {
+ return plan_err!(
+ "null_aware can only be true for LeftAnti joins and
RightAnti joins with `CollectLeft` `PartitionMode`, or LeftMark joins, got
{join_type} with {partition_mode}"
+ );
+ }
+ };
// An unset count is only unambiguous with a single key: ...
...
- let output = match (join_type, partition_mode) { ... };
- if partition_mode == PartitionMode::Partitioned { ... }
- Ok(Self::BuildRows { scope, output })
+ Ok(match output {
+ Some(output) => Self::BuildRows { scope, output },
+ None => Self::ProbeRows { scope },
+ })
```
##########
datafusion/optimizer/src/decorrelate_predicate_subquery.rs:
##########
Review Comment:
+1 to @kumarUjjawal. One addition: for single-column `IN` and `EXISTS` mark
joins this split is new in this PR. Before, both decorrelation and
`Join::try_new` stored `1`; now decorrelation stores `Some(1)` and `try_new`
stores `None`. Setting the count only when `null_aware` is true keeps `None` as
the single canonical form.
--
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]
