Skip to content

[common][core] Preserve identity-mapped iterators safely - #9702

Open
jianguotian wants to merge 2 commits into
apache:masterfrom
jianguotian:fix/preserve-specialized-iterators-full-identity
Open

[common][core] Preserve identity-mapped iterators safely#9702
jianguotian wants to merge 2 commits into
apache:masterfrom
jianguotian:fix/preserve-specialized-iterators-full-identity

Conversation

@jianguotian

@jianguotian jianguotian commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Purpose

Preserve specialized ColumnarRowIterator implementations when an index mapping is an exact no-op, while keeping row-tracking decoration isolated from reusable format-reader batches.

The read path can supply an explicit full identity mapping even when all columns remain in their original order. Previously, ColumnarRowIterator.mapping called copy for every non-null mapping, and the base copy implementation could erase capabilities carried by a specialized iterator.

Returning the source iterator introduces a separate ownership requirement: assignRowTracking replaces entries in batch.columns in place. A format reader may reuse that column array across batches, so row-tracking wrappers must be applied to an isolated array rather than to the reader-owned batch.

This change:

  • returns the original iterator only when there is no partition mapping and the index mapping is exactly [0, 1, ..., batchArity - 1];
  • keeps the existing copy path for projections, reordering, missing columns, short mappings, and partition injection;
  • copies the column array before row-tracking decoration whenever mapping returned the reusable source iterator;
  • preserves the original recycler and batch-release lifecycle;
  • adds regression coverage for specialized-iterator preservation and repeated reusable-batch row tracking.

Tests

Validated commit: 151f38b6a91830770caf14f55e3a1b8bf690e1a5.

  • JDK 8: ColumnarRowIteratorTest and DataFileRecordReaderTest, 5/5 passed.
  • JDK 11: ColumnarRowIteratorTest and DataFileRecordReaderTest, 5/5 passed.
  • JDK 11 non-fast Maven verify for paimon-common, paimon-core, and required upstream modules passed Checkstyle, Spotless, Enforcer, RAT, and packaging.
  • DataFileRecordReaderTest repeats reusable identity-mapped batches and asserts that reader-owned columns remain unchanged and each batch is recycled exactly once.
  • git diff --check passed.
# JDK 8, then JDK 11
mvn -pl paimon-core -am -Pfast-build \
  -DfailIfNoTests=false -DwildcardSuites=none \
  -Dtest=ColumnarRowIteratorTest,DataFileRecordReaderTest test

# Configured non-fast verification on JDK 11
mvn -pl paimon-common,paimon-core -am -DskipTests verify

git diff --check "$(git merge-base origin/master HEAD)"...HEAD

@jianguotian jianguotian closed this Sep 9, 2026
@jianguotian jianguotian reopened this Sep 9, 2026

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

Found one row-tracking regression in the identity-mapping shortcut; details and reproduction are inline.

Comment on lines +118 to +119
if (partitionInfo == null && isIdentityMapping(indexMapping, row.batch().getArity())) {
return this;

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] Keep row-tracking wrappers out of the reader's reusable batch

For an unpartitioned table with row tracking enabled, reading t$row_tracking can reach this branch with a full identity mapping: FormatReaderMapping.Builder.trimKeyFields() returns an explicit identity array even when the schema mapping is null. DataFileRecordReader then calls assignRowTracking(), which replaces entries in batch.columns in place. Returning the original iterator exposes the Parquet/ORC reader's reusable column array to those mutations, so each reused batch wraps the previous batch's wrappers. Since their isNullAt() always returns false, each metadata getLong() recursively traverses the accumulated wrappers, causing progressively slower reads and eventually StackOverflowError. Previously, createMappedVectors() plus copy() isolated these mutations in a separate column array.

I reproduced this on JDK 8 with a real Parquet file, batch size 1, and the same mapping(...).assignRowTracking(...) sequence: this implementation overflows when checking the row at approximately 20,000 batches, while the identical test with the parent implementation completes all 100,000 batches. The four existing PR tests pass but do not exercise this reuse path.

Please keep row-tracking decoration isolated from the format reader's column array, or retain the copy path when row tracking needs to modify the vectors, and add a regression covering repeated batch reuse.

@jianguotian jianguotian changed the title [common] Preserve specialized iterators for full identity mappings [common][core] Preserve identity-mapped iterators safely Sep 10, 2026
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