JingsongLi commented on code in PR #10415:
URL: https://github.com/apache/paimon/pull/10415#discussion_r4216188442
##########
paimon-core/src/main/java/org/apache/paimon/operation/DataEvolutionSplitRead.java:
##########
@@ -1223,16 +1231,25 @@ static boolean shouldReadRowSidecar(
@Nullable List<Range> rowRanges,
long maxSelectedRows,
double maxSelectionRatio) {
- if (rowRanges == null
- || rowRanges.isEmpty()
+ return shouldReadRowSidecar(file, rowRanges, null, maxSelectedRows,
maxSelectionRatio);
+ }
+
+ @VisibleForTesting
+ static boolean shouldReadRowSidecar(
+ DataFileMeta file,
+ @Nullable List<Range> rowRanges,
+ @Nullable FileIndexResult fileIndexResult,
+ long maxSelectedRows,
+ double maxSelectionRatio) {
+ if (isNullOrEmpty(rowRanges)
|| file.rowCount() <= 0
|| isBlobFile(file.fileName())
|| isVectorStoreFile(file.fileName())
|| rowSidecarFileName(file) == null) {
return false;
}
- long selectedRowCount = selectedRowCount(file, rowRanges);
+ long selectedRowCount = selectedRowCount(file, rowRanges,
fileIndexResult);
Review 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.
--
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]
