luoyuxia commented on PR #3227: URL: https://github.com/apache/fluss/pull/3227#issuecomment-4881744443
@Kaixuan-Duan Thanks for fixing the empty projection issue. My main concern is the newly added Spark `COUNT` pushdown: before merging, I think it should be aligned with the existing Flink count-pushdown behavior. Flink already has a clear contract in `FlinkTableSource#applyAggregates` / `PushdownUtils.countTable`: only push down a single `COUNT(*)` / `COUNT(1)` / `COUNT(non-null column)` without grouping; do not push down nullable `COUNT(col)`, streaming reads, or lake-enabled reads; and let WAL / legacy PK tables with disabled row count fail with the existing row-count-disabled error. The Spark implementation is partially aligned, but a few cases still need to be clarified or covered: - WAL / legacy row-count-disabled PK tables are not covered by Spark tests. If failure is intended to match Flink, please add an explicit test. - Spark should explicitly avoid aggregate pushdown for streaming reads, matching Flink's `streaming` guard, or add coverage proving Spark will never call this path for streaming. - `FlussCountScan` bypasses the existing Spark PK batch scan startup-mode restriction: normal PK batch reads only support `FULL`, but this fast path can return a result under other startup modes. I think the empty-projection fix can be kept, but Spark `COUNT` pushdown should either be aligned with Flink's contract and tested for these edge cases, or be split out from this 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]
