Repository navigation
Conversation
JingsongLi
left a comment
There was a problem hiding this comment.
I reproduced one P1 correctness regression in the new bitmap-driven row-sidecar path; details are in the inline comment. On JDK 8, the 58 existing tests in DataEvolutionSplitReadTest and DataEvolutionFileIndexTest pass, but two additional regression tests pass on the parent commit (3f29758) and fail on this PR head (165cefa). With executeFilter(), both affected queries incorrectly return no rows.
| formatBuilder(readRowType, fileFilters, nestedFieldEnabled) | ||
| .build(formatIdentifier, schema, dataSchema)); |
There was a problem hiding this comment.
[P1] Use the physical schema when switching to a row sidecar
The new bitmap-driven selection can choose a row sidecar for an ordinary filtered read with rowRanges == null, but this mapping still uses the full table schema, and formatBuilder adds the virtual _ROW_ID and _SEQUENCE_NUMBER fields. The sidecar is written using the actual writeSchema. Unlike Parquet, RowBlockReader decodes bytes using the supplied field order and field count before applying the projection, so this schema mismatch silently changes the values.
I reproduced two cases with row sidecars enabled and a selective bitmap index:
- Write aligned
[f0, f1]and[f2]files, indexf2, and query onlyf2withf2 = 'b050'. Scan pruning leaves the file that physically contains onlyf2, but it is decoded using the full schema;b050becomes an empty string. - Write seven INT columns and filter the indexed column for
50. The writer uses a one-byte null header, while the reader adds two virtual fields and expects a two-byte header;50becomes838860800.
With residual filtering enabled, both queries return no rows instead of the matching row. These ordinary filtered reads use Parquet and pass on the parent commit.
Please build the row decoder's schema from dataFileSchema(file.writeCols()) and exclude the virtual tracking fields from its physical decoding schema. Tracking values should continue to be assigned from the manifest by DataFileRecordReader; applying only the writeCols projection will not fix the seven-column case.
There was a problem hiding this comment.
Thanks for identifying this regression. For this PR, I narrowed the scope so row-sidecar selection requires non-empty rowRanges, preserving the previous behavior for ordinary File Index/DV reads. The final File Index/DV-aware selection is still used for row-sidecar decisions when row ranges are available. I added regression coverage and both now return the correct results through executeFilter().
I will handle rowRanges == null in a follow-up PR by fixing the physical row-sidecar schema mapping first, then re-enabling bitmap-driven row-sidecar selection for that path.
| } | ||
|
|
||
| long selectedRowCount = selectedRowCount(file, rowRanges); | ||
| long selectedRowCount = selectedRowCount(file, rowRanges, fileIndexResult); |
There was a problem hiding this comment.
[P1] Non-empty row ranges still expose the physical-schema mismatch
The null/empty guard fixes the ordinary filtered-read cases, but this bitmap-based count still enables new sidecar reads for wide, non-empty ranges. For a 100-row file with rowRanges = [0, 99] and a bitmap containing only position 50, the baseline counts 100 selected rows and keeps reading Parquet; this version counts one row and switches to the row sidecar. The unchanged reader mapping then decodes the sidecar using the wrong physical schema.
This is reachable through normal APIs: ReadBuilder.withRowRanges(Collections.singletonList(new Range(0L, 99L))) combined with a selective file-index predicate, or a predicate such as _ROW_ID BETWEEN 0 AND 99 AND c6 = 50. Non-empty ranges are not guaranteed to be sparse or to include the file-index filtering result.
I extended the two new regression tests to exercise both range sources, including _ROW_ID in the read type for the row-id predicate cases. The projected [f2] file and seven-INT-column cases both incorrectly return no rows through executeFilter(). All four cases pass with the pre-change implementation (3f297583) and fail on this head (5ee90ac7); the existing 58 tests still pass on JDK 8.
Please fix the row decoder's physical schema mapping before enabling these additional sidecar switches: use dataFileSchema(file.writeCols()) and exclude the virtual tracking fields from the decoding schema, while continuing to assign tracking values from the manifest. Requiring non-empty rowRanges alone does not protect this path.
There was a problem hiding this comment.
Fixed by using the physical data schema for row-sidecar decoding, while continuing to supply row-tracking fields from the manifest. Added regression coverage for full explicit row ranges and _ROW_ID predicates.
|
[P1] Include the physical row schema in the merged sidecar mapping cache key The physical-schema fix resolves the earlier reported cases, but I reproduced this with real writes to one data-evolution table with row sidecars enabled:
The query returns two rows. The first Please include the physical data schema/order in the cache key whenever the target format is the row sidecar, as the existing nested cache key already does. Reusing only projected names is unsafe for positional decoding. Validation: all 58 existing tests in |
Reprodeced and fixed by including dataSchema.fields() from dataFileSchema(writeCols) in the merged reader cache key. |
Purpose
Before this change, row-sidecar selection was estimated from row ranges
only and did not account for rows pruned by file indexes and deletion
vectors, so sparse reads could still fall back to the main data files.
This change reuses the existing DV-aware file-index result, intersects
its bitmap with the row-range selection, and uses the resulting row count
and ratio to choose row sidecars for both single-file and merged-group
reads without reevaluating the file index or performing an additional
deletion-vector read.
Tests
after applying file indexes and deletion vectors.