JingsongLi commented on code in PR #10001:
URL: https://github.com/apache/paimon/pull/10001#discussion_r4056471363
##########
docs/docs/concepts/spec/rowformat.md:
##########
@@ -182,11 +182,12 @@ To read a row by its zero-based row number within the
file:
1. **Read Footer**: Seek to file end - 32 bytes, read the 32-byte footer.
Validate magic number.
2. **Read Block Index**: Seek to `indexOffset`, read `indexLength` bytes,
decode the three arrays. Compute block offsets by prefix sum of
`blockCompressedSizes[]`.
-3. **Select Block**: Find block `b` where `blockRowStarts[b] <= rowNum <
blockEnd`. For the last block, `blockEnd` is `totalRowCount`; otherwise it is
`blockRowStarts[b + 1]`.
-4. **Read Block**: Seek to `blockOffset(b)`, read `blockCompressedSizes[b]`
bytes.
-5. **Decompress**: ZSTD decompress into a buffer of size
`blockUncompressedSizes[b]`.
-6. **Locate Row**: Compute `localIdx = rowNum - blockRowStarts[b]`. Read
`offsets[localIdx]` from the offset array at the end of the decompressed block.
-7. **Deserialize**: Read the row starting at the computed offset using the row
serialization format.
+3. **Check Consistency**: The three arrays must have the same length, that
length must equal `blockCount`, and `blockCompressedSizes[]` must sum to
`indexOffset`, because the blocks are written contiguously from position 0 and
the index follows the last one. A reader that bounds its block loop by one of
the two — the footer's `blockCount` or the index array length — must reject a
file where they disagree rather than silently reading fewer blocks.
Review Comment:
[P1] Apply the new consistency contract to the Python row reader too
This now defines rejection as a format-reader requirement, and the PR
description specifically calls out that Python bounds iteration by the footer
count, but `pypaimon/read/reader/format_row_reader.py::_read_metadata` still
trusts `block_count`, `index_offset`, and `index_length` and never compares the
three decoded array lengths or their compressed-size sum. For example, changing
`blockCount` to 0 still makes Python return an empty result for a non-empty
file rather than reject it. Please implement the same validation and regression
cases in Python so Java and Python do not retain the cross-language behavior
this change is meant to remove.
##########
paimon-format/src/main/java/org/apache/paimon/format/row/RowBlockIndex.java:
##########
@@ -36,12 +38,62 @@ class RowBlockIndex {
RowBlockIndex(
long[] blockCompressedSizes, long[] blockUncompressedSizes, long[]
blockRowStarts) {
+ checkArgument(
+ blockCompressedSizes.length == blockUncompressedSizes.length
+ && blockCompressedSizes.length ==
blockRowStarts.length,
+ "Row file block index arrays disagree on the block count: %s
compressed sizes, %s uncompressed sizes, %s row starts.",
+ blockCompressedSizes.length,
+ blockUncompressedSizes.length,
+ blockRowStarts.length);
this.blockCompressedSizes = blockCompressedSizes;
this.blockUncompressedSizes = blockUncompressedSizes;
this.blockRowStarts = blockRowStarts;
this.blockOffsets = computeOffsets(blockCompressedSizes);
}
+ /**
+ * Checks the index against the footer, which is the only place both are
in hand. Blocks are
+ * written contiguously from position 0 and the index follows the last
one, so the compressed
+ * sizes must sum to exactly {@code indexOffset} — see the row format
spec. Row starts index the
+ * arrays of every later lookup and size the per-block selection array.
+ */
+ void validate(RowFileFooter footer) throws IOException {
+ if (blockCount() != footer.blockCount) {
+ throw new IOException(
+ String.format(
+ "Row file block index holds %d blocks, but the
footer declares %d.",
+ blockCount(), footer.blockCount));
+ }
+
+ long blocksEnd =
+ blockCount() == 0
+ ? 0
+ : blockOffset(blockCount() - 1) +
blockCompressedSize(blockCount() - 1);
+ if (blocksEnd != footer.indexOffset) {
+ throw new IOException(
+ String.format(
+ "Row file blocks end at %d, but the footer puts
the block index at %d.",
+ blocksEnd, footer.indexOffset));
+ }
+
+ for (int i = 1; i < blockCount(); i++) {
+ if (blockRowStarts[i] < blockRowStarts[i - 1]) {
Review Comment:
[P1] Reject row-start gaps and duplicates before selection uses them
Checking only for a decrease still accepts `[10]` for a one-block file and
`[0, 0]` for two blocks. `RowFormatReader.computeBlocksToRead` then treats
those values as block ranges: selected rows 0-9 are omitted in the first case,
and the first block has the empty range `[0, 0)` in the second, so
selection-backed reads can silently drop valid rows even though this validation
succeeds. Please require the first start to be 0, subsequent starts to increase
strictly, and an empty index only when `totalRowCount` is 0; a selection
regression would pin the behavior.
--
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]
