kumarUjjawal commented on code in PR #27111:
URL: https://github.com/apache/datafusion/pull/27111#discussion_r4477116913
##########
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:
The new unset-count check runs before the join-type/partition-mode check in
`NotIn::try_new`, and `build()` calls `not_in()` before its `on.is_empty()`
check, so invalid joins report the wrong cause and point to a fix that cannot
help.
Failure scenario: A null-aware Inner join with 2 keys, or a null-aware
RightAnti in Partitioned mode, now fails with 'set it with
`with_null_aware_value_keys`'. Setting the count only leads to a second error,
'null_aware can only be true for LeftAnti...'. A null-aware join with empty
`on` reports 'join with 0 keys needs the number of its `NOT IN` value keys'
instead of 'On constraints in HashJoinExec should be non-empty', and no count
can satisfy it.
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -1057,8 +1058,29 @@ impl AsLogicalPlan for LogicalPlanNode {
// them silently loses both fields. Both sides of the round
// trip should already have validated keys, so we don't need
// the builder's normalization / equijoin-pair checks.
- Ok(LogicalPlan::Join(
- Join::try_new(
+ //
+ // The value key count is presence-tracked: an absent count
+ // (including every plan encoded before the field existed,
+ // which can only hold a scalar `NOT IN`) stays unset, and
+ // physical planning accepts that only for a single key.
+ let null_aware_value_keys = join
Review Comment:
The decode logic 'wire u32 -> usize -> NonZeroUsize, reject 0' is duplicated
in the logical and physical proto decoders, with different error kinds and
messages.
Failure scenario: The logical decoder returns
`proto_error("...null_aware_value_keys 0, it must be at least 1")`, while
`exec.rs:2302` returns `plan_datafusion_err!("...must be at least 1, got 0")`.
Callers matching on error kind see different behaviour for the same malformed
input. A small shared helper such as `non_zero_usize_from_wire` next to
`usize_from_wire` would remove both copies.
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -2171,12 +2179,15 @@ impl ExecutionPlan for HashJoinExec {
Some(v) => v.iter().map(|x| *x as u32).collect(),
},
null_aware: *null_aware,
- null_aware_value_keys:
u32::try_from(*null_aware_value_keys)
- .map_err(|_| {
- internal_datafusion_err!(
- "HashJoinExec: null_aware_value_keys
{null_aware_value_keys} does not fit in u32"
- )
- })?,
+ null_aware_value_keys: null_aware_value_keys
Review Comment:
The encode paths hand-roll
`u32::try_from(..).map_err(internal_datafusion_err!)` instead of the existing
`datafusion_common::utils::usize_to_wire` helper, which is the counterpart of
the `usize_from_wire` used on decode.
Failure scenario: The same conversion is duplicated in
`HashJoinExec::try_to_proto` and
`datafusion/proto/src/logical_plan/mod.rs:1823`. Each copy has its own wording
and error kind (internal vs plan), and they will drift from the shared helper.
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -1892,20 +1892,22 @@ impl DefaultPhysicalPlanner {
PartitionMode::CollectLeft
};
- Arc::new(
- HashJoinExecBuilder::new(
- physical_left,
- physical_right,
- join_on,
- *join_type,
- )
- .with_filter(join_filter)
- .with_partition_mode(partition_mode)
- .with_null_equality(*null_equality)
- .with_null_aware(*null_aware)
- .with_null_aware_value_keys(*null_aware_value_keys)
- .build()?,
+ let mut builder = HashJoinExecBuilder::new(
+ physical_left,
+ physical_right,
+ join_on,
+ *join_type,
)
+ .with_filter(join_filter)
+ .with_partition_mode(partition_mode)
+ .with_null_equality(*null_equality)
+ .with_null_aware(*null_aware);
+ // An unset count is passed on unset: the build accepts it
+ // only for a single key, a scalar `NOT IN`.
+ if let Some(value_keys) = null_aware_value_keys {
Review Comment:
Passing an optional count needs a `let mut builder` plus `if let Some(v) {
builder = builder.with_null_aware_value_keys(v) }`, repeated in the physical
planner and the physical proto decoder, because the setter only takes a
`NonZeroUsize`.
Failure scenario: Two call sites rebuild the builder chain imperatively, and
`eliminate_join` and the logical proto decoder switch to struct-update syntax
(`Join { null_aware_value_keys, ..Join::try_new(..)? }`) for the same reason. A
setter that takes `Option<NonZeroUsize>` (or a `set_null_aware_value_keys`) on
both `HashJoinExecBuilder` and `Join` would keep all four as one fluent chain.
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -10093,6 +10151,35 @@ mod tests {
);
}
+ /// [`HashJoinExec::try_new`] for a scalar `NOT IN` correlated by any
+ /// keys after the first: a null-aware join gets one `NOT IN` value key,
+ /// which `try_new` cannot set and a multi-key join must state.
+ #[expect(clippy::too_many_arguments)]
+ fn try_new_scalar_not_in(
Review Comment:
The test helper `try_new_scalar_not_in` copies the whole body of
`HashJoinExec::try_new`, including `#[expect(clippy::too_many_arguments)]`,
just to add the count, and its name implies a NOT IN even when `null_aware` is
false.
Failure scenario: This is a second 9-argument constructor to keep in sync
with `try_new`/`HashJoinExecBuilder`; a future builder option added to one will
be missed in the other. Call sites could use `HashJoinExecBuilder` directly, or
the helper could take only the arguments that vary.
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -10833,11 +10920,12 @@ mod tests {
Ok(())
}
- /// The value key count must name at least one key and no more than the
- /// join has.
+ /// The value key count must name no more keys than the join has (it
+ /// names at least one by its type).
#[tokio::test]
async fn test_null_aware_validation_value_keys() -> Result<()> {
- for value_keys in [0, 3] {
+ {
Review Comment:
`test_null_aware_validation_value_keys` keeps an empty bare block `{ let
value_keys = NonZeroUsize::new(3).unwrap(); ... }` where the `for value_keys in
[0, 3]` loop used to be.
Failure scenario: The extra nesting and indentation serve no purpose and
read like a leftover loop. Removing the block, with `value_keys` bound at
function scope, does the same job.
##########
datafusion/optimizer/src/decorrelate_predicate_subquery.rs:
##########
Review Comment:
https://github.com/apache/datafusion/blob/ddbce35ef4caec8587c5ccf7e40311e8f5b5ca4e/datafusion/optimizer/src/decorrelate_predicate_subquery.rs#L1230
The count is set on every decorrelated mark join, including ones that are
not null-aware, while all other construction paths now default to None, so
plans that mean the same thing compare unequal.
Failure scenario: A non-null-aware multi-column IN mark join from
decorrelation carries Some(2), while the same join built via `Join::try_new` or
the builder carries None. The field is part of the derived PartialEq/Hash, so
plan comparison, dedup or common-subexpression checks treat them as different.
The proto also encodes the meaningless field. Setting the count only when
`null_aware` is true would keep None as the 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]
