HyukjinKwon commented on PR #59313:
URL: https://github.com/apache/spark/pull/59313#issuecomment-6157137709
@cloud-fan Thanks for the thorough review. I addressed all the inline
comments in aa0f3ba4dd77 (and e3ff80cc6bba, a test), and replied to each. On
the findings of the summary:
**Six `NativeBridge` statements**, all fixed in the Javadoc and on the docs
page:
- Errors: the errors of `closeReader`, `closeStreamReader` and `closeWriter`
are only logged, so the work that can fail a query, such as flushing data,
belongs in `commit`.
- Optional methods: `commitOffset` is listed with the pushdown and
`serialize` functions.
- `createDataSource`: the name is the one listed in the manifest, or the
requested name in lower case for an installed library.
- Schemas: the variants that Spark reads are listed exactly: `large_utf8`
and `utf8_view`, `large_binary` and `binary_view`, and any names for the
entries of a map. The rejected types now include large and fixed-size lists.
Spark also rejects them with a `NATIVE_DATA_SOURCE_ERROR` that names the
column, instead of `UNSUPPORTED_ARROWTYPE`.
- Handles: a batch reader whose partitions are never planned, for example to
only explain the query, is released once it is no longer referenced. The
Lifecycle and Handles sections say so.
- `pushPredicates`: the `-0.0` sentence is fixed. IEEE 754 comparisons treat
`-0.0` as equal to `0.0`, while total orders such as `f64::total_cmp` and the
arrow-rs kernels do not, and NaN differs from IEEE 754. Spark now also
evaluates all the predicates again after reading (see @tdcmeehan's thread), so
the rule is that a library must never skip a row for which they are true.
**Five guards without tests**, all added:
- The mapping of the answers: in `upper(name) = 'N1' AND id < 5`, the first
predicate cannot be encoded, and only `id < 5` is shown as pushed. The test
fails if the answers are zipped with all the predicates.
- Dictionary-encoded columns: the test library can export one
(`extra=dictionary`).
- A failed micro-batch write: Spark aborts epoch 0 with a null message for
its task. The test library now writes `_ABORTED_<epoch>`, with the number of
null messages.
- `stop()` releases the stream reader: `closeStreamReader` of the test
library writes a marker.
- A nested field: `select("st.b")` reads the whole `st` column. The test
fails if the pruned struct is passed as the read schema, because the check of
the schema rejects the data.
**PR description**: both sentences are updated. The logging one is accurate
now, since Spark also logs the packages of an unsupported version.
**Should version 1 be a C ABI?** That is #59424, which I opened for
comparison. It is the same feature with a C header as the interface:
`spark_datasource.h`, with `spark_ds_*` functions on `int64_t`, `int32_t` and
`bool`, UTF-8 strings, the Arrow C structs, status codes with
`spark_ds_last_error`, and byte arrays through a sink callback. It is called
through the FFM API, so it requires Java 22 or later. A JNI shim for Java 17
and 21 would mean building and shipping native code with Spark for each
platform.
**Could a failed load of Arrow's JNI library look like an unsupported
operation?** Yes. In Arrow 19, `JniLoader` loads the bundled `arrow_cdata_jni`
with `System.load` in a block that only catches `IOException`, so an
`UnsatisfiedLinkError` escapes. That happens for example when `java.io.tmpdir`
does not allow mapping the library. Fixed as you suggested: `NativeLibrary`
turns the `UnsatisfiedLinkError` of the method handle of a function into a
`MissingFunctionException`, and only that exception means a missing function to
`optional` and `call`. The link errors of Arrow are reported as a
`NATIVE_DATA_SOURCE_ERROR` with their message. New test: "tell missing
functions from other link errors".
--
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]
