cloud-fan commented on code in PR #56486:
URL: https://github.com/apache/spark/pull/56486#discussion_r3462204698
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/jdbc/JDBCScanBuilder.scala:
##########
@@ -206,6 +224,12 @@ case class JDBCScanBuilder(
}
val otherJdbcScanBuilder = other.asInstanceOf[JDBCScanBuilder]
+ if ((leftSample != null && tableSampleClause.isEmpty) ||
Review Comment:
This guard looks unreachable. `pushDownSample` runs before `pushDownJoin` in
the rule, and a holder's `pushedSample` is only set when `pushTableSample`
succeeded — which for `JDBCScanBuilder` means `tableSampleClause` is already
defined. So a non-null `leftSample`/`rightSample` always implies a non-empty
`tableSampleClause`, and this branch never fires. It's safe (never wrongly
rejects), but dead — either drop it, or add a comment that it's defensive
against a future invariant change.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/SupportsPushDownJoin.java:
##########
@@ -63,6 +65,61 @@ boolean pushDownJoin(
Predicate condition
);
+ /**
+ * Pushes down the join of the current {@code SupportsPushDownJoin} and the
other side of join
+ * {@code SupportsPushDownJoin}, with pushed samples from either 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 leftSideRequiredColumnWithAliases} and {@code
rightSideRequiredColumnWithAliases}
+ * @param leftSample pushed sample from the left side, or null if there is
no pushed sample.
+ * @param rightSample pushed sample from the right side, or null if there is
no pushed sample.
+ * @return True if join has been successfully pushed down.
+ *
+ * @since 4.3.0
+ */
+ default boolean pushDownJoin(
Review Comment:
Are we going to add a parameter (and another delegating `default`) every
time a new pushed operator needs to compose with join pushdown? Sample today,
plausibly limit/aggregate next — each one telescopes the signature.
And these params are already largely redundant: the connector holds its own
pushed state (`JDBCScanBuilder.tableSampleClause`, the holder's
`pushedSample`), and `JDBCScanBuilder` actually ignores
`leftSample`/`rightSample` — it builds the SQL from its own
`tableSampleClause`, and the only use of the params is the (unreachable) guard
at line 227. They exist just to drive the default's fail-safe.
I'd suggest a single context object (e.g. `JoinPushDownInfo`) carrying the
pushed state, so a new dimension adds a field instead of a method. The one
thing to design in up front: the per-method default is doing real safety work
today — an un-updated connector doesn't override the new overload, so a real
sample is rejected. A context object loses that unless it's built in: a stale
override would silently ignore a newly-added field (e.g. drop a future pushed
limit). So the context should expose which pushed operators are present, and
the default should reject any the connector hasn't acknowledged. (A
capability-query that keeps `pushDownJoin` at 5 args is a fallback, but I'd
prefer the context object.)
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/SupportsPushDownJoin.java:
##########
@@ -63,6 +65,61 @@ boolean pushDownJoin(
Predicate condition
);
+ /**
+ * Pushes down the join of the current {@code SupportsPushDownJoin} and the
other side of join
+ * {@code SupportsPushDownJoin}, with pushed samples from either 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 leftSideRequiredColumnWithAliases} and {@code
rightSideRequiredColumnWithAliases}
Review Comment:
The `@code` references are singular but the parameters are plural
(`...Columns...`):
```suggestion
* {@code leftSideRequiredColumnsWithAliases} and {@code
rightSideRequiredColumnsWithAliases}
```
--
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]
