HyukjinKwon commented on PR #59424: URL: https://github.com/apache/spark/pull/59424#issuecomment-6180680750
@tdcmeehan Thank you for the detailed proposal, and for offering to write the `.proto`. I agree that version 1 is what libraries get built against, so its shape has to be settled now. In d2df236bd234, I kept typed C functions, but behind a single entry point and size-prefixed structs, the way ADBC's function table and JNI's `JNIEnv` grow, and moved what grows into fields and capabilities. On your five points: 1. **Every feature is a new function.** Features that are parameters now go into structs that grow: memory accounting, for example, would be functions appended to `SparkDsTaskContext`, and overwrite by filter a write mode with a field appended to `SparkDsWriteInfo`. A new operation, such as statistics, is still a new function, but a slot appended to the function table rather than a new exported symbol, and Spark only calls it for a data source that declares it. 2. **Each function is a separate contract.** Handles now follow one rule: each handle is released by the close function of its kind, and by nothing else. `commit_data_writer` no longer releases the data writer, which removes the class of bug that @LuciferYang found: Spark calls the new `close_data_writer` after the commit or the abort. What Spark passes is valid only during the call, except `SparkDsHost`, until the JVM exits, and `SparkDsTaskContext`, until the stream is released or the data writer is closed. 3. **Signatures are frozen.** `read` and `create_data_writer` get a `const SparkDsTaskContext*`, and `create_writer`, which replaces `create_writer` and `create_stream_writer`, gets a `const SparkDsWriteInfo*` with the write mode and the query ID. Both start with their size and only get fields appended, so a task context for memory accounting or cancellation does not need a second `read`. 4. **Support is inferred from symbol names.** A library exports only `spark_ds_init`, which returns its table, `SparkDsLibrary`, so in C and C++ the compiler checks the names and types of the functions. `create` returns the capabilities of each data source, which can depend on its options. Spark reports an operation that a data source does not declare as unsupported without calling the library, and if a declared capability needs a function that the table leaves NULL, it fails on the driver with the name of the function. `push_predicates` now answers, for each predicate, `SPARK_DS_PREDICATE_UNUSED`, `PARTIAL` (only to skip data) or `COMPLETE`. Spark shows the used ones as pushed and still evaluates all of them, so trusting `COMPLETE` can come later as an opt-in capability. 5. **Nothing flows from Spark to the library.** `spark_ds_init` gets `SparkDsHost`: the newest ABI version that Spark supports, the version of Spark, and a `log` function that writes to the log of the driver or the executor. `SparkDsTaskContext` has the IDs of the stage, partition, attempt and task attempt, and `is_interrupted`, so a read or a write that blocks can stop once its task is killed. The library returns a table of the version that it implements, so a new version does not need ADBC's fail-and-retry handshake. Your table, in these terms: | Addition | Now | |---|---| | Declared predicates, three-state answer | in version 1: the answers of `push_predicates` | | Cancellation | in version 1: `SparkDsTaskContext.is_interrupted` | | Memory accounting | functions appended to `SparkDsTaskContext` | | Statistics, partitioning, sort order | a function appended to the table, with a capability | | Aggregate and top-N pushdown | a function each appended to the table, with a capability | | Admission control | a function appended to the table, with a capability | | Overwrite by filter | a write mode, and a predicate field appended to `SparkDsWriteInfo`, with a capability | | Substrait | a pushdown function appended to the table, with a capability | Why I would keep typed functions rather than messages: - The header only depends on the C standard library, like the Arrow C data interface that it builds on. Messages would add a protobuf runtime and generated code to every library, which weighs most on C and C++ libraries. - The compilers check the table. With messages, the encoding is checked, but which fields a request carries, and who owns what, would still be prose. - Control calls are rare, and the data is Arrow C structs either way, so neither design is faster. Two costs that messages would avoid remain: a new operation is still a new function to specify, and a host that runs a library in another process would marshal each function instead of forwarding bytes. Tests: `the bindings match the layout of spark_datasource.h` compiles the header and compares every offset and size with the bindings, and a library whose table ends before `read` checks that Spark does not read past the size of a table. Others cover the capabilities, the host and its logging, the task context, a read that only ends once `is_interrupted` is true, and a failing `spark_ds_init`. Once 4.4.0 is released, the C counterpart of your replay test would be to keep building the test library against the released header. I also updated the PR description. Does this address your concern? If you still think the control calls should be messages, could you point to the addition that you think does not fit a table? -- 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]
