924060929 commented on PR #65548: URL: https://github.com/apache/doris/pull/65548#issuecomment-4998390770
Thanks for the fix — the `COUNT(*)` vs `COUNT(col)` correctness change looks solid. Threading the explicit argument slots through thrift (absent = old FE → normal scan, empty = `COUNT(*)`, non-empty = `COUNT(col)`) is a clean way to stop BE from inferring semantics off the post-pruning scan shape. One request before merge: **the title and description undersell the scope.** The title `[fix](be) Harden FileScannerV2 schema and reader edge cases` only claims the BE / FileScannerV2 half. But this PR also carries a cross-cutting **FE + thrift + BE** change: - a new thrift field `TPlanNode.push_down_count_slot_ids` (a protocol change / rolling-upgrade surface); - a Nereids rule change in `AggregateStrategies` (carry the COUNT argument `ExprId`s, and disable pushdown for `COUNT(CAST(...))` on file scans); - split-planning changes in `HiveScanNode` / `IcebergScanNode` / `PaimonScanNode` / `TVFScanNode`. That half fixes a real wrong-result bug (external `COUNT(*)` returning the non-null count of a retained placeholder column — 9015 vs 10000), which is arguably the most user-visible part of the PR, yet it's invisible from the title and buried mid-description. Could you: 1. Retitle so both areas are discoverable, e.g. `[fix](scanner)(nereids) Harden FileScannerV2 edge cases and fix external COUNT(*)/COUNT(col) pushdown semantics`; and 2. Lead the description with the COUNT-pushdown fix (symptom → root cause → the absent/empty/non-empty contract), then the FileScannerV2 edge cases. Even better would be to split the COUNT-pushdown fix (FE + thrift + BE aggregate paths) into its own PR — it's logically independent from the reader hardening and would be far easier to review, bisect, and back-port. Not a blocker, but at minimum the title/description should name the thrift + FE change so it's findable later. -- 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]
