gengliangwang commented on PR #59424: URL: https://github.com/apache/spark/pull/59424#issuecomment-6199268275
Following up on [my two DSv2 adapter suggestions on #59313](https://github.com/apache/spark/pull/59313#issuecomment-6198311277), after checking this PR at `51af65c`. The planner/reader separation still applies; the capability suggestion is partly addressed and can now be narrowed to the DSv2 table layer. 1. **Separate driver-side planning from executor-side reading.** [`ColumnarReader`](https://github.com/apache/spark/blob/51af65c63a9369d793aab1a198ddde4c90c8f85b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/columnar/ColumnarDataSource.scala#L62-L90) still combines pushdown negotiation and partition planning with executor `read(partition)`. [`NativeDataSourceReader`](https://github.com/apache/spark/blob/51af65c63a9369d793aab1a198ddde4c90c8f85b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ffi/NativeDataSource.scala#L217-L295) still changes roles through a transient native handle and mutable `readerState`, and [`ColumnarScanBuilder.build()`](https://github.com/apache/spark/blob/51af65c63a9369d793aab1a198ddde4c90c8f85b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/columnar/ColumnarTable.scala#L142-L145) passes closures capturing the mutable builder. Could we keep `ColumnarTable -> ColumnarScanBuilder -> ColumnarScan`, but introduce a driver-only `BatchReadPlanner` whose `plan()` returns `PlannedBatch(partitions, readerFactory)`? The serializable executor factory would contain only the library location, source name, read schema, and serialized native state. `build()` would establish stable scan metadata and explicit planner ownership, with defined behavior for repeated calls. This can preserve the C ABI and lazy planning point: call `partitions`, then `serialize_reader`, and close the native reader in `finally`. Keep the existing caching of successful plans **and planning failures**, lazy batch-planner creation for streaming-only sources, and the Cleaner fallback for scans that never reach planning. The benefit is clearer ownership and a smaller executor object, without changing when native partitions are planned. 2. **Expose source-specific capabilities through `Table.capabilities()`.** The new capability flags returned by native `create` and the [checks in `NativeDataSource.create`](https://github.com/apache/spark/blob/51af65c63a9369d793aab1a198ddde4c90c8f85b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ffi/NativeDataSource.scala#L138-L155) address discovery and reject unsupported operations before invoking native reader/writer creation. However, [`ColumnarTable.capabilities()`](https://github.com/apache/spark/blob/51af65c63a9369d793aab1a198ddde4c90c8f85b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/columnar/ColumnarTable.scala#L55-L56) still advertises batch/streaming reads and writes plus truncate for every source, and [`NativeDataSourceV2.getTable()`](https://github.com/apache/spark/blob/51af65c63a9369d793aab1a198ddde4c90c8f85b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ffi/NativeDataSourceV2.scala#L59-L66) constructs that table without a capability override. Could we propagate the existing source-specific capability information into the DSv2 table as well, so a batch-only source is represented as such during DSv2 capability checks? The discovery mechanism now exists, so there is no need to add another native ABI query. Since capabilities may depend on options and query options can override table options, the table-level declaration needs defined, stable semantics; the operation-specific checks should remain. These are internal adapter suggestions. The Rust-side planner/factory split in #59520 complements them, but does not change these Scala classes. -- 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]
