liucao-dd opened a new pull request, #16984: URL: https://github.com/apache/iceberg/pull/16984
## Summary - Fix a race in `ManifestFilterManager` where parallel manifest filtering concurrently mutates the shared `deleteFiles` set. - Make `duplicateDeleteCount` thread-safe with `AtomicInteger`. - Add a regression test that exercises concurrent adds to `deleteFiles` during a full overwrite across many manifests. ## Problem `ManifestFilterManager.filterManifests()` filters manifests in parallel via `Tasks.range(...).executeWith(workerPoolSupplier.get())`. During expression-based deletes, each worker thread calls `deleteFiles.add(fileCopy)` on a shared instance-level set backed by `WrapperSet` / `LinkedHashSet`, which is not thread-safe. Under heavy concurrent adds (large overwrites with many manifests), this can corrupt the backing map. Production failures have surfaced as `ClassCastException: LinkedHashMap$Entry cannot be cast to HashMap$TreeNode`, `StackOverflowError`, or hangs. ## Fix Wrap `deleteFiles` with `Collections.synchronizedSet(newFileSet())` so concurrent `add`/`contains` operations from worker threads are safe. `Collections.synchronizedSet` protects individual method calls; iteration over `deleteFiles` must remain after the parallel filtering phase completes (current behavior) or be guarded with `synchronized(deleteFiles)`. `duplicateDeleteCount` is updated with `AtomicInteger` because workers increment it concurrently when duplicate paths are detected. ## Why `Collections.synchronizedSet` instead of a concurrent set - `ConcurrentHashMap.newKeySet()` keyed on raw `DataFile` would change equality semantics. `DataFileSet` / `DeleteFileSet` deduplicate by file location via `WrapperSet`. - A concurrent `WrapperSet` backed by `ConcurrentHashMap.newKeySet()` is feasible but would change insertion-order guarantees for a shared `api/` type used widely outside this path. - `Collections.synchronizedSet(newFileSet())` preserves existing `WrapperSet` semantics with minimal, localized change. Lock contention is negligible relative to manifest I/O on this path. Rejected alternatives: deferred per-task merge (more invasive refactor), per-task result objects (non-standard for this code path). ## Test plan - [x] `./gradlew :iceberg-core:test --tests org.apache.iceberg.TestOverwrite.testFullOverwriteAcrossManyManifests` - [x] `./gradlew :iceberg-core:test --tests org.apache.iceberg.TestOverwrite` - [x] `./gradlew :iceberg-core:test --tests org.apache.iceberg.TestDeleteFiles` - [x] `./gradlew :iceberg-core:test --tests org.apache.iceberg.TestOverwriteWithValidation` - [x] `./gradlew spotlessApply` `testFullOverwriteAcrossManyManifests` disables manifest merging, appends 16 manifests with 64 files each using hash-colliding file locations, then runs a full overwrite with `alwaysTrue()`. This concentrates concurrent adds into the same hash bucket and guards against regressions that remove synchronization from `deleteFiles`. --- **AI Disclosure** - Model: Claude Opus 4.6 - Platform/Tool: Cursor - Human Oversight: partially reviewed - Prompt Summary: Fix concurrent mutation of ManifestFilterManager.deleteFiles during parallel manifest filtering, add regression test, and open upstream PR. Made with [Cursor](https://cursor.com) -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
