LuciferYang opened a new issue, #9581:
URL: https://github.com/apache/paimon/issues/9581

   ### Search before asking
   
   - [x] I searched in the [issues](https://github.com/apache/paimon/issues) 
and found nothing similar.
   
   ### Paimon version
   
   master, `2788fe596` (2.1-SNAPSHOT).
   
   ### Compute Engine
   
   Flink and Spark. Not reachable through any current call site, see below; 
this is about the class's contract rather than an observed failure.
   
   ### Minimal reproduce step
   
   `OrcWriterFactory.getWriterOptions()` caches the options in a field:
   
   ```java
   private OrcFile.WriterOptions writerOptions;
   
   protected OrcFile.WriterOptions getWriterOptions() {
       if (null == writerOptions) {
           writerOptions = OrcFile.writerOptions(writerProperties, 
configuration());
           writerOptions.setSchema(this.vectorizer.getSchema());
       }
       return writerOptions;
   }
   ```
   
   and `create()` writes per-file state into whatever it gets back:
   
   ```java
   opts.compress(CompressionKind.valueOf(compression.toUpperCase()));
   opts.physicalWriter(new PhysicalFsWriter(new FSDataOutputStream(out, null) 
{...}, opts, ...));
   ```
   
   `WriterImpl` reads the physical writer out of the options in its 
constructor, so two `create()` calls on one factory racing between 
`opts.physicalWriter(...)` and `new WriterImpl(...)` can both end up with the 
second call's writer, i.e. both writers appending to one output stream. Two 
interleaved ORC streams in one file, and a zero-length file where the other one 
was meant to go, with both data files still committed with their row counts and 
statistics.
   
   ### What doesn't meet your expectations?
   
   Nothing today reaches it: each rolling writer builds its own 
`FormatWriterFactory`, and `RollingFileWriterImpl` rolls files strictly one at 
a time, closing the current writer before opening the next. The reason to 
remove it anyway is that factory sharing in this shape is what FLINK-27070 was 
about, and that one was fixed by making the call sites build a writer factory 
per writer rather than by changing this class. The convention that 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 was the only implementation of it that kept per-file 
state in a field.
   
   Removing the field does not make the factory safe to share either, since the 
`Vectorizer`'s `TypeDescription` is still common to every writer a factory 
creates. It removes one specific trap, not the requirement.
   
   ### Anything else?
   
   The field is not buying much. Building the options per call costs one Hadoop 
`Configuration` and one `WriterOptions` per file, which is noise next to 
writing the file, and `createWithShreddingWritePlan` already builds a 
`Configuration` per call.
   
   ### Are you willing to submit a PR?
   
   - [x] I'm willing to submit a PR!
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to