felipepessoto commented on PR #12215:
URL: https://github.com/apache/gluten/pull/12215#issuecomment-5326297625
Following up on which stage builds the dictionary over an empty base: it is
not Delta, and not the aggregator. It is an upstream Velox bug in the scan.
`SelectiveStructColumnReaderBase::next()` has a branch for structs with **no
child readers** — a scan that projects only columns which are not read from the
file. There it clears `outputRows_` and repopulates it only when there is a
deletion, then sizes the result vector to `numValues` but sizes the synthesized
fields to `outputRows()`:
```cpp
outputRows_.clear();
if (hasDeletion_) {
fillOutputRowsFromMutation(numValues);
numValues = outputRows_.size();
}
...
resultRowVector->unsafeResize(numValues);
// parent <- numValues
setRowNumberField(currentRowNumber_, outputRows(), ..., childField);
// child <- outputRows().size()
```
and
```cpp
bool useOutputRows() const { return scanSpec_->hasFilter() ||
hasDeletion(); }
```
So a scan with a **filter but no deletion** gets the empty `outputRows_`,
and `_metadata.row_index` comes back with zero rows while the RowVector
containing it reports N rows. `BaseVector::wrapInDictionary` does not validate
indexes against base size, so that empty child is then wrapped in a dictionary
of size N, and the aggregator reads past the end of a zero-length buffer —
which is where the garbage row IDs come from.
The Delta DV write path hits all three conditions at once: it projects only
`_metadata.row_index`, `file_path` and a partition key, and predicate pushdown
puts a filter on the partition key. That gives a clean discriminator — same
test, same DV setup, only pushdown differs:
| variant | `TableScan[0]` output-validation failures | result |
|---|---|---|
| `...DVsPredPushOnSuite` | 9 | FAILED |
| `...DVsPredPushOffSuite` | 0 | passed |
Upstream:
* Issue: https://github.com/facebookincubator/velox/issues/18535
* Fix: https://github.com/facebookincubator/velox/pull/18536
With the fix applied to the Velox revision we pin, the deterministic
reproduction goes from 9 validation failures and a failed test to 0 and a pass,
and the full Delta DV suite run with `debug.validate_output_from_operators`
enabled reports 0 validation failures with results identical to the existing
baseline.
Two side notes that may be useful:
1. This explains why bumping or reverting Velox never changed the CI
outcome. `SelectiveStructColumnReader.cpp` and `SelectiveColumnReader.h` are
unchanged between `dft-2026_08_05` and `dft-2026_08_14`; the only commit
touching them in that range is an unrelated DWRF flat-map fix. The defect was
never in the code being bumped.
2. We do not have to wait for a Velox bump to unblock CI —
`ep/build-velox/src/get-velox.sh` already supports `UPSTREAM_VELOX_PR_ID`,
which applies a patch directly from a `facebookincubator/velox` PR.
--
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]
