comphead commented on code in PR #7361:
URL: https://github.com/apache/datafusion-comet/pull/7361#discussion_r4486202965
##########
native/core/src/execution/planner/join.rs:
##########
@@ -260,12 +259,23 @@ fn reduce_before_sort(
return Ok(None);
}
}
- let reducer: Arc<dyn ExecutionPlan> =
- Arc::new(MembershipKeyReductionExec::new(Arc::clone(input)));
- let sort = Arc::clone(¶ms.right.native_plan).replace_children(
- vec![Arc::clone(&reducer)],
- ReplaceChildrenOptions::new(ChildrenPropertiesMode::Recompute),
- )?;
+ let (input, ordering) = if key.is::<Column>() {
+ (Arc::clone(input), sort.expr().clone())
+ } else {
+ // Spark admitted an integer coalesce. Evaluate it once and use that
column for
+ // reduction, sorting, and the join. A left-semi join needs no other
right columns.
+ let name = "membership_key";
+ let projected =
+ ProjectionExec::try_new([(Arc::clone(key), name.to_owned())],
Arc::clone(input))?;
+ *key = Arc::new(Column::new(name, 0));
+ let ordering =
LexOrdering::from([PhysicalSortExpr::new(Arc::clone(key), *options)]);
+ (Arc::new(projected) as Arc<dyn ExecutionPlan>, ordering)
+ };
+ let reducer: Arc<dyn ExecutionPlan> =
Arc::new(MembershipKeyReductionExec::new(input));
+ let sort = Arc::new(
+ SortExec::new(ordering, Arc::clone(&reducer))
Review Comment:
This builds a new `SortExec` and carries over only `preserve_partitioning`,
where `replace_children` kept every setting of the original, so `sort.fetch()`
would be dropped silently. The caller excludes `fetch` and `skip` today, so
this is not a bug now. Would `.with_fetch(sort.fetch())` here keep the function
from depending on that check in `SortMergeJoinBuilder::build`?
##########
spark/src/test/scala/org/apache/comet/exec/CometJoinSuite.scala:
##########
@@ -1806,6 +1815,33 @@ class CometJoinSuite extends CometTestBase {
}
}
+ test("right key reduction coalesces two nullable INT columns") {
Review Comment:
The Scala gate admits every `IntegralType`, but the end-to-end coverage is
`INT` and `BIGINT` with two operands, so the projected `Int8` and `Int16` paths
are not planned in any test. Could this loop over `TINYINT` and `SMALLINT` too,
and add a three-operand `coalesce(r._1, r._2, <literal>)` so a second `WHEN`
branch runs?
--
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]
