From 0458d6078ad347ca38b66c7ab952a665dff3d36d Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Tue, 18 Aug 2026 10:41:08 +0000 Subject: [PATCH] [GLUTEN-12377][VL] Run the Delta suite with Velox operator output validation The DV bitmap row-index failures in #12377 are intermittent and report a meaningless value, because by the time anything notices, the damage is an out-of-bounds read: a Delta deletion-vector write scans only synthesized columns with a pushed-down filter, and on that path Velox emits a RowVector whose row-index child has no rows. The child is then wrapped in a dictionary, and reading it returns whatever heap memory follows. Whether that memory happens to fall outside Delta's valid range decides whether the query aborts or silently accepts a wrong row index, which is why the failure moved from test to test and why the reported value differed every time. Velox can catch this at the source. `debug.validate_output_from_operators` checks every operator's output for structural consistency -- among other things, that a dictionary's indexes address rows its base vector has -- and names the first operator to emit a malformed vector. Expose it as a Gluten config and turn it on for the Delta suite. The suite is expected to fail while this is enabled, deterministically and with the responsible operator named, instead of intermittently with an arbitrary number: Output validation failed for [operator: TableScan, plan node ID: 0]: Child vector has size 0 less than parent and parent has no nulls 10. That is the point: it turns #12377 into a reliable signal. Once the upstream fix is picked up the suite must go green again, which is what validates it. Velox issue: https://github.com/facebookincubator/velox/issues/18535 Velox fix: https://github.com/facebookincubator/velox/pull/18536 Verified locally against the pinned Velox with and without that fix, on MergeIntoExtendedSyntaxSQLPathBasedDVsPredPushOnSuite "extended syntax - only update - isPartitioned: true": 9 validation failures and a failed test before, 0 failures and a pass after. The config defaults to off, so nothing outside this suite changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> [GLUTEN-12377][VL] Apply clang-format to the new config declaration Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../spark/sql/delta/test/DeltaSQLCommandTest.scala | 13 +++++++++++++ cpp/velox/compute/WholeStageResultIterator.cc | 2 ++ cpp/velox/config/VeloxConfig.h | 8 ++++++++ 3 files changed, 23 insertions(+) diff --git a/backends-velox/src-delta40/test/scala/org/apache/spark/sql/delta/test/DeltaSQLCommandTest.scala b/backends-velox/src-delta40/test/scala/org/apache/spark/sql/delta/test/DeltaSQLCommandTest.scala index 9c2b9efd5d6..2471a39161b 100644 --- a/backends-velox/src-delta40/test/scala/org/apache/spark/sql/delta/test/DeltaSQLCommandTest.scala +++ b/backends-velox/src-delta40/test/scala/org/apache/spark/sql/delta/test/DeltaSQLCommandTest.scala @@ -55,6 +55,19 @@ trait DeltaSQLCommandTest extends SharedSparkSession { .set(VeloxDeltaConfig.ENABLE_NATIVE_WRITE.key, "true") .set("spark.databricks.delta.snapshotPartitions", "2") .set("spark.gluten.sql.fallbackUnexpectedMetadataParquet", "true") + // Validate every operator's output vector. A Delta deletion-vector write scans only + // synthesized columns with a pushed-down filter, and on that path Velox emits a + // RowVector whose row-index child has no rows; the child is then wrapped in a + // dictionary, and reading it goes out of bounds and yields arbitrary heap values. + // Without this the failure is intermittent and reports a meaningless row index, which + // is how it went unnoticed for so long. With it, the scan is caught the moment it + // produces the malformed vector, so the suite fails deterministically and names the + // operator responsible. + // Velox issue: https://github.com/facebookincubator/velox/issues/18535 + // Velox fix: https://github.com/facebookincubator/velox/pull/18536 + // Remove this once that fix is picked up, at which point the suite must go green + // again -- which is what validates the fix. + .set("spark.gluten.sql.columnar.backend.velox.validateOutputFromOperators", "true") } } // spotless:on diff --git a/cpp/velox/compute/WholeStageResultIterator.cc b/cpp/velox/compute/WholeStageResultIterator.cc index 4e680fb25ba..842b7a567c6 100644 --- a/cpp/velox/compute/WholeStageResultIterator.cc +++ b/cpp/velox/compute/WholeStageResultIterator.cc @@ -525,6 +525,8 @@ void WholeStageResultIterator::collectMetrics() { std::unordered_map WholeStageResultIterator::getQueryContextConf() { std::unordered_map configs = {}; // Find batch size from Spark confs. If found, set the preferred and max batch size. + configs[velox::core::QueryConfig::kValidateOutputFromOperators] = + veloxCfg_->get(kValidateOutputFromOperators, kValidateOutputFromOperatorsDefault) ? "true" : "false"; configs[velox::core::QueryConfig::kPreferredOutputBatchRows] = std::to_string(veloxCfg_->get(kSparkBatchSize, 4096)); configs[velox::core::QueryConfig::kMaxOutputBatchRows] = diff --git a/cpp/velox/config/VeloxConfig.h b/cpp/velox/config/VeloxConfig.h index c0ae4b3ff86..bd97e22f2e4 100644 --- a/cpp/velox/config/VeloxConfig.h +++ b/cpp/velox/config/VeloxConfig.h @@ -107,6 +107,14 @@ const std::string kValueStreamDynamicFilterEnabled = "spark.gluten.sql.columnar.backend.velox.valueStream.dynamicFilter.enabled"; const bool kValueStreamDynamicFilterEnabledDefault = false; +/// Turns on Velox's per-operator output vector validation +/// (`debug.validate_output_from_operators`). Every operator's output is checked for +/// structural consistency -- among other things that a dictionary's indexes address rows its +/// base vector actually has -- and the first operator to emit a malformed vector is named. +/// Debugging aid only: it costs a pass over every output batch, so it is off by default. +const std::string kValidateOutputFromOperators = "spark.gluten.sql.columnar.backend.velox.validateOutputFromOperators"; +const bool kValidateOutputFromOperatorsDefault = false; + const std::string kShowTaskMetricsWhenFinished = "spark.gluten.sql.columnar.backend.velox.showTaskMetricsWhenFinished"; const bool kShowTaskMetricsWhenFinishedDefault = false;