HeartSaVioR commented on PR #59908: URL: https://github.com/apache/spark/pull/59908#issuecomment-6512710446
> [!NOTE] > **DISCLAIMER:** This comment was written by an LLM, Claude Opus 5.5 (`claude-opus-5-5`, 1M context), posting on behalf of @HeartSaVioR. Thanks for the review. All the Javadoc contracts you asked about are settled in [795fc14c3d5](https://github.com/apache/spark/pull/59908/commits/795fc14c3d55a9ba9feac1244a257a5678755222); the one hook is deferred. **PR description (empty selection).** Agreed. This PR adds only `DATA_SOURCE_EXTERNAL_LOOKUP_NOT_SUPPORTED`; the description's 'Empty selection' bullet will say that the planning rules ([SPARK-60689](https://issues.apache.org/jira/browse/SPARK-60689), #59909) raise `EXTERNAL_LOOKUP_JOIN_UNSUPPORTED.NO_LOOKUP_PREDICATE`. **Does Spark close a cursor it has read to the end?** Yes, and it is now documented. `ExternalLookupReader.lookupBatch` says Spark closes every cursor it obtains, including one it has read to the end, before the next `lookupBatch` call and before closing the reader. `ExternalLookupResultCursor` says Spark closes every cursor, which may be before its end. A connector can release per-request resources in `close()`, and the input-retention rule now ends at that close. **Can a lookup builder receive nested-pruned schemas?** No. `SupportsExternalLookup.selectLookupPredicates` now says Spark passes only whole top-level columns to `pruneColumns`, never nested fields. So a builder that shares its nested pruning with batch scans never sees a nested field from a lookup. `Scan.toExternalLookup()` also says what `readSchema()` must contain: top-level columns, each at most once, in any order, including every column passed to `pruneColumns` (or every table column if the builder was not pruned). **Which operand holds the lookup column?** The `NamedReference` is always `children()[0]` and the `LookupParameter` is `children()[1]`. When the query writes the lookup column on the right, Spark flips the operator: `s.ts < t.event_time` is offered as `event_time > ?`. This is in the `selectLookupPredicates` Javadoc, with that example. A connector never has to handle the reversed order. **Should the public `V2ExpressionSQLBuilder` render `LookupParameter`?** Not in this PR. No connector in Apache Spark compiles lookup predicates to SQL yet, so there is no caller to design the hook against. A SQL-backed connector can already subclass `V2ExpressionSQLBuilder` and handle `LookupParameter` in `visitUnexpectedExpr`, as `ToStringSQLBuilder` does. Because operand order is now guaranteed, that subclass can rely on column-first. Your caveat about `<=>` is right: the inherited expansion repeats each operand, so such a connector needs its own `visitBinaryComparison` override, as `ToStringSQLBuilder` now has. If a connector needs a hook, adding one before the API is released stays cheap, since it is an added protected method with a default. The three inline threads are answered separately. -- 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]
