XiaoHongbo-Hope commented on PR #8930: URL: https://github.com/apache/paimon/pull/8930#issuecomment-5166132114
> Thanks for working on this. I think this PR needs two changes before merge: > > 1. Please complete snapshot pinning for every reader before advertising deletion-vector support. `supportsDeletionVectors()` is a factory-wide capability, but snapshot pinning is currently incomplete. The core and Flink readers assign `planSnapshot = plan.snapshot()`, while `SparkDataEvolutionVectorRead#read` overrides the method without doing so. PyPaimon's `VectorSearchScanPlan` also only carries splits, and its live-row filter creates a new scan against the latest snapshot. If materialized-deletion compaction commits between planning and reading, an old index plan can be combined with the new row-id/DV state, producing missing or incorrect top-K results. Please thread the planned snapshot through Spark and PyPaimon as well, including the live-row/scalar filters, and add regression coverage for this race. > 2. This PR should also support `paimon-vector`. `paimon-vector` is the more important/current native vector-index implementation; Lumina is only the legacy index. `NativeVectorGlobalIndexReader` already honors `includeRowIds` for both single and batch search, so `NativeVectorGlobalIndexerFactory` should opt in to deletion-vector support after the snapshot-pinning issue above is fixed, with tests covering the native index types on DV tables. Limiting this change to Lumina leaves the primary implementation unsupported and makes the feature incomplete. > > I consider both items part of this PR rather than follow-ups. The `FileStoreCommitImpl` refresh still makes sense because it closes the preparation-to-commit/retry race. Thanks for the detailed review. Both requested changes are addressed in https://github.com/apache/paimon/commit/6363ee86a6ce0885bfb901dfe985902cb5164809: -- 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]
