[DO NOT MERGE] Same as #12783, plus the upstream Velox fix (expected green) - #12808
[DO NOT MERGE] Same as #12783, plus the upstream Velox fix (expected green)#12808felipepessoto wants to merge 2 commits into
Conversation
07a55cc to
3766a99
Compare
Result: green. All 8 Delta suite shards and the aggregate job passed, with Velox's per-operator output validation enabled throughout. #12783 is this same branch without the one line that applies facebookincubator/velox#18536, on the same base commit and the same pinned Velox, and 5+ of its 8 shards fail with Plan from here: once velox#18536 merges it arrives through the regular weekly Velox bump, after which a separate PR removes the DV bitmap entries from |
|
Superseding the green result above: that run applied the patch at 19:04 UTC, before facebookincubator/velox#18536 was reworked in response to review. It fetched 6,425 bytes / one commit, where the PR now serves 12,276 bytes / three, so it validated the earlier per-call-site approach rather than the current one. Re-triggered with an empty commit ( |
…idation The DV bitmap row-index failures in apache#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 apache#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: facebookincubator/velox#18535 Velox fix: facebookincubator/velox#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>
ef63b2b to
6967a62
Compare
apache#12783 turns on Velox's per-operator output validation for the Delta suite and is expected to fail, deterministically, at the scan that emits a row-index child shorter than the vector containing it (GLUTEN-12377). This branch is that change plus one line: it points UPSTREAM_VELOX_PR_ID at the upstream fix, so get-velox.sh applies it before the native build. The same suite is expected to pass here. Red on apache#12783 and green here is the before/after evidence that the defect is the Velox one and that the fix addresses it, without waiting for the fix to merge and a Velox bump to land. Velox issue: facebookincubator/velox#18535 Velox fix: facebookincubator/velox#18536 Not for merge: it pins the build to an unmerged pull request. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
6967a62 to
30a2144
Compare
What this is
This is #12783 plus one line: it sets
UPSTREAM_VELOX_PR_ID="18536"inep/build-velox/src/get-velox.sh, soget-velox.shapplies the upstream Velox fix before the native build.Both are branched from the same commit on
main, so the fix is the only difference between them.Why
#12377 has been hard to act on because the failure is intermittent and reports a meaningless row index. Root cause: on a Delta deletion-vector write the scan projects only synthesized columns with a pushed-down filter, and Velox emits a
RowVectorwhose row-index child has no rows. That child is wrapped in a dictionary and read out of bounds, so the "row index" is whatever heap memory follows.Red on #12783 and green here is the before/after evidence, available now rather than after the upstream fix merges and a Velox bump lands.
Notes
--build_tests=OFFin the CI native build, so the patch'sTableScanTest.cpphunk is applied but never compiled; only the fix is.dft-2026_08_17(b99dd5720), which is whatget-velox.shchecks withgit apply --checkbefore applying. The defect is also confirmed still present there, so [GLUTEN-12377][VL][DO NOT MERGE] Run the Delta suite with Velox operator output validation #12783 is expected to be red on the same base.Was this patch authored or co-authored using generative AI tooling?
Generated-by: GitHub Copilot CLI (Claude Opus 5)