cloud-fan commented on code in PR #56486:
URL: https://github.com/apache/spark/pull/56486#discussion_r3499509629
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/SupportsPushDownJoin.java:
##########
@@ -63,6 +69,157 @@ boolean pushDownJoin(
Predicate condition
);
+ /**
+ * Returns pushed operators this data source can preserve when pushing down
a join.
+ * <p>
+ * Spark uses this as a safety check before attempting join push down. If
the join input
+ * already contains pushed operators not listed here, Spark will not push
down the join.
+ * This prevents a data source from silently dropping pushed state it does
not understand.
+ *
+ * @since 4.3.0
+ */
+ default Set<PushedOperator> supportedPushedOperatorsForJoin() {
+ return Set.of();
+ }
+
+ /**
+ * Pushes down the join of the current {@code SupportsPushDownJoin} and the
other side of join
+ * {@code SupportsPushDownJoin}, with information about pushed operators
from each side.
+ *
+ * @param other {@code SupportsPushDownJoin} that this {@code
SupportsPushDownJoin}
+ * gets joined with.
+ * @param joinType the type of join.
+ * @param leftSideRequiredColumnsWithAliases required output of the
+ * left side {@code
SupportsPushDownJoin}
+ * @param rightSideRequiredColumnsWithAliases required output of the
+ * right side {@code
SupportsPushDownJoin}
+ * @param condition join condition. Columns are named after the specified
aliases in
+ * {@code leftSideRequiredColumnsWithAliases} and {@code
rightSideRequiredColumnsWithAliases}
+ * @param joinPushDownInfo information about pushed operators from each side
of the join.
+ * @return True if join has been successfully pushed down.
+ *
+ * @since 4.3.0
+ */
+ default boolean pushDownJoin(
+ SupportsPushDownJoin other,
+ JoinType joinType,
+ ColumnWithAlias[] leftSideRequiredColumnsWithAliases,
+ ColumnWithAlias[] rightSideRequiredColumnsWithAliases,
+ Predicate condition,
+ JoinPushDownInfo joinPushDownInfo) {
Review Comment:
This new API may not be needed at all. DSv2 pushdown has two properties that
matter here: pushes happen in a fixed order (sample before join,
`V2ScanRelationPushDown:50-61`), and each push returns accept/reject
immediately. Since the `ScanBuilder` is stateful, by the time `pushDownJoin`
runs it already holds its accepted sample and is the one deciding accept/reject.
So `JoinPushDownInfo` hands the connector data it already has — JDBC's
override never reads `joinPushDownInfo`; it preserves the sample from its own
`tableSampleClause`. And the rule itself already holds
`leftHolder.pushedSample` / `rightHolder.pushedSample`, so it doesn't need the
connector to hand the sample back to compute `isNoOp()`.
`supportedPushedOperatorsForJoin()` only does real work for a legacy
connector that implements both sample and join pushdown and was written before
this guard existed — an updated connector just rejects from its own state (as
JDBC effectively could). Given this interface is `@Evolving` and JDBC is the
only such OSS connector (updated in this PR), I think a one-line contract on
`pushDownJoin` — "if you hold a pushed sample you can't preserve across the
join, return false" — covers that case without new public types.
That would shrink the PR to: keep the guard removal, keep JDBC preserving
its own `tableSampleClause`, compute the no-op check in the rule from
`pushedSample`, and drop `JoinPushDownInfo` / `JoinSideInfo` / `TableSample` /
`PushedOperator` + the 6-arg overload. This walks back my context-object
suggestion from last round — the stateful + dynamic model is why I think we can
go further. Did you have a non-JDBC connector in mind that needs the
negotiation?
--
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]
