Skip to content

fix: metrics SQL compiles IS FALSE / IS NOT FALSE identically to IS TRUE#9712

Merged
nishantmonu51 merged 1 commit into
mainfrom
nishant/fix-metrics-sql-is-false
Jul 20, 2026
Merged

fix: metrics SQL compiles IS FALSE / IS NOT FALSE identically to IS TRUE#9712
nishantmonu51 merged 1 commit into
mainfrom
nishant/fix-metrics-sql-is-false

Conversation

@nishantmonu51

Copy link
Copy Markdown
Collaborator

parseIsTruthOperation in the metrics SQL filter parser never inspected ast.IsTruthExpr.True, which distinguishes IS TRUE from IS FALSE. As a result, flag IS FALSE compiled to the exact same expression as flag IS TRUE (and IS NOT FALSE the same as IS NOT TRUE), silently inverting boolean filters in metrics SQL queries, canvas dimension_filters, and SQL-based where expressions.

  • Branch on node.True so IS FALSE compiles to eq(expr, false) instead of eq(expr, true).
  • Compare against a typed boolean value instead of the string 'TRUE', which relied on implicit string-to-boolean casts that are not reliable across ClickHouse/Druid.
  • IS NOT TRUE / IS NOT FALSE now compile to expr != <bool> OR expr IS NULL, matching SQL semantics where the negated forms include NULL rows (a plain != excludes them).
  • Added parser tests for all four combinations; all four fail against the previous implementation.

Steps to reproduce manually:

  1. In a Rill project with a metrics view mv that has a boolean dimension flag, run a metrics SQL query (e.g. via a metrics_sql resolver in a custom API, or a canvas widget's dimension_filters):
    SELECT flag, count FROM mv WHERE flag IS FALSE
  2. Before this fix, the result contains only rows where flag is true; the same query with IS TRUE returns identical results. After the fix, IS FALSE returns the false rows.
  3. The inversion is also visible at the parser level:
    expr, _ := metricssql.ParseFilter("flag IS FALSE")
    b, _ := json.Marshal(expr)
    fmt.Println(string(b))
    // before: {"cond":{"op":"eq","exprs":[{"name":"flag"},{"val":"TRUE"}]}}  (identical to IS TRUE)
    // after:  {"cond":{"op":"eq","exprs":[{"name":"flag"},{"val":false}]}}

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

parseIsTruthOperation ignored ast.IsTruthExpr.True, so IS FALSE compiled
to the same expression as IS TRUE and IS NOT FALSE to the same as
IS NOT TRUE, silently inverting boolean filters.

Also compare against a typed boolean instead of the string 'TRUE',
and include NULL rows in the negated forms (IS NOT TRUE / IS NOT FALSE)
to match SQL semantics.
@nishantmonu51

Copy link
Copy Markdown
Collaborator Author

@codex: review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: c48fd4bae2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@begelundmuller begelundmuller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nishantmonu51
nishantmonu51 merged commit ac7f6c2 into main Jul 20, 2026
16 checks passed
@nishantmonu51
nishantmonu51 deleted the nishant/fix-metrics-sql-is-false branch July 20, 2026 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants