Skip to content

bug: expression_parser deterministic checks miss context-dependent expressions after TypeChecker rewrites them to constants #19833

Description

@forsaken628

Summary

Some expression parser paths validate determinism after TypeChecker::resolve, but TypeChecker rewrites session/catalog/auth/settings-dependent sugar functions into literals
before the deterministic check runs.

As a result, expressions such as current_database(), current_user(), getvariable(...), last_query_id(...), and timezone() can become ConstantExpr/Expr::Constant
first, so later Expr::is_deterministic() no longer sees that the original expression depended on query/session context.

This is especially risky for persisted or storage-level expressions, such as computed columns, cluster keys, and pruning/filter expressions.

Evidence on main

In src/query/sql/src/planner/expression/expression_parser.rs:

  • parse_computed_expr_to_string resolves the AST first, then checks computed_expr.is_deterministic(...).
  • analyze_cluster_keys resolves the AST first, then checks expr.is_deterministic(...).
  • parse_computed_expr, parse_exprs, parse_to_filters, and parse_cluster_keys use full TypeChecker::try_create(...) and do not consistently enforce a context-
    independent policy.

In src/query/sql/src/planner/semantic/type_check.rs:

  • resolve_function calls try_rewrite_sugar_function(...) before normal scalar function resolution.
  • Context sugar functions are rewritten to literals, for example:
    • current_catalog() -> current catalog string
    • current_database() -> current database string
    • current_user() -> current user string
    • connection_id() / client_session_id() -> session values
    • timezone() -> setting value

In src/query/expression/src/expression.rs, Expr::is_deterministic() only checks function calls against FunctionProperty::non_deterministic. Once a context function has
already been rewritten to a constant, this check cannot distinguish it from a real literal.

Problem

There are two different concepts currently mixed together:

  1. Function determinism inside common_expression::Expr
    Example: rand() and now() are non-deterministic because they remain function calls with non_deterministic properties.

  2. Context independence / persistability
    Example: current_database() may be folded to a constant during binding, but the value depends on the binding session. It should not be accepted in persisted schema
    expressions or storage-level expressions just because it becomes a literal before Expr::is_deterministic().

The current checks only reliably cover the first concept.

Examples of problematic shapes

These should be rejected for persisted/context-independent expression use, but the current architecture can lose the original dependency before validation:

  • computed column expression using current_database() or getvariable('x')
  • cluster key expression like concat(to_string(a), current_database())
  • storage/pruning filter expression using context-dependent sugar
  • any expression parser path that converts full SQL expressions into reusable common_expression::Expr without preserving context dependencies

Expected behavior

Expression parser entry points should choose an explicit policy before resolving:

  • Full query binding may allow context-dependent functions and fold them for the current query.
  • Persisted/schema/storage expression parsing should reject:
    • catalog/session/auth functions
    • session variables
    • last_query_id
    • settings-dependent values such as timezone() if the expression is meant to be portable/persisted
    • ordinary non-deterministic functions such as rand(), now(), etc.

Pure sugar rewrites such as coalesce, decode, greatest/least, to_variant, and decode-string helpers can still be allowed if their arguments and resolved functions are
deterministic.

Suggested fix direction

Add a pre-resolution or lowering-level capability check for expression parser paths that require context-independent expressions.

The check should run before context-dependent sugar functions are folded into constants, or it should preserve dependency information through lowering. Relying only on
common_expression::Expr::is_deterministic() after type checking is not sufficient.

Possible implementation shape:

  • introduce an expression policy such as ContextIndependent / Persistable
  • have TypeChecker or its adapter reject context-dependent sugar/functions under that policy
  • keep existing Expr::is_deterministic() for normal function-property validation
  • apply the stricter policy to computed columns, cluster keys, storage/pruning expressions, and similar expression_parser entry points
  • add regression tests that assert these are rejected before being folded to constants:
    • computed column: current_database()
    • computed column or cluster key: getvariable('x')
    • cluster key: concat(to_string(a), current_database())
    • storage filter parser: a > rand()

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