feat: AST SQL rewriting

[MazterQyou]

Check List

• Tests have been run in packages where changes have been made if available
• Linter has been run for changed code
• Tests for the changes have been added if not covered yet
• Docs have been added / updated if required

Description of Changes Made

GET ?query= → extract filters from logical plan, no rewriting:
{ "status": "ok", "filters": [ ... ] }

POST { query, add | set | delete | replace } - exactly one op required, else Exactly one of add, set, delete or replace parameters is required:

• add: [filters]: adds each; already-present identical filter = no-op.
• set: [filters]: drops the filter predicates of the outermost WHERE + HAVING, then adds the set. Predicates that are not Cube filters (join conditions, subquery predicates, predicates over computed columns) are kept. set: [] strips all outermost filters. CTEs and subqueries untouched.
• delete: [filters]: attempts deletion; all occurrences of equal filters removed; not-present = tolerated no-op.
• replace: { old: [filters], new: [filters] }: exact set→set; every old must be found (else error); all occurrences replaced. Single old + single new in same clause → in-place, positions preserved; otherwise remove-all + add.

[codecov]

Codecov Report

❌ Patch coverage is 81.63265% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.09%. Comparing base (c4f6627) to head (48b515c).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
packages/cubejs-api-gateway/src/sql-server.ts	0.00%	5 Missing ⚠️
packages/cubejs-api-gateway/src/gateway.ts	90.90%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10604      +/-   ##
==========================================
- Coverage   84.88%   81.09%   -3.79%     
==========================================
  Files         261      502     +241     
  Lines       87535   111391   +23856     
  Branches        0     3923    +3923     
==========================================
+ Hits        74306    90336   +16030     
- Misses      13229    20506    +7277     
- Partials        0      549     +549     

Flag	Coverage Δ
cube-backend	60.73% <81.63%> (?)
cubesql	85.37% <ø> (+0.49%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @MazterQyou's task in 4m 1s —— View job

---

Review complete — 1 high, 3 medium, 4 low. Main concern: unvalidated user values emitted as raw SQL numeric literals (#3722392675).

Full review — findings, praise, and checklist

Findings

#	Sev	Where	Issue
1	High	ast_conv.rs:440	value_to_expr_by_member_type wraps caller-supplied strings in ast::Value::Number, which sqlparser prints verbatim. For number-typed dimensions / numeric measures, equals/notEquals skip the single_numeric_value validation that gt/lt do apply, so values: ["0 OR 1=1"] renders as = 0 OR 1=1. No test covers a numeric member.
2	Medium	gateway.ts:1586	const { old } = replace runs before the replace === null guard → replace: null throws TypeError and surfaces as 500 instead of the intended 400 UserError.
3	Medium	ast_conv.rs:1300	delete is documented as a tolerated no-op when the filter is absent, but modify_sql_ast returns Err when the member is unresolvable — so deleting a filter on a member not projected in the outermost SELECT errors instead of no-op'ing. Breaks idempotent callers.
4	Medium	ast_conv.rs:1203	Re-parses + re-prints the whole (growing) SQL string per filter → O(n²) with n full parser runs, and no cap on array length anywhere from the HTTP body down. Same shape in delete_sql_filters and the replace fallback. Combined with the unrated-limited routes CodeQL flagged, that's a cheap CPU-pin.
5	Low	gateway.ts:1507	Native errors come back as HTTP 200 + {status:"error"}. Matches sql4sql's precedent, but the endpoint now has three failure shapes (200 in-band / 400 / 500).
6	Low	ast_conv.rs:709	alias_for_relation_in_from matches on the last name part only, ignoring schema and the WITH list → a CTE named after a cube is misresolved as CubeTable. Caught later by the verification re-plan, but as an opaque planner error.
7	Low	ast_conv.rs:1085	let _ = plan.accept(&mut visitor) swallows visitor errors, so a truncated filter set becomes the verification oracle — misleading "was not applied" errors, and a false success in delete_sql_filters (inverted check).
8	Low	ast_conv.rs:419	LIKE %/_ escaping relies on the implicit backslash escape with escape_char: None; explicit is dialect-independent.

Non-blocking, no inline comment posted:

• Docs. Two new public REST endpoints with no /docs-mintlify page. The PR checklist has the docs box unticked — worth confirming whether these are intended as public API or internal-only.
• Tests. Rust coverage of the AST layer is good (~19 tests: CTEs, derived tables, filter groups, duplicates, in-place replace, set-operation rejection). What's untested: the validation branches in modifySqlFilters (the four Array.isArray guards, the exactly-one-op rule, the replace shape check), the sql-server.ts/js/index.ts pass-through, and any non-string member type. The four gateway guards are exactly where finding 2 lives.
• filter_key treats values: null and values: [] as identical and a missing member as "", so {} and {member: "", operator: ""} collide. Harmless given the surrounding validation, but the canonicalization is looser than the "perfect match" wording suggests.

What's good

• Clean separation: pure AST manipulation in ast_conv.rs, Neon glue in sql_filters.rs, HTTP in gateway.ts. Each layer is testable on its own, and the Rust tests exercise the AST layer directly without a session.
• The verify-by-replanning pattern in add_filters_and_verify is the right instinct — asserting the rewrite actually landed in the logical plan rather than trusting the string edit.
• spawn_sql_filters_task correctly factors out the promise/deferred dance across all five entry points, with the try_settle_with failure logged rather than silently dropped, and the auth-context construction matches the existing rest4sql/sql4sql precedent exactly.
• Doc comments on the tricky predicates (member_output_column_in_query, apply_action_to_outermost_select) state the invariants precisely, including what deliberately doesn't qualify.
• assertApiScope('sql', …) reuses the existing scope rather than inventing a new one; route middleware ordering matches every other POST in the file.

Checklist

• Gather context (diff, changed files)
• Review ast_conv.rs (core rewriting logic)
• Review native bridge (sql_filters.rs, node_export.rs, js/index.ts)
• Review gateway HTTP endpoints (security, validation)
• Assess test coverage
• Post review

No prior threads of mine on this PR, so nothing to resolve or dedupe against. Findings are from static reading — I did not build the Rust crate or run the test suite.

• branch [`feat-ast-sql-rewriting`](https://github.com/cube-js/cube/tree/feat-ast-sql-rewriting)

[claude]

Claude finished @MazterQyou's task in 4m 9s —— View job

---

Review complete — solid design, but 2 high / 4 medium / 5 low open; nothing from the previous round was addressed in this push.

Full review — expand for findings, prior-round status, and notes

• Gather context (diff, changed files)
• Review ast_conv.rs (core rewriting logic)
• Review native bridge (sql_filters.rs, node_export.rs, js/index.ts)
• Review gateway HTTP endpoints (security, validation)
• Assess test coverage
• Check prior review threads / post review

New this round (4 inline comments posted)

Sev	Where	Issue
High	ast_conv.rs:944-952	add_expr_to_clause AND-appends to an unparenthesized top-level OR clause. sqlparser's Display is not precedence-aware, so WHERE a OR b + filter c renders as a OR b AND c → reparses as a OR (b AND c). Silently changes query semantics, and add_filters_and_verify cannot detect it (the requested filter is in the plan). This file already wraps its own OR combinations in Expr::Nested at lines 364 / 405 / 694 for exactly this reason — the pre-existing clause just doesn't get the same treatment. No test starts from an unparenthesized OR.
Medium	ast_conv.rs:956-968	clause_contains_expr and remove_exprs don't descend through Expr::Nested. WHERE (a AND b) breaks the "already-present = no-op" guarantee for add (duplicate predicate returned as success) and makes delete/replace fail on filters that are plainly present.
Medium	ast_conv.rs:1219-1228	Verification oracle is plan-wide but the rewrite is outermost-SELECT-only. extract_filters_from_plan flattens filters from CTEs/subqueries too, so an identical filter inside a CTE can satisfy the add check for a filter never applied — and conversely makes delete of a CTE-only filter always error, contradicting the documented tolerant no-op. applied is discarded at line 1206.
Low	ast_conv.rs:883	is_direct_member_ref matches bare Identifier on name alone; two joined cubes sharing a dimension name resolve non-deterministically.

Previous round — all 8 threads re-verified, all still apply

Re-affirmed rather than re-posted (unresolved state already signals this):

• High — ast_conv.rs:456 value_to_expr_by_member_type emits caller-supplied strings as raw Value::Number for number-typed members. equals/notEquals skip the parse::<f64>() validation that gt/lt do via single_numeric_value. values: ["0 OR 1=1"] renders straight into the WHERE clause.
• Medium — gateway.ts:1591 replace destructured before the null guard → replace: null throws TypeError and surfaces as 500 instead of 400.
• Medium — ast_conv.rs:1085 let _ = plan.accept(...) swallows visitor errors; a truncated filter set is then used as the verification oracle.
• Medium — ast_conv.rs:1209 per-filter re-parse/re-print is O(n²) with no cap on array length, on routes CodeQL already flags as unrated-limited.
• Low — ast_conv.rs:1306 delete errors instead of no-op'ing when the member isn't resolvable in the outermost SELECT.
• Low — ast_conv.rs:712 a CTE named after a cube shadows it and is misresolved as MemberSource::CubeTable.
• Low — ast_conv.rs:436 %/_ escaped with \ but escape_char: None, relying on the dialect's default.
• Low — gateway.ts:1507 native errors returned as HTTP 200 + in-band {status:"error"}, giving the endpoint three distinct failure shapes (200/400/500).

The two github-advanced-security rate-limiting comments are consistent with every other /v1/* route on this gateway — not a regression introduced here, though the unbounded-array concern above makes these two routes more attractive as a CPU sink than most.

What's good

• Member resolution is deliberately conservative — member_output_column_in_query refuses post-processed and generated columns, and CTEs/subqueries are never rewritten. The docstrings explaining why (lines 571-575, 784-787) are genuinely helpful.
• Round-tripping the rewritten SQL back through convert_sql_to_cube_query and diffing the extracted filter set is a strong correctness net, and it catches most member-resolution mistakes even where the AST logic is imprecise.
• assertApiScope('sql', …) on both routes, and securityContext threaded through to transport.meta() — authorization matches the existing /v1/sql surface.
• Test coverage is broad for a change this size: all four ops, filter groups, nested groups, mixed WHERE/HAVING rejection, CTE non-modification, derived tables, set-operation rejection, duplicate filters. Gaps are the specific shapes called out above (unparenthesized OR, parenthesized AND, numeric-typed members with equals).

Not verified

• Did not build or run the Rust test suite (cargo test -p cubesql) or yarn lint — the sqlparser Display claim underpinning the high-severity finding rests on the vendored fork's behavior, which I inferred from this file's own Nested-wrapping at lines 364/405/694 rather than reading the fork's source (the cargo git checkout is outside the accessible working directory). Worth a one-line unit test to confirm.
• The PR checklist has docs unchecked, and /v1/sql-filters is a new public REST surface with no page under docs-mintlify/. Per the root CLAUDE.md these belong in /docs-mintlify (not /docs).

• [`feat-ast-sql-rewriting`](https://github.com/cube-js/cube/tree/feat-ast-sql-rewriting)

[claude]

Claude finished @MazterQyou's task in 5m 25s —— View job

---

Review complete — every finding from the last two rounds is addressed. 4 new: 1 medium, 2 low, 1 test-coverage.

Full review — expand for findings, prior-round status, and notes

• Gather context (diff, changed files)
• Check prior review threads (dedupe / resolve stale)
• Review ast_conv.rs (core rewriting logic)
• Review native bridge (sql_filters.rs, node_export.rs, js/index.ts)
• Review gateway HTTP endpoints
• Assess test coverage
• Post review

Previous rounds — 12 threads resolved

All 12 of my open threads are fixed in this push, most with a test pinning the behaviour:

Prior finding	Resolution
High — raw Value::Number from caller input (equals skipped gt's validation)	numeric_value_expr + is_numeric_literal (rejects empty, inf/NaN, stray chars) now gate every numeric literal, value_to_expr_by_member_type included. test_modify_sql_ast_numeric_value_validation
High — AND-appending to an unparenthesized top-level OR	add_expr_to_clause wraps Or/Xor clauses in Expr::Nested. test_modify_sql_ast_or_clause_is_parenthesized
Medium — replace destructured before the null guard → 500	Guard moved ahead of the destructure (gateway.ts:1610)
Medium — Nested not descended in clause_contains_expr / remove_exprs	Both recurse now; remove_exprs re-wraps and collapses correctly. test_modify_sql_ast_parenthesized_clause
Medium — let _ = plan.accept(…) swallowed visitor errors	Propagates; extract_filters_from_plan returns Result
Medium — O(n²) re-parse per filter, unbounded array	modify_sql_ast_many parses/prints once for the whole batch; MAX_SQL_FILTERS = 100 in the gateway
Medium — plan-wide oracle vs outermost-only rewrite	applied is now honoured in delete_sql_filters, and the doc comments state the plan-wide caveat explicitly (see one residual case below)
Low — delete errored instead of no-op'ing on an unresolvable member	Remove goes through resolve_filter_expr → Ok(false). test_delete_sql_filters_unresolvable_member
Low — CTE named after a cube misresolved as CubeTable	cte_names threaded through the resolver. test_modify_sql_ast_cte_shadowing_cube_name
Low — %/_ escaped with \ but escape_char: None	Answered with a comment: an explicit ESCAPE takes a rewrite path that yields no Cube filter, so the implicit backslash is deliberate. Reasonable
Low — native errors as HTTP 200 + in-band {status:"error"}	resSqlFilters maps status === 'error' to 400
Low — bare Identifier matched on name alone	allow_unqualified requires the cube to be the sole relation. test_modify_sql_ast_unqualified_ref_in_join

New this round (4 inline comments)

Sev	Where	Issue
Medium	ast_conv.rs:857	CTE name matching is asymmetric — shadow detection lowercases via cte_names, the CTE lookup compares byte-for-byte. WITH Orders AS (…) SELECT … FROM orders (valid Postgres, both fold to orders) resolves in neither branch, so the endpoint rejects it with is not available in the outermost SELECT. A refusal, not a wrong rewrite — but avoidable.
Low	ast_conv.rs:1486	The applied gate closes "filter only in a CTE"; the mirror case is open. A filter present in both the outermost SELECT and a CTE gets applied == true, the CTE copy keeps it in new_keys, and delete errors was not deleted on a rewrite that did exactly what was asked — discarding the correct SQL.
Low	sql_filters.rs:69	parse_filters_arg's cx.throw_error rejects with a plain JS Error, so a malformed filter item ("member": 42) is a 500 with a raw serde message, while every other bad input on this endpoint is a 400 UserError. Same for replace's two arrays.
Test	ast_conv.rs:3053	Three shapes reach the verification oracle untested: measure/HAVING filters (only asserted at the AST level — the likeliest branch to fail verification, and the only one synthesizing a function from agg_type), inDateRange (normalize_filter_value exists because the planner reshapes these values, but nothing tests that the reshaped values still filter_key-match), and any value containing ' (the last remaining literal-injection surface; safety rests entirely on the vendored fork's Display).

Minor, no thread posted: the schema-qualification nit from the CTE thread is still live — alias_for_relation_in_table_factor matches on the last name part, so FROM analytics.Orders is treated as cube Orders. And filter_key still canonicalizes values: null and values: [] identically with a missing member as "", so {} and {member: "", operator: ""} collide; harmless given the surrounding validation.

Nested and/or groups aren't counted toward MAX_SQL_FILTERS, but serde_json's 128-deep parse limit in parse_filters_arg bounds recursion depth before resolve_filter_expr/canonical_filter_json ever recurse, and breadth is linear against a single parse — no report.

What's good

• The fixes are real fixes, not papered-over ones: modify_sql_ast_many restructures the batch path rather than just capping input, and add_expr_to_clause's comment states the precedence invariant it's protecting. Each one landed with a test that would catch a regression.
• numeric_value_expr centralizes the literal-emission rule in one place with a docstring explaining why parse::<f64>() alone is insufficient — exactly the shape that keeps this from re-breaking when a new operator is added.
• resolve_filter_expr vs require_filter_expr is a clean way to express "absent is fine for delete, an error for add" without duplicating the resolution logic, and it's what makes the tolerant-delete contract actually hold.
• Doc comments now carry the awkward caveats rather than hiding them (add_sql_filters on plan-wide verification, member_output_column_in_query on what deliberately doesn't qualify, the like_family_expr note on why ESCAPE is omitted).
• resSqlFilters gives the endpoint one failure shape, and the guard ordering in modifySqlFilters is now uniform across all four ops.

Not verified

• Did not build or run cargo test -p cubesql or yarn lint — findings are from static reading. The sqlparser Display behaviour underpinning the '-escaping note is unverified (the cargo git checkout is outside the accessible working directory), which is part of why a test pinning it is worth having.
• Docs are still unchecked in the PR checklist, and /v1/sql-filters (GET + POST) is a new public REST surface with no page under docs-mintlify/. Per the root CLAUDE.md these belong in /docs-mintlify, not /docs — worth confirming whether these endpoints are intended as public API or internal-only.

• [`feat-ast-sql-rewriting`](https://github.com/cube-js/cube/tree/feat-ast-sql-rewriting)

[claude]

Claude finished @MazterQyou's task in 10m 8s —— View job

---

No blockers — 2 low posted inline, 2 re-affirmed on open threads, nothing high or medium

Review details

• Gather context (PR diff, all 13 changed files read in full)
• Review Rust: ast_conv.rs (non-test 1–3268 read in full), sql_filters.rs, auth.rs, node_export.rs, rest4sql.rs
• Review TS: gateway.ts, sql-server.ts, js/index.ts
• Review tests (sql-filters.test.ts, the 60-odd Rust tests) and docs (reference.mdx)
• Handle prior review threads
• Post findings

Posted inline

#	Where	Finding
1	packages/cubejs-api-gateway/src/gateway.ts:1588	resSqlFilters turns every in-band native error into a 400. handle_update (sql_filters.rs:152) catches all CubeErrors from the rewrite path, so an internal failure — a rewrite that makes the query unplannable, try_as_logical_plan() failing — is answered as a bad request and never surfaces as a 5xx. CubeError's user/internal distinction already exists and could be carried across the boundary.
2	packages/cubejs-backend-native/js/index.ts:182	SqlFiltersResponse.status typed string rather than 'ok' | 'error'; the gateway's result.status === 'error' check gets no compile-time help and the test has to cast to build an error response.

Re-affirmed, not re-posted

• auth.rs:106-115 — parse_security_context_arg maps a present-but-not-a-JsString argument to None, i.e. plans the query as an unauthenticated call, which is exactly the downgrade the function's own doc comment says must throw. Folded into the still-open thread that raised the same downgrade (.parse().ok() was fixed; the downcast arm is the surviving instance). Not reachable from today's JS callers, which always stringify.
• ast_conv.rs doc comments — several still run 4–6 lines (MAX_RELATION_DEPTH:868, MAX_EXPR_NESTING:876, expr_key:2048, push_word:2119, MAX_FILTERS:2926, plus MAX_CLAUSE_PREDICATES:1443, MatchContext:2030, report_filter:2531, delete_sql_filters:3121). The existing thread on this is still live and covers them; most of the content is load-bearing "why this value", so this is a trim, not a deletion.

Looked at and cleared

• Value rendering / injection. String values go through ast::Value::SingleQuotedString (escaped on render); numerics are validated by is_numeric_literal before being written verbatim, and it rejects inf/NaN/whitespace/1-1 via the char set plus a finite f64 parse; booleans go through parse::<bool>(). Identifiers written by the API come from the data model, quoted via Ident::with_quote.
• Unbounded recursion. resolve_filter_expr, normalized_filter_json and count_filter_json_leaves recurse over caller-supplied filter trees, but serde_json's 128-deep default limit caps the depth at parse_filters_arg, and the AST/clause walks are all iterative behind MAX_CLAUSE_PREDICATES, MAX_RELATION_DEPTH, MAX_EXPR_NESTING and MAX_RELATION_EXPANSIONS.
• Leaf-count bound parity. countFilters in gateway.ts and count_filter_json_leaves in ast_conv.rs agree on empty groups ({and: []} counts as one leaf), on and-over-or precedence and on walking nesting to the bottom; both reject at 501.
• Clause rewriting. append_expr_to_clause parenthesizes a top-level OR/XOR before AND-ing (NOT binds tighter than AND, so it needs none), and every group this API builds is already Expr::Nested, so no unparenthesized disjunction can be spliced in. matching_conjuncts decides which split matches before consuming the clause, so a needle that isn't there leaves the clause byte-identical.
• expr_key normalization. Qualifier dropping only fires when the outermost FROM has one relation; 1.5 is not mistaken for a qualified name; ilike→like is correct since a Cube filter has no case sensitivity. The aggregate-name collapse to measure does make MIN(col)/MAX(col) key identically, but a measure's column is its own member name and a query aggregating a measure with a non-matching function does not plan, so I could not construct a reachable case.
• Scope/auth. Both routes sit behind userMiddlewares and assertApiScope('sql', …), matching sql4sql/rest4sql. The two CodeQL rate-limiting notices apply equally to every existing route on this app and are not specific to this change.
• Behaviour change in rest4sql: it now throws on an unparseable security context instead of silently planning without one — strictly safer, worth a release note.

Not a code issue

The PR body says set "clears outermost WHERE + HAVING entirely"; the implementation keeps non-filter conjuncts (drop_filter_predicates), which is what set_sql_filters' own doc comment and the docs page describe. Worth updating the description before merge.

Verification not run

cargo check/cargo test for cubesql and the api-gateway Jest suite were not run — no prebuilt Rust target/ and no installed node_modules in this checkout, so both would have been cold builds. Findings above are from reading the code; CI covers the suites.

Review threads

The listing returned 13 of my own threads (single page). Resolved 11 as addressed by the current diff; re-affirmed 2 (the auth.rs security-context downgrade and the ast_conv.rs comment length), and left the 2 github-advanced-security threads alone. Findings 1 and 2 above had no prior thread.

· branch [`feat-ast-sql-rewriting`](https://github.com/cube-js/cube/tree/feat-ast-sql-rewriting)

[claude]

Claude finished @MazterQyou's task in 7m 29s —— View job

---

Review complete — 3 findings (2 medium, 1 low), posted inline

Details

• Gather context (diff, changed files)
• Review gateway.ts / sql-server.ts API surface
• Review sql_filters.rs + native bindings
• Review ast_conv.rs
• Review tests and docs
• Handle prior review threads
• Post findings

Findings (inline)

#	Where	Concern
1	rust/cubesql/cubesql/src/compile/ast_conv.rs:962	is_leaf_filter_predicate accepts a LIKE/ILIKE whose pattern has internal wildcards ('a%b', 'a_c'), but parse_like_pattern (rust/cubesql/cubesql/src/compile/rewrite/rules/filters.rs:87) refuses that shape as a Cube filter. set drops the predicate, extraction never reports it, and nothing can restore it — the rewritten query returns more rows. Same class for BETWEEN/comparisons on members that are neither numeric nor time.
2	rust/cubesql/cubesql/src/compile/ast_conv.rs:3049	"original planned, rewritten did not" is CubeError::internal → 500, yet it is reachable from a bad request (measure filter added to a query with no GROUP BY yields ... HAVING COUNT(...) > 5), so callers get 500s for caller errors.
3	rust/cubesql/cubesql/src/compile/ast_conv.rs:3220	old: [A, A] — the second removal is a no-op, so the request fails with "Filter to replace … was not found in the outermost SELECT" even though A was present and replaced.

Review threads

The listing returned 6 unresolved threads, 4 of them mine. All 4 were resolved as addressed by the current diff (doc-comment verbosity trimmed; parse_security_context_arg now throws on a non-string / invalid-JSON security context; in_band_or_thrown splits internal from user errors; status narrowed to 'ok' | 'error'). 0 re-affirmed, 0 skipped as duplicates. The two github-advanced-security CodeQL rate-limiting threads were left untouched.

What holds up well

• Value rendering is safe: string literals go through SingleQuotedString, and is_numeric_literal (ast_conv.rs:265) rejects whitespace, inf/NaN and anything non-numeric before a number is rendered verbatim — no injection path through filter values.
• parse_security_context_arg (packages/cubejs-backend-native/src/auth.rs:103) fixes a real pre-existing hole in rest4sql: a context that failed to parse used to silently become "no security context", dropping the filters derived from it. Throwing is the right call.
• Every clause/relation walk is iterative with an explicit budget (MAX_CLAUSE_PREDICATES, MAX_RELATION_DEPTH, MAX_RELATION_EXPANSIONS), and filter-tree depth is capped by serde_json's own 128-level recursion limit, so the arbitrary-SQL entry point has no obvious stack-exhaustion path.
• add plans only the rewritten query and falls back to planning the original solely to attribute a failure — the right trade for the expensive step.
• Docs (docs-mintlify/reference/core-data-apis/rest-api/reference.mdx) are accurate about the 500-filter bound counting group leaves, the outermost-SELECT-only scope, and delete-not-found being a tolerated no-op.

Not verified

No build or test run: this checkout has no node_modules and no rust/cubesql/target, so neither yarn test in packages/cubejs-api-gateway nor cargo test -p cubesql was executed. All three findings are from reading the code; #1 is cross-checked against parse_like_pattern, #2 relies on ... HAVING COUNT(...) > 5 over a non-aggregated projection failing to plan, which I could not execute here.

· branch [`feat-ast-sql-rewriting`](https://github.com/cube-js/cube/tree/feat-ast-sql-rewriting)

[claude]

Claude finished @MazterQyou's task in 9m 21s —— View job

---

1 high, 1 medium, 3 low — 5 inline comments posted

Full review

• Gather context (diff, PR body, docs preview, CodeQL threads)
• Review gateway.ts / sql-server.ts (API surface, auth, validation)
• Review sql_filters.rs + native bindings + auth.rs
• Review ast_conv.rs (2 900 lines of code, 4 250 of tests)
• Review tests and docs
• Resolve stale review threads / check for duplicates
• Post findings

Findings

Sev	Where	What
High	ast_conv.rs:1780	is_plain_identifier keeps the quotes in a filter key for any non-lowercase identifier, reasoning from PostgreSQL folding — but cubesql resolves unquoted camelCase members case-insensitively (compile/mod.rs:2652). A query writing someNumber unquoted keys as somenumber while the needle keys as "someNumber", so delete and set silently no-op with status: "ok" and replace returns a 400 "not found".
Medium	gateway.ts:129	MAX_SQL_FILTERS + countFilters duplicate, hand-synced, the leaf bound assert_filter_count already enforces and reports in-band as the same 400.
Low	ast_conv.rs:2748	set emits one removal per reported filter with no bound on that count, and each removal re-keys the whole clause — ~10⁸ expr_key calls at MAX_CLAUSE_PREDICATES.
Low	ast_conv.rs:2642	add_filters_and_verify takes original_sql and sql, always the same string from its single caller; get_sql_filters is a verbatim copy of plan_and_extract_filters.
Low	ast_conv.rs:2669	Comment restates the code under it (same at :2893).

What holds up well

The SQL-injection surface is closed off properly: values reach the AST only as ast::Value::SingleQuotedString / Number, numeric_value_expr validates the character set and rejects inf/NaN before rendering a number verbatim, booleans go through parse::<bool>(), and operators are a closed whitelist. Members are resolved against MetaContext before a column name is written, and MetaContext itself is fetched under the caller's security context, so scoping is respected on both the read and the rewrite path. Every walk that a caller controls the depth of is iterative and budgeted (MAX_RELATION_DEPTH, MAX_RELATION_EXPANSIONS, MAX_EXPR_NESTING, MAX_CLAUSE_PREDICATES), and the recursive ones (resolve_filter_expr, normalized_filter_json) sit behind serde_json's 128-deep parse limit. in_band_or_thrown splitting caller faults (400) from internal ones (500) is the right seam, and resSqlFilters honours it. The Or/Xor parenthesization in append_expr_to_clause is the kind of thing that is normally found in production rather than in review.

Not flagged

• CodeQL rate-limiting on both new routes: every neighbouring route in gateway.ts (/v1/sql, /v1/dry-run, /v1/convert-query) is equally unrated, so this is a pre-existing repo-wide gap rather than something this PR introduces.
• parse_security_context_arg treating a missing argument as unauthenticated — fail-open, but identical to the rest4sql behaviour it replaces, and the JS wrappers always pass the argument.
• QueryConvertResponse.status narrowed from string to 'ok' | 'error' — only breaks a consumer constructing the type, not one reading it.

Verification

Neither suite was run: the checkout has no node_modules (no jest binary) and rust/cubesql has no target/, so a cubesql build would be from scratch. The high-severity finding was traced by hand through expr_key/push_word/is_plain_identifier and cross-checked against the existing planner test at cubesql/src/compile/mod.rs:2652, not reproduced at runtime. docs-mintlify changes read correctly against the implementation and match the Mintlify preview.

Review threads

The listing returned 3 claude-authored threads; all 3 resolved as addressed by the current diff (is_leaf_filter_predicate replaced by plan-driven ReportedFilters; the CubeError::internal→400 path now CubeError::user via plan_and_extract_filters; dedupe_filters now runs before the removal walk at :2848). None re-affirmed, no findings skipped as duplicates. The 2 github-advanced-security threads were left untouched.

· branch [`feat-ast-sql-rewriting`](https://github.com/cube-js/cube/tree/feat-ast-sql-rewriting)

[claude]

Claude finished @MazterQyou's task in 9m 19s —— View job

---

Review: 1 medium, 3 low — 4 inline comments posted, 5 prior threads resolved

Full review

• Read changed files (gateway, sql-server, native bindings, ast_conv.rs)
• Review Rust AST conversion / filter rewriting logic
• Review API surface, auth, input validation
• Review tests and docs
• Handle prior review threads
• Post findings

Findings

Sev	Where	What
Medium	rust/cubesql/cubesql/src/compile/ast_conv.rs:1056	MAX_CLAUSE_PREDICATES = 10_000 admits a clause the recursive sqlparser Display can't render. add renders before it plans, so POST /v1/sql-filters with a ~10k-conjunct WHERE reaches query.to_string() at 10k recursion depth on a 2 MB tokio worker stack. A Rust stack overflow aborts the process. Marked PLAUSIBLE, not confirmed — I could not build cubesql here to pin the threshold; the parse/render asymmetry itself is structural, and the 501..10_000 accepted-and-rendered range is untested.
Low	ast_conv.rs:609-617	assert_clauses_are_bounded re-walks both clauses once per action, O(actions × clause size) — up to ~10M node visits for a set the bounds otherwise accept. Not dead work (the clause grows), but the growth is countable, so the count can be carried forward instead of recomputed.
Low	ast_conv.rs:658, :2022	MAX_CLAUSE_PREDICATES spent as a per-expression visit budget where MAX_EXPR_NESTING is the module's stated expression bound. No behaviour change; reads as the wrong constant.
Low	packages/cubejs-api-gateway/test/sql-filters.test.ts:110	No coverage of assertApiScope('sql') on either route, the one pre-call check without a test — and set: [] strips every outermost filter, so a dropped scope check is the regression that matters.

Checked and clear

• Value injection. SingleQuotedString rendering doubles the quote; test_modify_sql_ast_string_value_quoting covers O'Brien' OR 1=1 --. Numeric values are rendered verbatim but is_numeric_literal gates on a character whitelist and a finite f64 parse, so inf/NaN/whitespace don't get through.
• Key collapsing. push_word folding every aggregation to measure looked like it could conflate two measures over one column, but the key carries the member name (short_name()), which identifies the measure uniquely — MEASURE(orders."max_price") and MAX(orders."max_price") matching is the intended behaviour, not a collision. Likewise ilike → like matches the planner, which maps both Like and ILike to contains (rewrite/rules/filters.rs:3702-3703).
• Filter-tree recursion. count_filter_json_leaves, normalized_filter_json, filter_members and resolve_filter_expr all recurse per group level, and MAX_FILTERS counts leaves — so a 100k-deep single-leaf {"and":[{"and":[…]}]} would slip the bound. serde_json's own 128-level limit on from_str caps it first.
• Column-path removal (remove_reported_column_predicates) drops every predicate on a column, gated on the filter being the sole reported one on its member. test_remove_by_column_only_for_the_sole_reported_filter and test_set_leaves_a_predicate_the_plan_does_not_report pin both directions; I could not construct a predicate that the AST reads as on-column but the planner doesn't report as a filter on that member.
• Gateway input handling. ?query=a&query=b yields an array and is caught by the typeof check; express.json strict mode rejects a bare null body; an add: null counts as the one requested op and then fails assertFilterArray as a 400. sql scope matches /v1/sql and /v1/convert-query.
• Rate limiting (the two CodeQL alerts) is absent on every other gateway route too, so it isn't specific to this change.
• Docs match the implementation, including the 500-filter leaf-counted bound and the qualified-and-quoted output spelling. The page documents no API scopes anywhere, so omitting sql is consistent.

Worth calling out

parse_security_context_arg (auth.rs:97) fixes a real silent failure it inherits from rest4sql: a security context that didn't parse as JSON used to be swallowed into None by .ok(), planning the query with no security context and dropping the filters derived from it. It now throws. Good catch to make while factoring the helper out.

The Rust test module is genuinely thorough — 80-odd cases including the escaping, case-folding, CTE-shadowing, batch-cache and Send-future paths, which is why most of what I went looking for was already pinned.

Not verified

• Neither cargo test -p cubesql nor the gateway Jest suite ran: no prebuilt rust/cubesql/target, and the Jest invocation was denied by the sandbox. All findings are from reading the code.

Review threads

The listing returned 5 of my own threads (7 unresolved in total; the 2 github-advanced-security ones left alone). All 5 resolved after checking the code at each site, each with a reply saying why: is_plain_identifier replaced by case-folding in expr_key; the JS-side MAX_SQL_FILTERS/countFilters bound removed in favour of the native one; get_sql_filters now delegating to plan_and_extract_filters; set_sql_filters now bounding the reported filters; the restating comment on the replace path gone. None re-affirmed, no finding skipped as a duplicate.

· branch [`feat-ast-sql-rewriting`](https://github.com/cube-js/cube/tree/feat-ast-sql-rewriting)

[claude]

Claude finished @MazterQyou's task in 8m 24s —— View job

---

Reviewed — 1 medium correctness, 1 docs accuracy, 3 low. 5 inline comments posted.

Full review

• Read PR diff and changed files
• Review Rust: ast_conv.rs, sql_filters.rs, native bindings
• Review JS: gateway endpoints, sql-server.ts, native index.ts
• Review tests and docs
• Handle prior review threads
• Post inline findings + summary

Findings

#	Severity	Where	Concern
1	Medium	rust/cubesql/cubesql/src/compile/ast_conv.rs:150	inDateRange/notInDateRange render a date-only end bound verbatim, so the rewritten query covers ~a day less than the identical filter through /v1/load, which pads to T23:59:59.999. Round-trip verification is blind to it because report_date_value strips T00:00:00.000 back to the bare date.
2	Medium (docs)	docs-mintlify/.../reference.mdx:392	"every filter GET reports is removed" isn't what set does — a reported filter that isn't sole on its member and isn't spelled out the way this API writes it is silently kept, with status: ok. The PR's own test_remove_by_column_only_for_the_sole_reported_filter pins that case.
3	Low	ast_conv.rs:2223	report_date_value duplicates normalize_filter_value's strip chain verbatim.
4	Low	ast_conv.rs:1547-1629	Four near-identical AND-conjunct splitters; with_keys already guards against the two walks drifting.
5	Low	packages/cubejs-backend-native/src/auth.rs:97	The shared security-context parser makes the pre-existing rest4sql endpoint throw where it previously fell back to no security context — a defensible fix, but an undocumented behaviour change to a shipped endpoint.

What holds up well

• SQL construction is injection-safe. Every string value goes through ast::Value::SingleQuotedString, booleans through parse::<bool>(), and the one verbatim path — ast::Value::Number — is gated by is_numeric_literal, which rejects anything outside [0-9.+-eE] and requires a finite f64. Identifiers are Ident::with_quote('"', …) over meta-model names. LIKE patterns escape \, % and _ in that order.
• The error taxonomy is carefully drawn. in_band_or_thrown splits caller mistakes (in-band {status: "error"} → 400) from internal failures (thrown → 500), and verify_additions re-plans the original query only on failure, specifically so a query that never planned is told apart from a rewrite that broke it.
• Recursion is bounded where it matters. Every clause and expression walk is iterative, with MAX_CLAUSE_PREDICATES / MAX_EXPR_NODES / MAX_RELATION_EXPANSIONS backstopping the two places that do recurse (sqlparser's renderer and Expr's drop), and test_clause_at_the_bound_renders_on_a_worker_stack renders at the bound on a 2 MiB stack.
• Test coverage is unusually thorough — ~80 cases covering CTE chains, shadowed cube names, identifier-case mismatches, wildcards over joins, batch-visible additions, group-member reachability, and the bound itself. The gateway test stubs the native layer at the SQLServer seam so the gateway's own validation is testable without the binary.

Two notes, not posted inline

• Rate limiting (gateway.ts:488, :496): CodeQL already has threads on both routes, so I did not pile on. Worth weighing separately, though — set/delete/replace each plan the query twice (three times on failure), which is a heavier unauthenticated-cost multiplier than the neighbouring /v1/sql.
• Comment density: the rationale prose in ast_conv.rs is load-bearing more often than not (MAX_CLAUSE_PREDICATES, normalized_filter_json, the MEASURE-path note all record things a later edit would otherwise undo), so I'm not flagging individual blocks. One duplication is worth collapsing: the same four-line in-band-vs-thrown rationale appears at gateway.ts:1577 and sql_filters.rs:133 — one of them can point at the other.

Not verified

Neither test suite ran: node_modules is absent and a cold cargo build of the DataFusion fork does not fit this run. Findings 1 and 2 are from code reading plus the PR's own test expectations; finding 1's key claim (no end-of-day padding on the reported bound) is corroborated by test_add_sql_filters_date_round_trip at ast_conv.rs:5450 passing as written.

Review threads

The listing returned 4 of my own threads (plus 2 CodeQL threads, untouched). All 4 resolved as addressed by this revision: the MAX_CLAUSE_PREDICATES stack bound (now measured and pinned by a test), the quadratic clause re-walk (now ClauseBudget), the clause constant reused as an expression budget (now MAX_EXPR_NODES), and the missing API-scope test (now sql-filters.test.ts:126). None re-affirmed; no finding this round duplicated an open thread.

· branch `feat-ast-sql-rewriting`

[claude]

"every filter GET reports is removed" overstates what set does — a reported filter can be silently kept, with the response still status: ok.

Column-based removal only engages for a filter that is sole on its member (ReportedFilters::is_sole_on_member, ast_conv.rs:2370). When two reported filters share a member and the query does not spell one of them out the way this API writes it, the Remove action is a no-op; verify_additions only checks the additions, so nothing surfaces. The PR's own test_remove_by_column_only_for_the_sole_reported_filter (ast_conv.rs:7134) pins exactly that case: order_date >= x AND order_date < y AND order_date IS NOT NULL, reported as inDateRange + set, leaves the clause untouched and reports applied == false.

A caller asking for set: [] on such a query gets status: ok and a filters array that still lists the range. Either say so here ("a reported filter the outermost SELECT does not spell out may be kept; compare the returned filters") or have set fail when a removal it promised did not apply.

[claude]

This changes rest4sql's behaviour, not only the new entry points. The code it replaces (rest4sql.rs:131 on master) swallowed both failures with .ok() / Err(_) => None; now a non-string argument throws Security context must be a JSON string and a malformed string throws Security context is not valid JSON.

Throwing is the right call — the old path planned as if the caller had no security context and silently dropped the row-level filters derived from it. But it's a behaviour change to a shipped endpoint riding in on a PR titled "AST SQL rewriting": any native-addon consumer that passed an object instead of a string, or that relied on the silent fallback, now gets a hard rejection. Worth a line in the PR description at least, since js/index.ts stringifies for every in-repo caller and nothing in the test suite covers the direct-native case.

[claude]

Claude finished @MazterQyou's task in 7m 37s —— View job

---

2 medium, 1 low — solid implementation with unusually thorough test coverage; both mediums are in the error/logging plumbing, not the rewriting logic.

Review details

• Read the diff and changed files
• Review TS layer (gateway, sql-server, native js bindings)
• Review Rust layer (sql_filters.rs, auth.rs, ast_conv.rs)
• Review docs and tests
• Handle prior review threads
• Post findings

Findings posted inline

#	Where	Severity
1	packages/cubejs-api-gateway/src/gateway.ts:1611, :1691 — the SQL string is passed as query to handleError, and sanitizeQueryForLogging spreads it into a character-indexed object; no redactedQuery, so literals reach the log sink with CUBEJS_LOG_REDACTION on; no requestStarted	Medium
2	packages/cubejs-backend-native/src/sql_filters.rs:137 + rust/cubesql/cubesql/src/compile/ast_conv.rs:2676 — try_as_logical_plan returns CubeError::internal, so SET / SHOW / BEGIN is answered 500 instead of an in-band 400, and add answers 400 for the same input	Medium
3	rust/cubesql/cubesql/src/compile/ast_conv.rs:1057 (also :1934, :2451) — explanatory comments well past the 3-line rule	Low

Checked and clear

• Injection. Filter values go through Value::SingleQuotedString (sqlparser doubles the quote) or numeric_value_expr, which refuses anything but a finite plain numeric literal — f64-only parsing would have admitted inf/NaN and whitespace, and the character check closes that. Member names are resolved against MetaContext before they are rendered, so an unknown member never reaches the AST. LIKE patterns escape \, %, _ and rely on PostgreSQL's default backslash escape, which the comment at ast_conv.rs:439 justifies.
• Authz. Both routes assertApiScope('sql', …) before anything reaches the native layer, and test 'both routes require the sql scope' pins it with a call-count assertion. parse_security_context_arg refusing a present-but-non-string context, rather than planning as if there were none, is the right call — the old rest4sql code silently dropped it and with it the context's filters.
• Resource bounds. MAX_FILTERS, MAX_CLAUSE_PREDICATES, MAX_EXPR_NESTING, MAX_RELATION_DEPTH and MAX_RELATION_EXPANSIONS cover the recursive and the branching walks; every clause walk is iterative. set additionally bounds the filter count the plan reports, since each reported filter costs a removal.
• Precedence on append. append_expr_to_clause parenthesizes an existing top-level OR/XOR before AND-ing. NOT, BETWEEN, LIKE and IS NULL all bind tighter than AND, so leaving those unparenthesized is correct.
• Tests. 54 Rust tests, including the group-reachability, batch-keying, date round-trip, CTE-shadowing, unqualified-ref-in-join and futures_are_send cases, plus 9 gateway tests separating in-band 400 from thrown 500.

Not run

yarn dependencies are not installed and there is no rust/cubesql/target, so neither packages/cubejs-api-gateway jest nor cargo test -p cubesql was executed here — a from-scratch cubesql build was out of budget for this session. Findings are from reading the files, not from a failing run.

Review threads

The listing returned 5 of my own threads (plus 2 CodeQL rate-limiting threads on gateway.ts:488/:496, left alone — not mine, and the neighbouring SQL endpoints are unrated too). Resolved 3 as fixed by this round: inDateRange upper-bound padding (now date_range_upper at ast_conv.rs:381/:2218), the duplicated date strip chain (report_date_value now delegates to normalize_filter_value, :2205), and the four copies of the and-conjunct walk (collapsed into into_and_conjuncts_with/and_conjuncts_with, :1554/:1588). Re-affirmed 2 that are still live and not duplicated by this round: the docs at reference.mdx:392 still describing set as removing the filters GET reports, when is_sole_on_member can silently keep one; and the rest4sql security-context behaviour change still being unmentioned in the PR description.
· branch feat-ast-sql-rewriting

[claude]

Passing the raw SQL string as query makes the logged query a character-indexed object. handleError runs it through sanitizeQueryForLogging (gateway.ts:2048), which does { ...query } — spreading a string yields {"0":"S","1":"E","2":"L",...}, one key per character of the statement. Both new handlers do this (gateway.ts:1611 and gateway.ts:1691); every other caller passes an object, and /v1/cubesql — the other endpoint taking a SQL API statement — wraps it as query: { sql: query } (gateway.ts:593).

Two more things are missing at the same site:

• redactedQuery: this.redactedSqlForLog(query), which /v1/cubesql passes at gateway.ts:596. Without it the statement's literals reach the log sink verbatim even with CUBEJS_LOG_REDACTION on — and /v1/sql-filters carries literals both in the statement and in the values of the filters it is handed.
• requestStarted, so this.duration(requestStarted) (gateway.ts:737) returns undefined and the error event logs no duration.

Suggested change

	this.handleError({
	this.handleError({
	e,
	context,
	query: { sql: query },
	redactedQuery: this.redactedSqlForLog(query),
	res,
	requestStarted,
	});

(needs const requestStarted = new Date(); at the top of each handler, as in sql() at gateway.ts:1711.)

[claude]

A statement with no logical plan is classified as internal, so it lands on 500 rather than the in-band 400 this function exists to produce.

plan_and_extract_filters (rust/cubesql/cubesql/src/compile/ast_conv.rs:2676) wraps only the convert_sql_to_cube_query failure as CubeError::user; the try_as_logical_plan()? on the next line propagates plan.rs:117's CubeError::internal("This query doesnt have a plan, because it already has values for response") untouched. MetaOk / MetaTabular / CopyFrom / CreateEmptyTempTable all reach it, i.e. anything that compiles but isn't a SELECT.

GET /v1/sql-filters?query=SET%20timezone%20%3D%20%27UTC%27 → Internal(_) → thrown → 500 with an internal-sounding message, for a plain caller mistake. Same for set, delete and replace, which all call plan_and_extract_filters on the original query first.

It is also asymmetric with add, which parses before it plans: parse_single_query (ast_conv.rs:588) rejects the same input with NotImplemented("Only SELECT statements are supported"), which add_sql_filters maps to CubeError::user and the gateway answers 400. Two operations, one input, two status codes.

Fix at the source rather than here — in plan_and_extract_filters, map the try_as_logical_plan error to a user error, e.g.

let logical_plan = query_plan
    .try_as_logical_plan()
    .map_err(|_| CubeError::user("Only SELECT queries are supported".to_string()))?;

[claude]

Seven lines to say one thing. The load-bearing sentence is the measurement — the bound backstops a renderer that recurses per conjunct, and twice it overflows a 2 MiB worker stack. The rest (MAX_FILTERS comparison, "a query this API produced is never one it then refuses") re-derives from the two constants, and test_clause_at_the_bound_renders_on_a_worker_stack already names the pin it is being credited with:

/// Upper bound on the predicates a clause of the outermost SELECT may hold,
/// backstopping the renderer and the drop, which recurse per conjunct.
/// Measured: twice this overflows a 2 MiB worker stack in a debug build.

Same shape at ast_conv.rs:1934 (with_keys, five lines for "the two splits walk in the same order; if that ever stops holding the keys are recomputed") and ast_conv.rs:2451 (thirteen lines of normalized_filter_json). House rule is 3 lines for an explanatory comment — worth a pass over the file, which carries a lot of these.