talatuyarer commented on PR #17280:
URL: https://github.com/apache/iceberg/pull/17280#issuecomment-5692154405

   Thanks for the updates, @Guosmilesmile. I apologize for the delay. After 
reviewing the PR with a long-term perspective, I would like to propose a 
structured roadmap rather than continuing with a line-by-line review.
   
   The core architecture is sound: using a custom LookupFunction over an 
IcebergLookupReader that pins a snapshot per load while pushing projections and 
filters into the scan. There is too much change. I suggest keeping the 
implementation memory-only and breaking the PR into three smaller, manageable 
chunks:
   
   I recommend treating this PR as Phase 1 of a roadmap toward robust, scalable 
lookup joins.
   For this initial phase, I suggest keeping the implementation memory-only and 
breaking the PR into three smaller, manageable chunks:
   * Chunk 1: Reader and heap store (no reload).
   * Chunk 2: Reload functionality.
   * Chunk 3: Documentation.
   
   And this is not a small feature. If you can create a design doc to explain 
little bit it will be useful for everybody. IN my mind there a roadmap like 
this:
   1. This PR: Memory-only full cache.
   2. Reload support using Flink's CacheReloadTrigger, standardized option 
naming, and a default-fail policy.
   3. FLIP-462 slicing for subtask-specific data distribution.
   4. Catch-up support for append-only tables.
   5. A pure-Java disk-spilling store.
   6. Async lookup/reload and feature parity with Paimon.
   
   This approach also allows us to drop the RocksDB dependency, reuse Flink’s 
standard lookup.* configuration names, and clarify that subtasks reload 
independently—which addresses the multi-slot concerns by clarifying that we 
currently use one cache per slot, with disjoint slices coming later. I also 
checked other table formats which has similar functionallity. Paimon [dropped 
RocksDB support](https://github.com/apache/paimon/pull/8886) recently. And 
Fluss is also not using it. Rocksdb could be need it for CDC for lookup tables 
but Iceberg currently does not support CDC. It is only append only. No need to 
have Rocksdb 
   
   I am happy to review these refactored chunks quickly and collaborate with 
you on the subsequent phases of the roadmap.
   
   cc @pvary 


-- 
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]

Reply via email to