Skip to content

[core] Select row sidecars using the final Data Evolution selection - #10415

Open
lilei1128 wants to merge 5 commits into
apache:masterfrom
lilei1128:policy-row-sidecar
Open

lilei1128 wants to merge 5 commits into
apache:masterfrom
lilei1128:policy-row-sidecar

Conversation

@lilei1128

Copy link
Copy Markdown
Contributor

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

  • Add a unit test for row sidecar selection using the final bitmap.
  • Add end-to-end tests for single-file and merged-group row sidecar reads
    after applying file indexes and deletion vectors.

@JingsongLi JingsongLi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +770 to +771
formatBuilder(readRowType, fileFilters, nestedFieldEnabled)
.build(formatIdentifier, schema, dataSchema));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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, index f2, and query only f2 with f2 = 'b050'. Scan pruning leaves the file that physically contains only f2, but it is decoded using the full schema; b050 becomes 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; 50 becomes 838860800.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@JingsongLi

Copy link
Copy Markdown
Contributor

[P1] Include the physical row schema in the merged sidecar mapping cache key

The physical-schema fix resolves the earlier reported cases, but createUnionReader still keys non-nested formatReaderMappings only by schema ID, format and requested field names. Different writeCols layouts can request the same fields while requiring different positional row decoders.

I reproduced this with real writes to one data-evolution table with row sidecars enabled:

  • Row IDs 0–99: files physically containing [f0, f1] and [f2].
  • Row IDs 100–199: files physically containing [f1] and [f2].
  • Both f2 files have a bitmap index. Query projection is [f1, f2], with f2 = 'b050' and no explicit row ranges.

The query returns two rows. The first f1 is correctly a050; the second should be second-a050, but becomes a corrupt string beginning nd-a050 and containing strings from subsequent rows (second-a051 ... second-a058). Both groups use the same FormatKey(schemaId, row, [f1]), so the second sidecar is decoded with the first group's [f0,f1] schema.

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 DataEvolutionSplitReadTest and DataEvolutionFileIndexTest pass with standard JDK 8 Maven checks. The additional real-table multi-group case fails on this head. It passes when a temporary cache-key change includes dataSchema.fields() for row sidecars, and also passes when the selection-ratio setting forces Parquet. All temporary source changes were restored. Independent review confirmed this layout matches supported partial-column writing, and found no further issue in row-ID/sequence/DV position handling.

@lilei1128

Copy link
Copy Markdown
Contributor Author

[P1] Include the physical row schema in the merged sidecar mapping cache key

The physical-schema fix resolves the earlier reported cases, but createUnionReader still keys non-nested formatReaderMappings only by schema ID, format and requested field names. Different writeCols layouts can request the same fields while requiring different positional row decoders.

I reproduced this with real writes to one data-evolution table with row sidecars enabled:

  • Row IDs 0–99: files physically containing [f0, f1] and [f2].
  • Row IDs 100–199: files physically containing [f1] and [f2].
  • Both f2 files have a bitmap index. Query projection is [f1, f2], with f2 = 'b050' and no explicit row ranges.

The query returns two rows. The first f1 is correctly a050; the second should be second-a050, but becomes a corrupt string beginning nd-a050 and containing strings from subsequent rows (second-a051 ... second-a058). Both groups use the same FormatKey(schemaId, row, [f1]), so the second sidecar is decoded with the first group's [f0,f1] schema.

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 DataEvolutionSplitReadTest and DataEvolutionFileIndexTest pass with standard JDK 8 Maven checks. The additional real-table multi-group case fails on this head. It passes when a temporary cache-key change includes dataSchema.fields() for row sidecars, and also passes when the selection-ratio setting forces Parquet. All temporary source changes were restored. Independent review confirmed this layout matches supported partial-column writing, and found no further issue in row-ID/sequence/DV position handling.

Reprodeced and fixed by including dataSchema.fields() from dataFileSchema(writeCols) in the merged reader cache key.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants