JingsongLi commented on PR #8423:
URL: https://github.com/apache/paimon/pull/8423#issuecomment-4890406365

   Thanks for the update. The main chain-table lookup path looks good to me 
now, and the new tests cover the previous branch-option and delta-snapshot 
refresh issues.
   
   I found one remaining concern: `LookupFileStoreTable.newStreamScan()` 
creates `new ChainTableStreamScan(chainGroupReadTable)` directly for 
chain-table lookup, which bypasses the validation in 
`ChainTableFileStoreTable.newStreamScan()` for unsupported `scan.mode` and 
`consumer-id`. As a result, a lookup join on a chain table may silently ignore 
settings such as `scan.mode=compacted-full` or `consumer-id`, while normal 
chain-table streaming would reject them. Could we either reuse 
`wrapped.newStreamScan()` here, or apply the same validation before creating 
`ChainTableStreamScan`? It would also be good to add a regression test for 
lookup join with unsupported `scan.mode` / `consumer-id`.
   
   Minor doc suggestion: the lookup-join limitations should also mention that 
partition filters / `scan.partitions` are not supported for chain-table lookup, 
otherwise users may think only partition keys in the join condition are 
forbidden.


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