Gabriel39 commented on PR #66831: URL: https://github.com/apache/doris/pull/66831#issuecomment-5315331868
I did not find a blocking correctness, memory-safety, or concurrency issue in the current patch. I have three review suggestions: 1. Please update the PR title, description, and verification section to cover the second Iceberg commit. The current metadata describes only the unity-skip fix, while the PR now also changes FE `COUNT(*)` capability logic and tests. Please include the targeted Iceberg connector test and Checkstyle results for the final head. 2. Could we extract the exact-count proof shared by `canProveCountFromManifests` and `planCountPushdown` into one helper that returns `OptionalLong`? Repeating the delete gate, position-delete handling, aggregate validation, and subtraction creates two sources of truth. If they drift, the capability could return `true` while actual planning falls back to a normal file scan, which is precisely the mismatch the pre-planning capability is meant to prevent. 3. The documented complexity is not strictly `O(manifests)` when delete state is not `NONE`: `livePositionDeleteRowCount` opens delete manifests and iterates their live delete-file entries. Please document that cost accurately, and consider avoiding it in a pre-planning capability probe for delete-heavy tables. -- 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]
