Doris-Breakwater commented on issue #70553: URL: https://github.com/apache/doris/issues/70553#issuecomment-6846502577
Breakwater-GitHub-Analysis-Slot: slot_7ef68ef46ff9 Source review supports the reported non-converging rewrite and a separate timeout-protection gap. This belongs with the FE/Nereids maintainers and merits high priority because the reported impact extends to FE availability. The issue currently has no labels. I inspected upstream base `1503e390275c7c2bcf814907776491b8cbf3ccc0` in the local repository. I also checked GitHub master at `ccfd4378d424292a363a96142ad795b8c9ace21e`: the three named files have identical Git blob hashes at both revisions. This is source-level verification; I have not built or executed either revision or the patched FE described here. The following mechanism is verified in the reported base: 1. `EliminateAggCaseWhen` requires no grouping, one output expression, and one null-ignoring aggregate with a single `IF` argument whose ELSE is NULL. It replaces the argument with the THEN expression and inserts the condition as a child filter. This establishes the reported aggregate/filter shape. [Source](https://github.com/apache/doris/blob/1503e390275c7c2bcf814907776491b8cbf3ccc0/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/EliminateAggCaseWhen.java#L53-L91). 2. `InferAggNotNull` examines only the immediate child filter, checks both ordinary and generated forms of each inferred predicate, and wraps the existing child with a filter containing only the missing predicates. `PlanUtils.filter` always creates a new filter for a nonempty set; it does not merge filters. Consequently, for inferred predicates `{a IS NOT NULL, b IS NOT NULL}`, an initial child containing only `a IS NOT NULL` produces `{b}`, then `{a}`, then `{b}`, indefinitely. The generated-predicate flag is already accounted for; the loss of visibility into the lower filter is the problem. [Rule](https://github.com/apache/doris/blob/1503e390275c7c2bcf814907776491b8cbf3ccc0/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/InferAggNotNull.java#L43-L67), [filter helper](https://github.com/apache/doris/blob/1503e390275c7c2bcf814907776491b8cbf3ccc0/fe/fe-core/src/main/java/org/apache/doris/nereids/util/PlanUtils.java#L86-L90). 3. The inference rules run top-down before subsequent filter-pushdown processing. `TopDownVisitorRewriteJob` repeatedly rewrites the aggregate until it stops changing, before visiting its children, so downstream filter merging cannot rescue this loop. Its timeout check precedes `doRewrite`; the inner loop contains neither that check nor an iteration bound. [Stage order](https://github.com/apache/doris/blob/1503e390275c7c2bcf814907776491b8cbf3ccc0/fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Rewriter.java#L555-L567), [visitor](https://github.com/apache/doris/blob/1503e390275c7c2bcf814907776491b8cbf3ccc0/fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/rewrite/TopDownVisitorRewriteJob.java#L67-L136). This also explains the control cases: without an existing partial filter, both inferred predicates can be added together; with both already present, nothing is added; grouping excludes this rule. More generally, the problematic shape is an eligible aggregate whose immediate filter contains a nonempty proper subset of its inferred not-null predicates. CASE/IF is one way to create that shape. The proposed PR direction is appropriate: - Merge the missing predicates into an existing child filter, retaining all original conjuncts and its child, following `InferFilterNotNull`. Preserve the existing null-ignoring, no-grouping, and common-predicate safeguards. Assert that a second application makes no change. [Reference implementation](https://github.com/apache/doris/blob/1503e390275c7c2bcf814907776491b8cbf3ccc0/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/InferFilterNotNull.java#L57-L81). - Check the planning deadline inside the repeated rewrite loop and add a bounded non-convergence failure with useful rule diagnostics. Timeout checking alone remains conditional on `enable_nereids_timeout`; the helper uses `nereids_timeout_second`, so testing only `query_timeout` is insufficient. Verify cancellation separately: adding a deadline check does not itself establish that KILL is honored. [Timeout helper](https://github.com/apache/doris/blob/1503e390275c7c2bcf814907776491b8cbf3ccc0/fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/rewrite/RewriteJob.java#L37-L53). - Cover the supplied SELECT/EXPLAIN reproducers, a direct aggregate over a partial filter, retention of unrelated conjuncts, generated and ordinary predicate forms, and result equivalence with inference enabled/disabled on empty, all-NULL, and mixed data. Keep grouped and null-sensitive aggregate controls. Test the visitor guard using a deliberately non-converging test rule, including with planning timeout disabled, and verify that the FE can answer a subsequent query. For mitigation before a fix, the reported session setting `SET disable_nereids_rules = 'INFER_AGG_NOT_NULL'` bypasses the offending rule; preserve any other rules already disabled. Apply it before planning the statement. A global default change should not be treated as a way to unwind a statement already spinning: disabled rules are cached per statement. [Cache access](https://github.com/apache/doris/blob/1503e390275c7c2bcf814907776491b8cbf3ccc0/fe/fe-core/src/main/java/org/apache/doris/nereids/CascadesContext.java#L490-L497). Remaining verification: please attach the existing thread dump and timestamped histogram excerpts, the effective `enable_nereids_timeout` / `nereids_timeout_second` / `disable_nereids_rules` values, and an isolated reproduction on the unmodified upstream base or current master. The reported heap counts, timing, and inability to kill the statement remain runtime observations from the report, not independently reproduced results. The source evidence is sufficient to proceed with the proposed PR; the clean-build run will establish independence from the additional patches. Affected release branches and a fixed version still need verification. -- 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]
