Skip to content

[file-index] Support stream file index containers - #10101

Open
zjw1111 wants to merge 11 commits into
apache:masterfrom
zjw1111:codex/file-index-v2-format
Open

zjw1111 wants to merge 11 commits into
apache:masterfrom
zjw1111:codex/file-index-v2-format

Conversation

@zjw1111

@zjw1111 zjw1111 commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Purpose

V2 file-index containers now support table writes and index rewrites when an index payload or the container exceeds 2 GiB. Previously, both production paths assembled the complete container in a ByteArrayOutputStream, so selecting V2 did not remove that size limit.

The container writer accepts payloads through writeIndex and FileIndexWriter.writeTo(OutputStream). DataFileIndexWriter and FileIndexProcessor write through a spillable output stream: small containers stay embedded in file metadata, while larger ones become independent index files. V2 records 64-bit payload positions and lengths in its footer; readers continue to accept V1 and V2. The plugin reader interface now takes a long payload length. Existing built-in index writers still serialize individual payloads as byte arrays, and V1 keeps its header-first, in-memory write behavior. V1 remains the default format.

Tests

  • FileIndexFormatFormatTest: testV2TotalSizeOverTwoGiBAndLegacyBackendAtLargePosition, testV2StreamedPayloadOverInt32, and testV2CopyPayloadOverInt32 cover large positions, a single large streamed payload, and copying that payload without a large byte array. testIncrementalWriteAndCopyPayload covers V1/V2 incremental writes, and testV2RejectInvalidTailMagic covers a damaged trailer.
  • DataFileIndexWriterTest: testSpillableIndexOutputStream covers the embed/spill boundary; testV2TableWriterStreamsPayloadOverTwoGiB writes a table index above 2 GiB; testV2FailedWriteDeletesPartialIndexFile checks cleanup after a failed write.
  • FileIndexProcessorTest: testV2RewritesContainerOverTwoGiB rewrites a table index above 2 GiB. The map-key rewrite test also runs with V1 and V2.
  • CoreOptionsTest.testFileIndexInManifestThreshold covers threshold bounds, and FileIndexPredicateCloseTest.testFileStatusFailureDoesNotOpenStream covers reader construction failure.
  • FileIndexesTableTest runs embedded and external index cases with both V1 and V2.

@zjw1111
zjw1111 marked this pull request as ready for review September 22, 2026 10:42
@zjw1111 zjw1111 changed the title [file-index] Support V2 file indexes over 2 GiB [file-index] Stream V2 index containers beyond 2 GiB Sep 22, 2026
@zjw1111 zjw1111 changed the title [file-index] Stream V2 index containers beyond 2 GiB [file-index] Stream V2 index containers Sep 22, 2026
@zjw1111 zjw1111 changed the title [file-index] Stream V2 index containers [file-index] Support stream file index containers Sep 22, 2026
FileIndexWriter createWriter();

FileIndexReader createReader(SeekableInputStream inputStream, int start, int length);
FileIndexReader createReader(SeekableInputStream inputStream, long start, long length);

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.

This directly replaces the method descriptor of the FileIndexer SPI. Existing index plugins are loaded through  ServiceLoader  and were compiled with  createReader(SeekableInputStream, int, int) ; after upgrading Paimon, those unchanged plugin binaries do not implement this new descriptor, so reading their indexes fails with  AbstractMethodError . Could we preserve the old overload and add the long overload as a default bridge? For example, keep the old int  method (deprecated), let the default long method range-check and delegate to it, and have built-ins that support large positions override the long method. Please add a compatibility test that compiles/loads an implementation exposing only the old descriptor and reads a V1 container through the new runtime.

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.

Done

try {
close();
} finally {
if (fileCreated) {

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.

non-blocking nit: FileIO.delete  can return  false  instead of throwing when cleanup fails (the Hadoop-backed implementations propagate FileSystem.delete ), but this return value is ignored. A failed streamed write can therefore leave its partial index file behind while abort() appears to have completed. Could we treat  false  as a cleanup failure when the path still exists and attach that failure as suppressed to the original write exception? A small fake- FileIO  test returning  false  would cover this backend behavior.

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.

Done

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

This has clear end-to-end value: the current V2 write and rewrite paths still assemble the whole container in memory, so a container over 2 GiB cannot be produced. The V2 footer and spill-to-file path address that production limit, with V1 remaining the default.

Blocking compatibility issue (P1): FileIndexer.createReader(SeekableInputStream, int, int) is replaced by the (long, long) descriptor. Existing third-party FileIndexer implementations loaded through ServiceLoader can still be discovered, but the first read now calls a method their binary does not implement and fails with AbstractMethodError. This affects existing V1 indexes even when the new V2 option is never enabled. Akash3121 already raised this on FileIndexer.java:35; I agree it needs a compatibility bridge and a test using a plugin compiled against the old API before merge. The rollout should also keep the documented reader-first order when V2 is enabled.

The V1/V2 container tests passed locally (10/10). DataFileIndexWriterTest passed locally (5/5), including the over-2-GiB streamed payload and failed-write cleanup cases. The FileIndexProcessorTest map-key rewrite cases could not initialize in this local workspace due to the existing CodeGenerator service-loading failure; FileIndexPredicateCloseTest failed before its logic ran because Mockito could not attach to this JVM. The PR head's JDK 8/11, Flink 1/2, Spark and E2E CI checks are green. No other confirmed correctness regression was found in the format and write/rewrite paths.

@zjw1111
zjw1111 force-pushed the codex/file-index-v2-format branch from 54e8a18 to deb0579 Compare September 24, 2026 06:59
@zjw1111

zjw1111 commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

This has clear end-to-end value: the current V2 write and rewrite paths still assemble the whole container in memory, so a container over 2 GiB cannot be produced. The V2 footer and spill-to-file path address that production limit, with V1 remaining the default.

Blocking compatibility issue (P1): FileIndexer.createReader(SeekableInputStream, int, int) is replaced by the (long, long) descriptor. Existing third-party FileIndexer implementations loaded through ServiceLoader can still be discovered, but the first read now calls a method their binary does not implement and fails with AbstractMethodError. This affects existing V1 indexes even when the new V2 option is never enabled. Akash3121 already raised this on FileIndexer.java:35; I agree it needs a compatibility bridge and a test using a plugin compiled against the old API before merge. The rollout should also keep the documented reader-first order when V2 is enabled.

The V1/V2 container tests passed locally (10/10). DataFileIndexWriterTest passed locally (5/5), including the over-2-GiB streamed payload and failed-write cleanup cases. The FileIndexProcessorTest map-key rewrite cases could not initialize in this local workspace due to the existing CodeGenerator service-loading failure; FileIndexPredicateCloseTest failed before its logic ran because Mockito could not attach to this JVM. The PR head's JDK 8/11, Flink 1/2, Spark and E2E CI checks are green. No other confirmed correctness regression was found in the format and write/rewrite paths.

Done

@zjw1111
zjw1111 requested a review from JingsongLi September 24, 2026 08:14

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

Re-review of head deb0579a56: the prior FileIndexer binary-compatibility blocker is fixed. The legacy implementation test compiles an indexer against the old (SeekableInputStream, int, int) interface and reads a V1 container through the new default long overload. The feature has end-to-end value: V2 now streams large containers through table writes and index rewrites while V1 remains the default.

[P1] Preserve the existing public container-writer method for older clients. The refactor removes FileIndexFormat.Writer.writeColumnIndexes(Map<String, Map<String, byte[]>>), even though FileIndexFormat.createWriter(...) and Writer remain public. An extension or integration compiled against the current V1 writer can still load, but calling that method on this head throws NoSuchMethodError before it writes any index; selecting V1 does not avoid it. I verified the old method exists in the PR base and ran a small client compiled against its old descriptor with this head's classes: NoSuchMethodError: FileIndexFormat$Writer.writeColumnIndexes(Ljava/util/Map;)V. Please retain a compatibility method that writes the supplied entries and finishes the container, and add an old-binary client test analogous to FileIndexerCompatibilityTest. This is a rolling-upgrade/plugin compatibility issue, separate from the now-fixed reader overload.

Verification: on JDK 8 the common format, old-indexer compatibility, predicate and cleanup tests passed 15/15. On JDK 17 the core DataFileIndexWriterTest, FileIndexProcessorTest, FileIndexesTableTest, and CoreOptionsTest passed 39/39, including >2 GiB paths. The exact-head Spark, Flink and interoperability CI jobs are green; the JDK 8 core CI job was still pending when checked. The writer ABI regression blocks production use.

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

Re-reviewed head a2dadb4ebf. Restoring Writer.writeColumnIndexes(Map) fixes the exact ABI failure I raised last time; the new compiled-old-client test passes. The streaming V2 format and >2 GiB offset work remain valuable. However, the broader pre-existing public container API still has binary breaks.

[P1] Preserve the other published FileIndexFormat entry points before merging. Comparing this head with master, existing binaries can still call FileIndexFormat.createWriter(OutputStream), createReader(SeekableInputStream, RowType), createMetadataReader(SeekableInputStream), new FileIndexFormat.Writer(OutputStream), new FileIndexFormat.Reader(SeekableInputStream, RowType), and Reader.readAll(). This PR removes all six descriptors (and makes the formerly concrete Writer abstract). Such callers will get NoSuchMethodError / InstantiationError at runtime even though the new writeColumnIndexes method exists. FileIndexMeta.sizeInBytes() also changes its JVM return descriptor from int to long, another NoSuchMethodError for compiled clients. These methods are public; older Paimon integrations and external file-index users must be able to load with a new Paimon jar. The new compatibility test covers only writeColumnIndexes, so please extend it with old bytecode invoking the factory/reader methods and metadata accessor, and retain compatibility adapters (or take an explicit versioned API migration decision). The two-argument reader needs a defined way to obtain length for V2; retaining its V1 behavior is sufficient for old callers.

Local verification on this head: JDK 8 FileIndexFormatCompatibilityTest, FileIndexerCompatibilityTest, FileIndexFormatFormatTest 12/12; JDK 17 DataFileIndexWriterTest and FileIndexProcessorTest 11/11. These green tests do not exercise the missing old descriptors. Affected GitHub CI is still pending at review time.

@JingsongLi

Copy link
Copy Markdown
Contributor

Re-reviewed head 24ee618 for production. The V2 streamed file-index container has clear value for payloads above 2 GiB. The latest commit restores the old FileIndexFormat factory methods, public writer/reader constructors, Reader.readAll(), and the int JVM descriptor of FileIndexMeta.sizeInBytes(). The new compatibility test compiles an old client against the old signatures and successfully invokes it with the new runtime, addressing my previous P1 ABI finding. The V1 default remains available; V2 still needs the documented reader-first rollout.

Local JDK 8 verification passed: common format and compiled-old-client compatibility suites 12/12; core index writer, rewrite processor, and system table suites 18/18, including the streamed writer path. git diff --check passed. Exact-head JDK 8/11, Flink, Spark, E2E, docs and licensing CI are all green. I found no remaining production blocker in the changed paths.

@JingsongLi

Copy link
Copy Markdown
Contributor

[P2] Avoid an extra file-status lookup for V1 indexes

FileIndexPredicate(Path, ...) now calls fileIO.getFileStatus(path) before opening every independent index file. FileIndexEvaluator constructs this predicate per indexed data file during filtered scans. Since V1 remains the default and its header can be read without the container length, existing V1 scans incur an additional metadata lookup per index file, which can be a remote request on object stores. Only V2 needs the length to locate its trailer.

Could we determine the container version first and obtain the length only for V2 (or reuse a length already available to the caller)? That would keep the V1 read-path cost unchanged.

Location: paimon-common/src/main/java/org/apache/paimon/fileindex/FileIndexPredicate.java:61-64.

@zjw1111

zjw1111 commented Sep 26, 2026

Copy link
Copy Markdown
Contributor Author

[P2] Avoid an extra file-status lookup for V1 indexes

FileIndexPredicate(Path, ...) now calls fileIO.getFileStatus(path) before opening every independent index file. FileIndexEvaluator constructs this predicate per indexed data file during filtered scans. Since V1 remains the default and its header can be read without the container length, existing V1 scans incur an additional metadata lookup per index file, which can be a remote request on object stores. Only V2 needs the length to locate its trailer.

Could we determine the container version first and obtain the length only for V2 (or reuse a length already available to the caller)? That would keep the V1 read-path cost unchanged.

Location: paimon-common/src/main/java/org/apache/paimon/fileindex/FileIndexPredicate.java:61-64.

Thank you for the detailed review. I’ve been on vacation these past few days, and I’ll fix this issue after my vacation ends on October 8.

@JingsongLi

Copy link
Copy Markdown
Contributor

Re-reviewed exact head 24ee6182b36f6233a85702997a84c2215c4c58d0. The streaming V2 container has clear production value, and the previously restored FileIndexFormat/FileIndexer compatibility entry points pass. I found one remaining ABI blocker outside those existing compatibility fixtures.

[P1] Preserve these remaining old file-index API descriptors.

  • FileIndexPredicate(SeekableInputStream, RowType) was removed. A JDK 8 client compiled against the actual base implementation reads a valid default V1 bitmap and verifies both a hit and a miss with the base runtime. The identical bytecode on this head throws NoSuchMethodError: FileIndexPredicate.<init>(SeekableInputStream, RowType). Retaining a deprecated two-argument constructor using the existing V1 reader compatibility path fixes the same probe; V2 does not have to be enabled to encounter the failure.
  • CoreOptions.fileIndexInManifestThreshold() and FileIndexOptions.fileIndexInManifestThreshold() changed their return type from long to int. Both old JVM descriptors end in ()J; changing the return type removes them. Separate callers compiled against the actual base sources return the default threshold of 500 with the base runtime, but each throws NoSuchMethodError with this head. The new 0..Integer.MAX_VALUE validation can remain while these public getters keep returning long; narrow the value at the internal spill-buffer boundary.

Please extend the compiled-old-client compatibility test to invoke these three descriptors. Existing compiled integrations cannot resolve them even when they continue to use the default V1 format.

Verification: 61 focused JDK 8 tests passed with normal Maven checks: 16 common format/predicate/plugin compatibility tests and 45 core option, writer, rewrite, evaluator and system-table tests. This includes streamed >2 GiB table-index creation/rewrite, V1/V2 multi-index predicates, and failed-write cleanup. The three additional old-bytecode probes above fail on the current head. Exact-head CI is green. The already-reported V1 extra file-status lookup remains pending as acknowledged in the September 26 reply; I am not duplicating that finding.

@zjw1111
zjw1111 requested a review from JingsongLi October 8, 2026 08:25
} catch (EOFException e) {
return false;
} finally {
input.seek(position);

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.

[P2] Avoid rewinding the V1 index stream after the version probe.

FileIndexPredicate.createReader calls isV2 for every external index. For the default V1 format, this consumes the 12-byte prefix, seeks back to the start, and then the V1 reader reads the prefix again. HadoopFileIO.HadoopSeekableInputStream forwards backward seeks to the underlying filesystem stream; on S3A, that can reopen the object stream and issue another GET per index file during filtered scans. This regresses the existing V1 read path even when V2 writing is disabled. Could we reuse the prefix already read (or replay it from a small buffer) so the V1 reader can continue without a backward seek, while still obtaining the file length only for V2?

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

Requirement fit: SUPPORTED. Streaming large V2 containers through real table-write and index-rewrite paths removes the demonstrated container-size limit. The prior public API/old-plugin compatibility findings and V1 extra file-status/rewind costs are fixed at this head.

Validation: normal Maven checks with JDK 8 passed 63 focused common/core tests, including compiled-old clients, V1/V2 predicates, >2 GiB creation/rewrite and failed-write cleanup. An additional close-failure injection passes for V2 but fails for default V1 as described below. The failing Flink-common CI stopped resolving org.apache:apache:pom:23 before compilation/tests, so it does not supply a functional failure result.


@Override
public void close() throws IOException {
IOUtils.closeQuietly(dataOutputStream);

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.

[P2] Propagate errors closing spilled V1 index files

The new DataFileIndexWriter/FileIndexProcessor paths pass SpillableIndexOutputStream directly to this writer, including for default V1. Once the container spills, closing it finalizes a real FileIO output; closeQuietly swallows an upload/flush failure and the caller proceeds to publish successful metadata naming an absent or partial index. Filtered reads open that file without a missing-index fallback. Previously the V1 container was assembled in memory and the external file output was closed separately by try-with-resources, which propagated the failure. A parameterized regression wraps actual local FileIO output with a close() that throws after accepting writes: DataFileIndexWriter.close() incorrectly succeeds for V1, whereas V2 raises and deletes the partial file. Please use normal dataOutputStream.close() so the existing abort handling prevents publication and cleans up the failed index, and cover both write and rewrite paths.

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.

3 participants