Skip to content

Physical plan check does not cover appended optimizer rules and does not name the rule that broke the plan #26460

Open

Description

@jayzhan211

Is your feature request related to a problem or challenge?

SanityCheckPlan checks that every node's required input ordering and distribution are satisfied, and that no pipeline-breaking operator runs on an unbounded input. Because it is a rule at the end of PhysicalOptimizer::new(), three things go wrong:

  1. Rules appended by the user are never checked. SessionStateBuilder::with_physical_optimizer_rule appends after SanityCheckPlan. A rule that breaks a requirement then produces wrong results instead of an error. On main, appending a rule that removes SortExecs:

    let state = SessionStateBuilder::new()
        .with_default_features()
        .with_physical_optimizer_rule(Arc::new(RemoveSorts))
        .build();
    let ctx = SessionContext::new_with_state(state);
    ctx.sql("CREATE TABLE t(a INT) AS VALUES (3), (1), (2)").await?.collect().await?;
    ctx.sql("SELECT a, row_number() OVER (ORDER BY a) AS rn FROM t").await?.show().await?;
    +---+----+
    | a | rn |
    +---+----+
    | 3 | 1  |
    | 1 | 2  |
    | 2 | 3  |
    +---+----+
    

    The window runs over unsorted input, and the plan is never checked.

  2. Custom pipelines keep the rule last by hand. For example, GreptimeDB removes SanityCheckPlan and pushes it again after its own rules, and Spice inserts its rules relative to its position. A custom rule list that leaves it out runs plans that do not satisfy their requirements.

  3. The error does not say which rule broke the plan. It only says SanityCheckPlan, so each report needed a manual bisect over the rules. SanityCheckPlan error: a constant sort key is lost when a projection reads a duplicate alias #26391 (ProjectionPushdown), Equivalence properties lost if projection is pushed down in FileScanConfig #17077 and LEFT JOIN with struct-field ORDER BY fails ordering validation #25921 are recent examples.

The physical planner already checks check_invariants(InvariantLevel::Executable) after the last rule, for every pipeline. SanityCheckPlan repeats that walk.

Describe the solution you'd like

Run the SanityCheckPlan check in the physical planner after the last rule, in place of the existing InvariantChecker(Executable) walk, and drop the rule from the default list. When ordering or distribution requirements fail, name the rule after which they stopped holding. Only plans from the last EnsureRequirements run onwards need to satisfy them, so only those are searched.

This makes the validity half of option 3 in #26171 checked for every pipeline: a custom pipeline must keep the plan valid, and if it does not, it fails at planning time and names the rule. It also gives the alternative-pipeline sqllogictest mode proposed there a check that does not depend on where SanityCheckPlan sits in the list.

Describe alternatives you've considered

  • Insert appended rules before SanityCheckPlan. This fixes 1 for appended rules only, not for custom rule lists or the error message.
  • An invariant region API (PoC: Explicit Invariant Regions in the Physical Optimizer #26360). This PR follows the review there: one rule (EnsureRequirements) makes the plan valid, every later rule keeps it valid, and that is always checked.
  • A config option to skip the check. A plan that fails it can return wrong results, so I would rather not add one.

Additional context

Related: #26171, #26351, #26360, #25688.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions