xiangfu0 commented on code in PR #18514:
URL: https://github.com/apache/pinot/pull/18514#discussion_r3258753751
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/plannode/JoinNode.java:
##########
@@ -118,6 +118,6 @@ public int hashCode() {
}
public enum JoinStrategy {
- HASH, LOOKUP, ASOF
+ HASH, LOOKUP, ASOF, BROADCAST_RIGHT
Review Comment:
Adding `BROADCAST_RIGHT` here is not sufficient yet:
`pinot-common/src/main/proto/plan.proto` still only defines
`HASH`/`LOOKUP`/`AS_OF`, and the unchanged switches in
`PlanNodeSerializer.convertJoinStrategy()`,
`PlanNodeDeserializer.convertJoinStrategy()`, `DefaultJoinOperatorFactory`, and
`InStageStatsTreeBuilder` all reject any new enum. As written, a query using
this hint will fail once the plan is serialized or executed. Please thread the
new strategy through the wire/runtime path (or map it back to the existing
hash/non-equi operators at execution time if this is intended to be a
planning-only hint).
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotJoinExchangeNodeInsertRule.java:
##########
@@ -87,6 +87,19 @@ public void onMatch(RelOptRuleCall call) {
// worker, avoiding hotspots.
newLeft = PinotLogicalExchange.create(left,
RelDistributions.hash(Collections.emptyList()));
newRight = PinotLogicalExchange.create(right,
RelDistributions.hash(Collections.emptyList()));
+ } else if
(PinotHintOptions.JoinHintOptions.useBroadcastRightJoinStrategy(join)) {
Review Comment:
This layout is not correct for `RIGHT`/`FULL` outer joins. Broadcasting the
right table means every worker materializes its own copy, but
`HashJoinOperator` tracks matched-right rows locally before emitting unmatched
right rows. With the left side hash/random-partitioned, workers that do not see
the matching left row will emit the same right row as unmatched, so
`RIGHT`/`FULL` joins can return duplicate or incorrect null-extended rows. The
default code below already special-cases these join types for exactly that
reason; `broadcast_right` needs to reject/override those join types here, and
the same guard is needed in `TraitAssignment.assignJoin()`.
--
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]
