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

   ### Search before asking
   
   - [X] I searched in the [issues](https://github.com/apache/paimon/issues) 
and found nothing similar.
   
   ### Paimon version
   
   master, `fcae20748a777fa72763d7a0f046482cc9ceb6f8`
   
   ### Compute Engine
   
   Java API for the reproduction below. The production call sites are Spark — 
`PaimonSparkWriter.scala:435` and `PaimonDeltaWriteBase.scala:206` — and 
Flink's `DataEvolutionDeleteOperator:241`.
   
   ### Minimal reproduce step
   
   1. Create an append table with `deletion-vectors.enabled = true` and 
`deletion-vectors.bitmap64 = false` (the default).
   2. Delete some rows, so a 32-bit deletion vector is written for a data file.
   3. `ALTER TABLE ... SET ('deletion-vectors.bitmap64' = 'true')` — accepted, 
the option is not `@Immutable`.
   4. Delete more rows from the same data file.
   
   Step 4 fails:
   
   ```
   java.lang.RuntimeException: Only instance with the same class type can be 
merged.
       at 
org.apache.paimon.deletionvectors.Bitmap64DeletionVector.merge(Bitmap64DeletionVector.java:74)
       at org.apache.paimon.deletionvectors.append.AppendDeleteFileMaintainer
              .notifyNewDeletionVector(AppendDeleteFileMaintainer.java:134)
   ```
   
   Reproduced at the maintainer level, which is the same call the Spark writer 
makes:
   
   ```java
   // deletion vectors written with bitmap64 = false
   TestAppendFileStore store = TestAppendFileStore.createAppendStore(tempDir, 
options);
   CommitMessageImpl cm = store.writeDVIndexFiles(
           BinaryRow.EMPTY_ROW, 0, Collections.singletonMap("f1", 
Arrays.asList(1, 3)));
   store.commit(cm);
   AppendDeleteFileMaintainer m = 
store.createDVIFMaintainer(BinaryRow.EMPTY_ROW, map);
   
   DeletionVector.read(LocalFileIO.create(), map.get("f1")).getClass()
   // -> BitmapDeletionVector
   
   // what the writer produces once the option is flipped
   DeletionVector fresh = new Bitmap64DeletionVector();
   fresh.delete(7);
   m.notifyNewDeletionVector("f1", fresh);
   // -> RuntimeException: Only instance with the same class type can be merged.
   ```
   
   And the option change in step 3 is accepted:
   
   ```
   manager.commitChanges(SchemaChange.setOption("deletion-vectors.bitmap64", 
"true"))
   // -> accepted, latest().options().get("deletion-vectors.bitmap64") == "true"
   ```
   
   ### What doesn't meet your expectations?
   
   The two halves pick the implementation from different places and nothing 
reconciles them:
   
   * `DeletionVector.read` (`DeletionVector.java:101-148`) dispatches on the 
**magic number in the file**, so a vector already on disk keeps its width no 
matter what the table option now says;
   * new vectors are created from the **table option** — 
`BucketedDvMaintainer:51` and 
`DataEvolutionCompactDeletionVectorRewriter:231-233`;
   * `merge` on both implementations rejects the other outright:
   
   ```java
   // Bitmap64DeletionVector:70-76, and the mirror image in 
BitmapDeletionVector:58-64
   if (deletionVector instanceof Bitmap64DeletionVector) {
       roaringBitmap.or(((Bitmap64DeletionVector) 
deletionVector).roaringBitmap);
   } else {
       throw new RuntimeException("Only instance with the same class type can 
be merged.");
   }
   ```
   
   `deletion-vectors.bitmap64` carries no `@Immutable` annotation, so it is not 
in `CoreOptions.IMMUTABLE_OPTIONS` and `ALTER TABLE ... SET` lets it through. 
Since the option's documented purpose is Iceberg compatibility — *"only 64 bit 
bitmap implementation is compatible with Iceberg"* — turning it on for a table 
that already exists is exactly what someone would want to do.
   
   Note that `BucketedDvMaintainer.notifyNewDeletion(fileName, position)` does 
not hit this: it goes through `computeIfAbsent`, so it keeps appending to 
whatever object was loaded from disk and quietly preserves the old width. Only 
the paths that build a fresh vector and then merge the previous one into it 
fail.
   
   ### Anything else?
   
   Which way this should be resolved looks like a project decision rather than 
something to just patch, so I have not opened a PR:
   
   1. **Mark the option `@Immutable`.** Smallest change, and it makes the 
failure a clear error at `ALTER` time instead of at the next delete. It also 
closes off migrating an existing table to the Iceberg-compatible format, which 
may be the main reason to touch the option at all.
   2. **Let `merge` widen.** 
`Bitmap64DeletionVector.merge(BitmapDeletionVector)` can be total — every 
32-bit position fits in 64 bits — so a flipped table would migrate file by file 
as deletes touch it. The reverse is not total: 
`BitmapDeletionVector.merge(Bitmap64DeletionVector)` has to reject positions 
above `Integer.MAX_VALUE`, so turning the option back off cannot be symmetric.
   3. **Leave the behaviour and improve the message**, naming the option and 
the file so the cause is recoverable from the exception alone.
   
   Happy to put up a PR for whichever of these you would prefer.
   
   ### 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