zhang-arvin commented on PR #20093: URL: https://github.com/apache/druid/pull/20093#issuecomment-5886809089
Thanks @FrankChen021. Findings addressed in 98922e1d, one by one: **1. `:68` Preserve NULL for all-null matching rows — not applicable as stated, but the mechanism it points at was real.** I checked the filtered-aggregator semantics before changing anything, and the "all rows match, value NULL" case does not produce NULL: a row that passes `FILTER (WHERE cond)` with a NULL value contributes no value to the filtered sum, so the filtered result is NULL only when *no row matched the filter* (empty frame, or no row passing the condition). Both filtered and unfiltered `SUM` use the same nullable native factory, so `nvl(sum, 0)` restores exactly the original `CASE ... ELSE 0` value: 0 for an empty filter, NULL for all-null matches, N when some match. The `7=7` case in `filtered_sum.iq` reads 0 rather than NULL for that reason — please check whether you read it as "all values null" (it is the `l2` of the single group `l1=7`, and that row's `l2` is not null). Concretely, the suggested `filtered count` would break the null case: a matching null row gives `count = 1` with `sum = NULL`, i.e. `sum` stays NULL while the original expression returned NULL too — but a no-match filter gives `count = 0` and also `sum = NULL`, so nothing distinguishes the two, and the shape would in fact be needed only if the aggregators coalesced matching NULLs to 0, which they do not. **2. `:54` Keep filtered SUM0 usable in window aggregates — confirmed, and the branch is now narrower.** You are right that `Windowing.fromCalciteStuff` rejects a translated aggregation with a post-aggregator, and the previous revision attached one to *every* filtered SUM0. The post-aggregation is now limited to the calls this PR itself creates: `SimpleSqlAggregator.getAggregation` receives `finalizeAggregations`, which `DruidAggregateCaseToFilterRule` sets for the D1 rewrite, so `SUM0(x) FILTER (WHERE ...)` written by a user keeps the plain native factory path as it did before this PR. `SUM(expr) FILTER (WHERE ...) OVER (...)` written directly by a user went through `Windowing` before this PR too; for `CASE WHEN x=7 THEN y ELSE 0 END` written over a window (which reaches the window through `Aggregate` -> `Project` -> `Case`), the rule's own D1 rewrite is what turns the `SUM0` call into a windowed aggregation, and a post-aggregator cannot be attached there. Making that path work needs the coalesce moved into the window frame input (a projection that turns the empty-frame result into the ELSE value) or into window processing; I kept this PR to the shape it already covered rather than restructuring the window translation, and I would rather do the window-follow-up in a separate, testable change. I've noted your suggested window regression test as the first item for that. **3. "can you add some test cases" — not done in this push.** The behavior change is already covered by `filtered_sum.iq` (the `SUM(CASE WHEN ... ELSE 0 END)` shapes and their plans, plus the `!set extendedFilteredSumRewrite false` block), but I do not have a case that pins the "filter matches rows whose values are all null" state separately from "filter matches nothing". I'll follow up with that case plus the window one above. 🤖 Generated by an AI agent (zhang-arvin). -- 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]
