Review Comments for PR #7889

Overall this is a well-designed change with thorough tests. The sequence-group fallback mechanism for blob placeholders is a solid approach. A few observations:

Issues

1. Typo in comment (DataEvolutionSplitRead.java)

} else if (bunch instanceof BlobFileBunch) {
    // for blob funch, fallback on placeholders

→ "funch" should be "bunch"

2. Potential resource leak in BlobFallbackRecordReader.readBatch()

for (int i = 0; i < groupReaders.size(); i++) {
    RecordIterator<InternalRow> iterator = groupReaders.get(i).readBatch();
    if (iterator == null) {
        return null;  // ← iterators[0..i-1] are not released
    }
    iterators[i] = iterator;
}

If the k-th reader returns null, the already-obtained iterators [0, k-1] will never have releaseBatch() called. Should release them before returning null.

3. Memory pressure from ForceSingleBatchReader wrapping all group readers

Each sequence group is fully materialized into memory via ForceSingleBatchReader. When there are many sequence groups with large row ranges, this could cause significant memory pressure, especially when blob-as-descriptor is disabled and actual blob data is loaded. The TODO comment acknowledges this - just want to confirm this is acceptable for the first iteration.

4. Singleton placeholder row reuse in BlobSequenceGroupRecordReader

private InternalRow placeHolderRow() {
    if (placeholderRow == null) {
        GenericRow row = new GenericRow(readRowType.getFieldCount());
        row.setField(blobIndex, BlobPlaceholder.INSTANCE);
        placeholderRow = row;
    }
    return placeholderRow;
}

This returns the same mutable GenericRow instance for all placeholder positions. It works here because ForceSingleBatchReader copies the data, but it's fragile - a future caller that holds references to returned rows would see aliased data. Consider adding a comment noting this intentional reuse.

5. BlobFileBunch doesn't validate schemaId across files (by design?)

VectorFileBunch.add() enforces file.schemaId() == files.get(0).schemaId(), but BlobFileBunch.add() only checks writeCols. This seems intentional since blob files from different sequences naturally have different schemas, but worth confirming.

6. DataEvolutionFileReader contract relaxation

- checkArgument(readers != null && readers.length > 1, "Readers should be more than 1");
+ checkArgument(readers != null && readers.length >= 1, "should not pass empty readers.");

This relaxes the precondition for all callers of DataEvolutionFileReader, not just the blob path. Is there a case where a single reader is passed from non-blob paths? If not, consider keeping > 1 for non-blob scenarios or documenting when single-reader is expected.

Minor / Style

• In BlobFallbackRecordReaderTest, the ReadResult.add() method silently treats placeholder rows differently (counts them vs collecting rowIds). This is fine for testing but the test would be more explicit if assertions on placeholder count were always paired with total row count assertions.

• The fixedBlobBytes helper in BlobUpdateTest allocates 2 * 1024 * 124 bytes (≈248KB). Was 2 * 1024 * 1024 (2MB) intended? Or is this intentionally smaller to keep tests fast?

Positive

• The separation of SpecialFieldBunch into BlobFileBunch and VectorFileBunch is a good refactoring - they have fundamentally different semantics now.
• The backward-compatible version 1 reading test (testReadLegacyVersionOneBlobFile) is a nice addition.
• The BlobSequenceGroupRecordReader javadoc with ASCII art is very helpful for understanding the complex layout.
• Python-side changes properly mirror the Java changes with appropriate error handling for unsupported placeholder in read_arrow_batch.

Thanks for the update. I went through the latest patch again, focusing on the placeholder encoding/decoding path and the data-evolution fallback reader.

The previous major concerns look addressed or acceptable for this iteration:

• BlobFormatWriter now encodes null and placeholder distinctly, and BlobFormatReader maps placeholders back to the internal sentinel without exposing blob data accessors.
• BlobFallbackRecordReader now keeps blob sequence groups aligned and releases the normal returned iterators through releaseBatch().
• The fallback behavior handles row-range pushdown and overlapping sequence groups in the added tests.
• The DataEvolutionFileReader one-reader relaxation makes sense for blob-only projections.

I also ran a local compile check on the patched core modules:

mvn -pl paimon-core -am -DskipTests -Dspotless.check.skip=true -Drat.skip=true -Dcheckstyle.skip=true -Dscala-2.12 -DskipITs -Dfast compile

It passes locally.

I only have two non-blocking follow-ups:

1. The known memory/IO overhead in BlobFallbackRecordReader is still worth optimizing later, because stale rows are fully advanced even when a newer non-placeholder row has already been found.
2. There are still a few small wording/typo issues in comments/messages (latestFistRowId, "offset the that row", "could not be null"), but they do not affect correctness.

LGTM from my side.

Thanks @steFaiz for update. Left two comments.