Repository navigation
Conversation
| FileIndexWriter createWriter(); | ||
|
|
||
| FileIndexReader createReader(SeekableInputStream inputStream, int start, int length); | ||
| FileIndexReader createReader(SeekableInputStream inputStream, long start, long length); |
There was a problem hiding this comment.
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.
| try { | ||
| close(); | ||
| } finally { | ||
| if (fileCreated) { |
There was a problem hiding this comment.
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.
JingsongLi
left a comment
There was a problem hiding this comment.
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.
54e8a18 to
deb0579
Compare
Done |
JingsongLi
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
|
Re-reviewed head 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. |
|
[P2] Avoid an extra file-status lookup for V1 indexes
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: |
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. |
|
Re-reviewed exact head [P1] Preserve these remaining old file-index API descriptors.
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. |
| } catch (EOFException e) { | ||
| return false; | ||
| } finally { | ||
| input.seek(position); |
There was a problem hiding this comment.
[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
left a comment
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
[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.
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
writeIndexandFileIndexWriter.writeTo(OutputStream).DataFileIndexWriterandFileIndexProcessorwrite 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 alongpayload 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, andtestV2CopyPayloadOverInt32cover large positions, a single large streamed payload, and copying that payload without a large byte array.testIncrementalWriteAndCopyPayloadcovers V1/V2 incremental writes, andtestV2RejectInvalidTailMagiccovers a damaged trailer.DataFileIndexWriterTest:testSpillableIndexOutputStreamcovers the embed/spill boundary;testV2TableWriterStreamsPayloadOverTwoGiBwrites a table index above 2 GiB;testV2FailedWriteDeletesPartialIndexFilechecks cleanup after a failed write.FileIndexProcessorTest:testV2RewritesContainerOverTwoGiBrewrites a table index above 2 GiB. The map-key rewrite test also runs with V1 and V2.CoreOptionsTest.testFileIndexInManifestThresholdcovers threshold bounds, andFileIndexPredicateCloseTest.testFileStatusFailureDoesNotOpenStreamcovers reader construction failure.FileIndexesTableTestruns embedded and external index cases with both V1 and V2.