malinjawi opened a new pull request, #12647: URL: https://github.com/apache/gluten/pull/12647
## What changes were proposed in this pull request? Revives #12030, which was auto-closed as stale, rebased onto current main. cc @felipepessoto This patch keeps simple Delta write invariants native in the Velox backend. The change: - Adds `GlutenDeltaInvariantChecker` for Delta 3.3 and Delta 4.0 sources. - Validates top-level `NOT NULL` constraints directly on Velox columnar batches before write. - Avoids `DeltaInvariantCheckerExec` and `ColumnarToRow` for supported NOT NULL-only writes. - Keeps unsupported constraints conservative: `CHECK` constraints and nested NOT NULL constraints still use Delta's row checker. - Adds a Velox JNI helper to detect nulls in selected columns, using cached `nullCount` when available. - Adds Spark 3.5 and Spark 4.0 tests for native NOT NULL checks, fallback behavior, and violation reporting. Changes compared to #12030: - Rebased onto current main. - Dropped the `VeloxTestSettings` (spark41) flaky-test excludes, which were unrelated to this change; main has since excluded the affected test independently. - Added a design note on `GlutenDeltaInvariantChecker` documenting why the check is batch-metadata-based rather than an offloaded expression (see below). ### Design note: why not offload `CheckDeltaInvariant` to Velox? @zhztheplayer asked this on #12030 — I looked at offloading `CheckDeltaInvariant` as an expression before settling on this design. Three reasons I went with the batch-metadata check for the NOT NULL-only case: 1. **Error contract.** Delta's contract is a typed `DeltaInvariantViolationException` with error classes (`DELTA_NOT_NULL_CONSTRAINT_VIOLATED`, etc.), and `writeFiles` unwraps `InnerInvariantViolationException` to re-surface exactly that type. Anything thrown inside Velox crosses the JNI boundary via `JNI_METHOD_END`, which flattens every native exception into `GlutenException(e.what())` — the exception type and error class are lost, only the message survives. Our existing offloaded `raise_error`/`assert_true` show this: `GlutenColumnExpressionSuite` can only assert `RuntimeException` + message-contains, not the vanilla Spark exception types. With this approach the violation is *detected* natively but *thrown* on the JVM side, so we throw the genuine `DeltaInvariantViolationException(constraint)` with full fidelity. 2. **Cost.** An offloaded `CheckDeltaInvariant` still evaluates per row and adds an operator to the write pipeline. The batch check reads Velox vector `nullCount` metadata (falling back to a null-buffer scan only when unset), so the common no-violation case is near-free and there is no plan change at all — which also means nothing new to validate or fall back on. 3. **Surface.** Expression offload needs a new Substrait function, a Velox-side throwing function registration, an `ExpressionTransformer` for a Delta-internal non-SQL expression, and a `DeltaInvariantCheckerExec` transformer with validation/fallback wired into the manually-built write plan. That is a lot of machinery for a case where a metadata read answers the question. Where expression offload wins is generality: it would cover `CHECK` constraints and nested NOT NULL, which this PR deliberately leaves on the row-based `DeltaInvariantCheckerExec` fallback. I see that as a follow-up that becomes attractive once Gluten has a structured way to map native errors back to typed Spark/Delta exceptions. The fallback structure here doesn't block it — `GlutenDeltaInvariantChecker.create` already returns `None` for anything beyond top-level NOT NULL, so an offloaded path could slot in for those cases later. ## Why are the changes needed? Delta writes with table invariants currently go through `DeltaInvariantCheckerExec`, which is row-based. Even simple top-level `NOT NULL` constraints can introduce a C2R transition in an otherwise native Delta write path. This patch handles the safe common case natively while preserving Delta's existing fallback behavior for unsupported constraints. ## Does this PR introduce any user-facing change? No public API change. Delta constraint behavior is preserved; supported NOT NULL-only writes can stay native. ## How was this patch tested? On this rebased branch: - Spark 3.5 (`-Pspark-3.5,delta,spark-ut`) test-compile of `backends-velox`: passed - Spark 4.0 (`-Pspark-4.0,scala-2.13,java-17,delta,spark-ut`) test-compile of `backends-velox`: passed - Spark 3.5 and Spark 4.0 spotless: passed - clang-format-15 on `cpp/velox/jni/VeloxJniWrapper.cc`: clean On #12030 (same change before the rebase; only conflicts with drifted test imports were resolved): - Spark 3.5 `DeltaNativeWriteInvariantSuite`: passed - Spark 4.0 `DeltaNativeWriteSuite`: passed - C++ Velox build passed, JNI symbol verified in `libvelox.dylib` Covered cases: - native Delta write checks top-level NOT NULL without `DeltaInvariantCheckerExec` - native NOT NULL path avoids `ColumnarToRow` - CHECK constraints keep `DeltaInvariantCheckerExec` - NOT NULL violations report `InvariantViolationException` Note on CI coverage: the Spark 3.5 suite runs in the `velox_backend_enhanced` Spark 3.5 job. The Spark 4.0 delta suites (including the pre-existing `DeltaNativeWriteSuite`) are compile-checked by CI but not currently executed by any Spark 4.0 test job — that is a pre-existing gap for the whole `src-delta40` test tree, so the Spark 4.0 tests were verified locally. From #12030, a targeted local benchmark for append workloads comparing the native top-level NOT NULL path with an equivalent unsupported CHECK fallback path (2,000,000 rows, 14 columns, 3 append iterations): | path | avg ms | | --- | ---: | | native top-level NOT NULL checker | 1713 | | row CHECK fallback path | 1664 | Excluding the first warm-up append: | path | avg ms | | --- | ---: | | native top-level NOT NULL checker | 1605 | | row CHECK fallback path | 1647 | The benchmark is effectively neutral because Delta write setup, Parquet output, and commit/log work dominate this microbenchmark. This PR is mainly native write correctness/posture work: it removes a row invariant operator and C2R transition from a common constrained Delta write path. Related issue: #10215 Tracked by #12025 Supersedes #12030 -- 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]
