[format] Build fresh ORC writer options per created writer - #9583
Merged
JingsongLi merged 2 commits intoSep 4, 2026
Conversation
OrcWriterFactory cached OrcFile.WriterOptions in a field, and create() writes per-file state into it: the compression kind, and a PhysicalFsWriter bound to that call's output stream. Two create() calls on one factory therefore share one options object, and since WriterImpl reads the physical writer out of the options in its constructor, a racing pair can leave both writers pointing at the same stream. Build the options per call instead. No factory is used concurrently today, so this is not reachable: each rolling writer builds its own FormatWriterFactory, and RollingFileWriterImpl rolls files strictly one at a time, closing the current writer before opening the next. It is worth removing anyway, because factory sharing in this shape is what FLINK-27070 was about; that one was fixed by making the call sites build a writer factory per writer rather than by changing this class, and the convention it established is written down nowhere. FileFormat does carry "NOTE: This class must be thread safe", added by that same commit, while FormatWriterFactory says nothing about it, and ORC is the only implementation of it that keeps per-file state in a field. This does not make the factory safe to share: the Vectorizer's TypeDescription is still common to every writer a factory creates. The cost is one Hadoop Configuration and one WriterOptions per file instead of per factory, which is noise next to writing the file, and createWithShreddingWritePlan already built a Configuration per call. The options carry no per-file memory manager: OrcFile.WriterOptions takes the JVM-wide static one, so testNotOverrideInMemoryManager is unaffected. Assisted-by: GLM-5.3
Contributor
|
+1 |
Contributor
Author
|
Thank you @JingsongLi |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
close #9581
OrcWriterFactorycachedOrcFile.WriterOptionsin a field, andcreate()writes per-file state into whatever it gets back:WriterImplreads the physical writer out of the options in its constructor, so twocreate()calls on one factory racing between those two statements can both end up with the second call's writer, i.e. two writers appending to one output stream. This builds the options per call instead.No factory is used concurrently today, so it is not reachable: each rolling writer builds its own
FormatWriterFactory, andRollingFileWriterImplrolls files strictly one at a time, closing the current writer before opening the next. What makes it worth removing is that factory sharing in this shape is what FLINK-27070 was about, and that was fixed by making the call sites build a writer factory per writer rather than by changing this class, so the convention it established is written down nowhere.FileFormatdoes carry "NOTE: This class must be thread safe", added by that same commit, whileFormatWriterFactorysays nothing about it, and ORC was the only implementation of it that kept per-file state in a field.To be clear about the scope: this does not make the factory safe to share. The
Vectorizer'sTypeDescriptionis still common to every writer a factory creates. It removes one trap, it does not establish thread safety.The cost is one Hadoop
Configurationand oneWriterOptionsper file instead of per factory, which is noise next to writing the file, andcreateWithShreddingWritePlanalready built aConfigurationper call. No memory manager is created per file:OrcFile.WriterOptionstakes the JVM-wide static one, which is whytestNotOverrideInMemoryManagerstill means what it did.Tests
OrcWriterFactoryTest.testWriterOptionsNotSharedBetweenCallsasserts two calls return different instances. It fails against the current code withExpected not same: org.apache.orc.OrcFile$WriterOptions@....That pins the mechanism rather than the outcome, so a different fix that kept the field and copied per
create()would fail it. I kept it because it is one line and reaches through the same@VisibleForTestingseam the neighbouring test already uses; a behavioural version would open two writers from one factory, write to each and read both files back, which is worth adding if reviewers prefer it.mvn -pl paimon-format teston JDK 8: 597 tests, 0 failures.spotless:checkandcheckstyle:checkare clean.