viirya commented on code in PR #27111:
URL: https://github.com/apache/datafusion/pull/27111#discussion_r4483902424
##########
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:
Thanks both, fixed in 41026ca7a. `NotIn::try_new` now checks the join type
and then `Partitioned` before it resolves the count, and `build()` checks for
an empty `on` before calling `not_in()`.
`test_null_aware_unset_value_keys_error_cause` covers a 2-key `Inner`, a
`Partitioned` `RightAnti`, a `Partitioned` `LeftAnti` and an empty `on`, and
fails with the old order.
##########
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:
Done. Both encoders now use `usize_to_wire`.
##########
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:
Done. I added `non_zero_usize_from_wire` next to `usize_from_wire` in
`datafusion_common::utils`, and both decoders use it, so a wire `0` is now the
same plan error from either one: `<Node>: null_aware_value_keys wire value 0
must be at least 1`.
##########
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:
Agreed. Both `with_null_aware_value_keys` setters now take
`Option<NonZeroUsize>`, like `with_fetch`. The physical planner and both proto
decoders are plain builder chains again, and `eliminate_join` no longer needs
struct-update syntax.
##########
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:
Every caller passed `projection: None`, `NullEqualsNothing` and `null_aware:
true`. The helper now takes only what varies (`filter`, `join_type`,
`partition_mode`), builds through `HashJoinExecBuilder`, and is always
null-aware, which matches its name.
##########
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:
Flattened.
--
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]
