fix(tesseract): reject FILTER_PARAMS string columns under a calendar time shift

[paveltiunov]

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

A multi_stage measure shifted by name — time_shift: [{ name: prior_fiscal_year }], resolved through a calendar cube's own time_shift declaration — rendered its cube's FILTER_PARAMS push-down as a bare column bound to the query's unshifted reporting bounds.

That contradicts the stage around it. The stage joins the calendar on its mapping column and reads prior-period fact rows, while the pushed-down predicate restricts the same scan to the reporting period. The stage comes back empty — the failure CORE-543 / #11030 fixed for interval shifts, reached by a different route.

Observed SQL before this change (both shifted stages, all bounds the reporting day):

cte_0 AS ( SELECT sum("fpc_margin".net_sales_a) …
  FROM (SELECT * FROM fpc_margin
        WHERE (week_end_d >= $1 AND week_end_d <= $2))  AS "fpc_margin"
  LEFT JOIN … ON "fpc_margin".week_end_d = "fpc_calendar".next_fiscal_year_d
  WHERE ("fpc_calendar".next_fiscal_year_d >= $3 AND … <= $4)),
cte_2 AS ( … next_two_fiscal_year_d … )

Root cause — two levels below where the shift is read. PushDownBuilderContext::make_sql_nodes_factory splits shifts via extract_time_shifts: interval shifts go to TimeShiftState, calendar shifts to a separate map consumed only by CalendarTimeShiftSqlNode. base_filter.rs read TimeShiftState alone, so a calendar shift looked like no shift at all and the offsetting branch added by CORE-543 was never reached.

Fix — carry the calendar shifts to the filter-params path too (SqlNodesFactory → VisitorContext → SqlEvaluatorVisitor), and have to_sql_for_filter_params distinguish the cases via a new FilterParamsTimeShift enum:

column	shift	behaviour
string	interval	offset by the interval — unchanged
string	calendar, declared as interval	offset by interval.inverse() — now correct, was empty stages
string	calendar, declared with sql	rejected, naming the binding, the shift and the remedy
string	calendar, neither	rendered bare, mirroring CalendarTimeShiftSqlNode's own fall-through
callback	interval	rejected — unchanged
callback	calendar	rendered — unchanged

Only an sql-mapped calendar shift is genuinely inexpressible: it resolves through a mapping column on the calendar, so there is no expression over the fact's own column that stands for it and no bound the planner can widen to without reading the calendar. A callback column is the one form that can express it — it receives the query's own bounds and widens the pushed-down range itself — so the error points there.

The lookup also probes the calendar cube's PK, since the calendar map is keyed by it: a filter on a non-PK dimension of the calendar would otherwise miss the bare name and render the column bare. (A filter on a non-calendar cube's dimension is out of reach either way — time_shift_pk_full_name is only populated for calendar-cube dimensions, and a named shift on such a dimension is dropped by extract_time_shifts earlier. Pre-existing, separate.)

⚠️ Behaviour change — narrow. Only models combining a string FILTER_PARAMS column with an sql-mapped calendar shift now fail to plan; they previously returned silently empty stages, so this surfaces an existing bug rather than breaking working behaviour. Calendar shifts declared as a plain interval are not affected — they now keep planning and start emitting correct SQL where they previously produced the same empty stages. Still a visible change, and worth a reviewer's judgement.

Open question — resolved. Reject vs. 1 = 1: rejecting stands for the sql-mapped case (a silent switch to a full-table scan is its own surprise, and it matches the callback+interval rejection two arms down). Narrowing to sql-mapped shifts is what shrank the blast radius.

Tests

• rust/…/tests/filter_params_calendar_time_shift.rs — sql-mapped string column rejected; callback column still pushed into every shifted stage; each stage filters on its own mapping column; interval-declared calendar shift offset with the sign pinned at (week_end_d + interval '-1 year'); shift found through the calendar PK when the binding is a non-PK calendar dimension (verified meaningful — reverting the probe makes it fail with the column bare).
• packages/cubejs-schema-compiler/test/unit/filter-params-calendar-time-shift.test.ts — the first three at the YAML level.

The first two commits landed the tests as characterisation tests pinning the broken behaviour, then the fix inverted them, so the diff shows exactly what changed.

Docs — new "Time-shifted measures" subsection under FILTER_PARAMS in reference/data-modeling/context-variables.mdx: why the pushed-down column must carry the stage's shift, which shift kinds can be carried, and the callback form as the remedy with a worked fiscal-prior-year band.

Verification: cargo test -p cubesqlplanner --lib → 1352 passed, 0 failed. Schema-compiler unit suite → 913 passed; the 2 ErrorReporter snapshot failures are pre-existing and reproduce on a clean tree. cargo fmt / clippy / eslint clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01EwtxWoLUp7ULnsQJjVLkZt

[claude]

Claude finished @paveltiunov's task in 2m 25s —— View job

---

Review: sound diagnosis and a clean plumbing fix — but the rejection is too broad (1 high, 1 medium, 2 low)

Full review — expand for findings, answer to the open question, and the checklist

• Gather context / read diff
• Review Rust changes (filter, sql_nodes, visitor)
• Review tests
• Post review

What's good

The root-cause analysis is right and easy to verify: extract_time_shifts (time_shift_state.rs:75-125) splits into two maps and base_filter.rs only ever read one of them, so a calendar shift was indistinguishable from no shift. Threading calendar_time_shifts through SqlNodesFactory → VisitorContext → SqlEvaluatorVisitor mirrors the existing time_shifts plumbing exactly, including the with_* clone-and-set idiom, so nothing new is invented. The FilterParamsTimeShift enum makes the four-way table explicit at the type level, and switching the callback guard from time_shift.is_some() to matches!(…Interval(_)) correctly preserves the callback+calendar case that was already working. Both tests were landed as characterisation tests first and then inverted — the diff genuinely shows what changed.

Findings

#	Severity	Where	Issue
1	High	typed_filter.rs:98-113	A calendar shift declared as interval (no sql) is pure arithmetic — CalendarTimeShiftSqlNode renders it as add_timestamp_interval(input, interval.inverse()) — yet this branch rejects it. The <unnamed> name fallback in the message is the tell. Gate on shift.sql.is_some(); offset by interval.inverse() otherwise (note the inverse: extract_time_shifts:113 and the calendar node both inverse, so the map holds the un-inverted declaration).
2	Medium	base_filter.rs:39-44	calendar_time_shifts is keyed by the calendar cube's PK full name and looked up with a bare full_name(), while the sibling interval path uses get_for_symbol, which also probes the owned-by-cube child. A FILTER_PARAMS bound to the fact's time dimension misses the lookup and still renders bare — the same silent-empty stage, one binding away.
3	Low	typed_filter.rs:79-87, base_filter.rs:29-33	Long explanatory comments that restate the error message / narrate the routing.
4	Low	tests	Neither the Rust nor the YAML test covers an interval-declared calendar shift (see #1), nor a FILTER_PARAMS bound to the fact's own dimension (see #2). Both are the shapes where the current behaviour is wrong.

Your open question — reject vs. 1 = 1

Rejecting is the right call for the case where the shift is genuinely inexpressible, i.e. shift.sql is set. Your reasoning holds there: a silent full-table scan is its own surprise, and it's consistent with the callback+interval rejection two arms down. But finding #1 means the current code also rejects a case that is expressible, and that's the part I'd change before merging — not the reject-vs-1 = 1 choice itself.

Narrowing the rejection to sql-mapped shifts also shrinks the blast radius of the behaviour change you flagged: models using interval-declared calendar shifts keep planning (and start producing correct SQL rather than empty stages), and only the genuinely unrepresentable models get the error.

Docs

The behaviour change introduces a new user-facing planning error. The docs checkbox is unticked — a short note on the FILTER_PARAMS / calendar-shift interaction in /docs-mintlify (the callback form as the remedy) would save a support round-trip, given the error text is the only place that remedy is currently written down.

· branch `claude/filter-params-date-injection-pm2qgo`

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[claude]

Claude finished @paveltiunov's task in 2m 25s —— View job

---

Re-review of the two follow-up commits: both prior high/medium findings addressed; 1 medium, 2 low remain

Full review — expand for findings and checklist

• Check what changed since the last review
• Verify finding ReferenceError: self is not defined - Issue with whatwg-fetch when using the core lib with Node.js #1 (interval-declared calendar shifts) is correctly fixed, incl. the sign
• Verify finding MUTEX for requests #2 (PK-key lookup) is correctly fixed
• Check comment trimming + test coverage for the new shapes
• Post review

What the follow-ups fixed

6aa097b and f259ace land after my first pass and address all four earlier findings.

Finding #1 (high) — resolved, and the sign is right. The rejection is now gated on shift.sql.is_some(), with interval-declared calendar shifts offset by interval.inverse().to_sql(). I traced the sign against both sources: CalendarTimeShiftSqlNode renders add_timestamp_interval(input, interval.inverse().to_sql()) (sql_nodes/calendar_time_shift.rs), and type: next is already negated at symbol-build time (dimension_symbol.rs:383), so the pushed-down column and the rendered dimension carry the same offset for prior and next alike. interval_declared_calendar_shift_offsets_the_column pins week_end_d + interval '-1 year', which is the correct direction — the shape my earlier note said was uncovered. The third arm (calendar shift with neither sql nor interval) also correctly mirrors the calendar node's own fall-through to unshifted.

This is the narrowing I asked for on the open question: only genuinely inexpressible (sql-mapped) shifts now fail to plan, so the behaviour-change blast radius is smaller than the PR description states — interval-declared models keep planning and start emitting correct SQL rather than empty stages. Worth updating the "⚠️ Behaviour change" paragraph to say so.

Finding #3 (low) — resolved. The base_filter.rs routing narration is down to two lines, and the paragraph that restated the error message is gone.

Remaining findings

#	Severity	Where	Issue
1	Medium	base_filter.rs:37-41	The PK fallback is real but the comment describes a path that cannot fire: time_shift_pk_full_name is populated only under if is_calendar (dimension_symbol.rs:417-445), so a fact dimension never has one, and symbol_to_match is the filtered dimension, not the binding target. What the arm actually covers is a filter on a non-PK calendar dimension — and no test exercises it, so the one branch added to close a silent-miss hole is the one branch with no coverage.
2	Low	typed_filter.rs:88-119	The two guarded arms differ only in interval.is_some(), and the first pays an unwrap() the guard already proved; one arm with an inner match &shift.interval drops both. The 12-line comment above them has one load-bearing paragraph (the pre-inverted-TimeShiftState vs. as-written-calendar-map asymmetry — a later "consistency fix" there would silently reverse the bounds); the first paragraph restates the match guard.
3	Low	typed_filter.rs:50-53	The FilterParamsTimeShift doc still asserts a calendar shift "has no expression over the fact's own column" — now true only of sql-declared ones, with the new arm ten lines below as the counterexample.

Details and a concrete diff are in the two inline comments.

Verification

I read the code and traced the sign against CalendarTimeShiftSqlNode and DimensionSymbolFactory rather than re-running the suite — cargo test -p cubesqlplanner was not run in this pass, so I'm taking the PR's reported 1350-passed on trust for the new test.

Docs

Still unticked, and the narrowed rejection makes the note shorter to write: a string FILTER_PARAMS column cannot carry an sql-mapped calendar shift, and the callback form is the remedy. The error text remains the only place that is written down.

· branch `claude/filter-params-date-injection-pm2qgo`

[claude]

Reviewing f690585

• Check what changed since the last review
• Verify the new non-PK test actually covers the or_else probe
• Review the docs addition
• Check prior threads / dedupe
• Post review

View job run · branch claude/filter-params-date-injection-pm2qgo

[paveltiunov]

All four findings addressed. Head is now f690585.

#1 (High) — real bug, fixed in 6aa097b. You were right and I'd have shipped it. Gated on shift.sql instead of shift kind: sql: None, interval: Some → offset by interval.inverse(); sql: None, interval: None → bare, matching CalendarTimeShiftSqlNode's own fallthrough; sql: Some → rejected. Took the .inverse(), and interval_declared_calendar_shift_offsets_the_column now pins the sign at (week_end_d + interval '-1 year').

#2 (Medium) — fixed in 6aa097b, but your framing was slightly off and the difference matters. The probe is worth having, just not for the case you named. time_shift_pk_full_name is only populated for dimensions on a calendar cube (dimension_symbol.rs:417-445 — the if is_calendar guard), so a FILTER_PARAMS bound to the fact's own time dimension has no PK to probe and can't reach the calendar map at all; a named shift on such a dimension is dropped by extract_time_shifts before it ever gets here. That's a separate pre-existing gap, not this one.

What the probe does reach is a calendar cube whose shifted dimension isn't its PK — the shift registers under the PK while the binding names the dimension. shift_is_found_when_the_binding_is_not_the_calendar_pk covers exactly that, and I confirmed it's meaningful by reverting the probe: it fails with the column rendered bare.

I did not add find_owned_by_cube_child. That probe exists to reach an owned member behind a TimeDimension wrapper, whereas this map is keyed by the PK specifically — happy to add it if you can point at a wrapper case that still slips through.

#3 (Low) — trimmed in f259ace.

#4 (Low) — both shapes now covered, per the two tests above. The fact-dimension shape you listed is the one that isn't, for the reason in #2.

On the open question: agreed, and #1 was the substance of it. Narrowing to sql-mapped shifts is what shrinks the blast radius — interval-declared calendar shifts keep planning and now emit correct SQL instead of empty stages, and only genuinely unrepresentable models get the error. Leaving reject-vs-1 = 1 as-is for the mapped case.

Docs: added a "Time-shifted measures" subsection under FILTER_PARAMS in reference/data-modeling/context-variables.mdx — why the column must carry the stage's shift, which shift kinds can be carried, and the callback form as the remedy with a worked fiscal-prior-year band. Checkbox ticked.

Planner suite: 1352 passed, 0 failed. fmt/clippy/eslint clean.

Worth flagging for whoever picks this up: every review so far has been automated. The behaviour change — models with sql-mapped calendar shifts and a string column now fail to plan where they previously returned silently-empty stages — still deserves a human call before merge.

---

Generated by Claude Code

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[claude]

Reviewing f690585…bda197c

• Check what changed since the last review
• Verify the folded calendar arms and corrected PK comment
• Verify the new non-PK test covers the or_else probe
• Review the docs addition
• Resolve stale threads / dedupe
• Post review

View job run · branch claude/filter-params-date-injection-pm2qgo

[claude]

Claude finished @paveltiunov's task in 1m 56s —— View job

---

Re-review of f690585…08f3495: all prior findings closed; 1 low remains (stale header in the YAML test)

Full review — expand for findings, docs notes and checklist

• Check what changed since the last review
• Verify the folded calendar arms and corrected PK comment
• Verify the non-PK test + tightened assertion
• Review the docs addition
• Resolve stale threads / dedupe
• Post review

What the three commits fixed

1c57118, bda197c and 08f3495 close every finding from the last two passes.

Folded arms (typed_filter.rs:95-111). One Some(Calendar(shift)) if shift.sql.is_none() arm with an inner match &shift.interval; the duplicated guard and the unwrap() are gone, and the surviving comment is the load-bearing one — the TimeShiftState-is-pre-inverted / calendar-map-is-as-written asymmetry, which is the thing a later "consistency fix" would silently reverse. The enum doc at :50-54 now distinguishes sql-declared from interval-declared correctly.

PK-probe comment (base_filter.rs:37-47). Now describes the case that can fire (filter on a non-PK dimension of the calendar) and states the fact-dimension shape as out of reach with the reason. Both halves check out against dimension_symbol.rs's if is_calendar guard.

Assertion tightening (08f3495). contains("cannot carry") && contains("prior_fiscal_year") pins the non-PK test to this rejection rather than any error mentioning the shift, which is what makes the or_else probe's coverage real. The Ok(..) => panic! arm still catches the silent-bare-column regression.

All seven of my prior inline threads are resolved; nothing to re-affirm.

Remaining

#	Severity	Where	Issue
1	Low	filter-params-calendar-time-shift.test.ts:4-17	The YAML test header still carries the pre-6aa097b claim that a calendar shift "has no such offset, so a string column cannot carry it" — the same staleness bda197c fixed on the Rust side, in the file it didn't touch. Inline comment has a five-line replacement.

Docs

The new "Time-shifted measures" subsection reads well and sits in the right place. Spot-checks: [ref-calendar-cubes]: /docs/data-modeling/concepts/calendar-cubes matches the four other files that link it, #time_shift resolves against measures.mdx:828, and the Python-lambda-in-YAML form matches the existing BigQuery-shard example at :266, so it isn't inventing syntax. "Every stage then scans every band, so keep the bands as narrow as the calendar actually requires" is the right closing caveat — it's the cost a reader following this advice actually pays.

One stylistic gap: the surrounding subsections pair YAML with JavaScript in a <CodeGroup>; this one is a bare ```yaml block. Not worth blocking on, but a JS twin would match the page.

Verification

Read-only pass — I traced the folded arm and the comment claims against dimension_symbol.rs, time_shift_state.rs and calendar_time_shift.rs, and checked the docs links against the repo. cargo test -p cubesqlplanner was not run in this pass; the PR reports 1352 passed, 0 failed.

Merge note

Standing by the earlier point, which the author also flagged: every review on this PR has been automated. The behaviour change is now narrow — only sql-mapped calendar shifts combined with a string FILTER_PARAMS column fail to plan, where they previously returned silently empty stages — but it's still a new hard planning error on models that used to compile, and deserves a human call before merge.

· branch `claude/filter-params-date-injection-pm2qgo`

[paveltiunov]

Low #1 — taken, with a correction to the premise. 94d3a60.

The page isn't dialect-neutral: _TABLE_SUFFIX / FORMAT_TIMESTAMP at :267-289 are BigQuery-only, and that section's prose says so explicitly ("add BigQuery shard filtering"). So the gap isn't that I broke a neutral-SQL convention — it's that the neighbouring example names its dialect and mine didn't, which is what leaves a Postgres reader copying a band that won't parse.

Fixed in prose rather than by rewriting the example, keeping it consistent with its neighbour: the intro now says the date arithmetic is BigQuery syntax and to adjust it, and points at the 371/364 bracketing as the part that carries across dialects. That's the load-bearing bit anyway — DATE_SUB vs - interval '371 day' is incidental.

Low #2 — leaving as is, taking you at your word that it's a judgement call. The second paragraph documents a shape that looks like it should be covered and isn't; without it the natural next question ("what about a binding on the fact's own dimension?") has no answer in the file, and the answer isn't obvious — it needs both the if is_calendar guard and the extract_time_shifts drop to explain. Happy to cut it if you feel differently.

Thanks for diffing the <CodeGroup> against origin/master's existing example and checking all three link refs resolve — that's verification I couldn't do for my own docs.

On the merge note: a human approval landed from @keydunov at 21:54, after your last pass, so that item is now closed. I'm not merging.

---

Generated by Claude Code

[claude]

Claude finished @paveltiunov's task in 2m 11s —— View job

---

Re-review of ce09d2e…94d3a60: docs-only commit; 1 medium (the new dialect label is the one dialect the example doesn't run on), 1 low

Full review — expand for findings, verification and checklist

• Check what changed since the last review
• Verify the dialect note in the docs example
• Resolve stale threads / dedupe
• Post review

What changed

One commit since the last pass, 94d3a60, taking my earlier low nit about dialect-neutrality on context-variables.mdx:411-413. No code, test or Rust changes — the planner fix, the folded calendar arm and the PK probe are untouched since 08f3495, and everything I verified in the previous two passes still stands.

Findings

#	Severity	Where	Issue
1	Medium	context-variables.mdx:411-441	The label lands on the wrong engine. For a time dimension, {x} / {y} render as TIMESTAMP(?) on BigQuery — timeStampParam returns timeStampCast('?') (BaseQuery.js:2136), and BigqueryQuery.timeStampCast is TIMESTAMP(${value}) (BigqueryQuery.ts:48). BigQuery's DATE_SUB is DATE-only, so DATE_SUB(TIMESTAMP(?), INTERVAL 371 DAY) fails with No matching signature. TIMESTAMP_SUB is the BigQuery spelling. DATE_SUB on a datetime is valid MySQL, so the unlabelled version was true of some dialect and the label is what pins it to the one it doesn't run on — either switch to TIMESTAMP_SUB or say MySQL. My earlier nit asked for the dialect to be named; naming it surfaced that the arithmetic itself needed to move.
2	Low	context-variables.mdx:425-429	First filter() callback on the page with a top-level OR (the shard example at :266 is AND-only), thirty lines above a FILTER_GROUP warning about exactly that. Correct as written since the substitution is the whole WHERE; one outer paren makes it safe for a reader who appends AND ….

Both are on the same lines the commit touched, and neither duplicates an existing thread — all eight of my prior threads are resolved and none were on this file.

Verification

Read-only pass. I traced the param rendering through BaseFilter.allocateTimestampParam → BaseQuery.timeStampParam → BigqueryQuery.timeStampCast rather than running anything; the commit under review touches only prose and two code fences, and no Rust or TypeScript changed since the suite numbers the PR reports.

Merge note

Unchanged, and the author has flagged it too: every review on this PR has been automated, and the behaviour change — an sql-mapped calendar shift with a string FILTER_PARAMS column now fails to plan where it previously returned silently empty stages — is a new hard planning error on models that used to compile. keydunov's approval is on the tree as of ce09d2e; 94d3a60 is docs-only on top of it.

· branch `claude/filter-params-date-injection-pm2qgo`

[claude]

Re-review of 94d3a60…c8a9739: the cast is correct and my earlier premise was wrong — 0 blocking, 1 low still open

Full review — expand for the correction, verification and checklist

• Check what changed since the last review
• Verify the cast against the callback param-rendering path
• Re-check the neighbouring shard example
• Dedupe against prior threads
• Post review

What changed

One commit, c8a9739, docs-only: wraps every {x}/{y} in DATE(TIMESTAMP(…)) in both the YAML and JavaScript arms of the time-shift example, and adds a prose line saying the values arrive as bare query parameters. No Rust, TypeScript or test changes — the planner fix, the folded calendar arm and the PK probe are untouched since 08f3495.

Correction to my previous finding

My last pass claimed {x} renders as TIMESTAMP(?) on BigQuery and that DATE_SUB would therefore fail. That traced the wrong path. In the callback form the params come from BaseQuery.renderFilterParams, which maps them through plain allocateParam (BaseQuery.js:5403-5407) — not allocateTimestampParam. timeStampParam/timeStampCast are only reached by the built-in filter renderers, not by a user filter() callback. So {x} is a bare placeholder, and the pre-existing shard example casting it as FORMAT_TIMESTAMP('%Y%m%d', TIMESTAMP({x})) at :266 is exactly that fact showing in the page already.

Given that, c8a9739 is right and the shape it landed on is the correct BigQuery one: TIMESTAMP(?) → DATE(...) → DATE_SUB(DATE, INTERVAL n DAY), all valid signatures, and consistent with its neighbour one section up. TIMESTAMP_SUB, which I suggested, would have been the wrong call here — it would return a TIMESTAMP compared against a DATE column. Nothing to change.

Still open

#	Where	Note
1	context-variables.mdx:427-431	The top-level OR in the callback body is still unwrapped — existing thread re-affirmed, not re-posted. Correct as written since the substitution is the whole WHERE; one outer paren makes it safe for a reader who appends AND …, thirty lines above a FILTER_GROUP warning about exactly that. Low, and a judgement call.

Eight of my nine prior threads are resolved; the one above is the only unresolved one and it still applies, so I left it rather than duplicating it.

Verification

Read-only pass. I traced renderFilterParams → allocateParam in packages/cubejs-schema-compiler/src/adapter/BaseQuery.js and compared against BaseFilter.allocateTimestampParam / BigqueryQuery.timeStampCast to establish which path a callback actually takes, and diffed the new example against the shard example on the same page. No suites were run — the commit under review touches prose and two code fences only.

Merge note

Unchanged, and now partly closed: keydunov approved at 21:54. The behaviour change remains narrow — only an sql-mapped calendar shift combined with a string FILTER_PARAMS column now fails to plan, where it previously returned silently empty stages — and the docs-only commits since sit on top of that approval.

View job run · branch claude/filter-params-date-injection-pm2qgo

[claude]

Claude finished @paveltiunov's task in 1m 50s —— View job

---

Re-review of 94d3a60…1fb4b70: docs-only; the cast correction checks out on both planners; 1 low

Full review — expand for findings, verification and checklist

• Check what changed since the last review
• Verify the DATE(TIMESTAMP({x})) cast claim against the code
• Verify the parenthesised OR band
• Resolve stale threads / dedupe
• Post review

What changed

Two commits, both on context-variables.mdx. No Rust or TypeScript changed since 08f3495, so the planner fix, the folded calendar arm and the PK probe are as verified in the previous passes.

c8a9739 — the cast. Your correction is right and mine was wrong. I traced it on both planners rather than taking it on trust:

• Tesseract: FilterParamsColumn::Callback gets its args from filter_param_values, which for DateRange/DateSingle/RollingWindowOffset uses format_and_allocate_from_date_no_cast / ..._to_date_no_cast (typed_filter.rs:205-219). Those route to format_and_allocate_date(.., cast: false), whose allocate closure calls query_tools.allocate_param directly and skips allocate_timestamp_param — the only place time_stamp_cast is applied (filter_sql_context.rs:116-152).
• Legacy: BaseQuery.renderFilterParams invokes the callback as filterParamArg.__column().apply(null, filterParams.map(allocateParam)) (BaseQuery.js:5404-5408) — bare ?, no timeStampParam in the path. allocateTimestampParam is used by the non-callback filter renderers.

So the callback receives an uncast placeholder on both paths, which is exactly why the neighbouring shard example wraps in TIMESTAMP(…), and my timeStampParam citation was the wrong call site. DATE(TIMESTAMP({x})) is well-typed for BigQuery's DATE_SUB, so keeping the "BigQuery" label is consistent. Wrapping both the comparison bounds and the subtraction arguments — not just the subtraction — is the part TIMESTAMP_SUB alone wouldn't have fixed, as you note.

One incidental benefit: DATE(TIMESTAMP(y)) truncates the upper bound, whose param value carries a 23:59:59.xxx time, to the day — which is what a DATE-typed week_end_d wants anyway.

1fb4b70 — the outer paren. Confirmed the callback result is not wrapped by the renderer for FILTER_PARAMS (renderFilterParams returns it as-is; only filterGroupFunctionImpl adds the surrounding parens for FILTER_GROUP), so the paren has to come from the model. Both fences match, and the prose note is accurate.

All ten of my prior inline threads are resolved; nothing to re-affirm and no duplicates skipped.

Remaining

#	Severity	Where	Issue
1	Low	context-variables.mdx:426-455	The example binds retail_calendar.calendar_d from inside weekly_margin.sql, but shows neither the calendar cube nor the join. Every other example on the page binds a dimension the same snippet declares, and here the join is also what makes the section's premise true. Inline comment has the two-line fix.

The three-paragraph preamble now sitting between "Pass a function instead" and the fence is on the long side and the "Two details … are easy to miss" framing reads as review residue, but both details are genuinely non-obvious and undocumented elsewhere, so I'd leave it — noting it only so a later editor knows it can be tightened.

Verification

Read-only pass; I read the two commits and traced the callback-arg path through typed_filter.rs, filter_sql_context.rs and BaseQuery.js. No suite run — the commits under review touch only prose and two code fences, and nothing executable has changed since the numbers the PR reports.

Merge note

Unchanged and now largely settled: keydunov's human approval landed on ce09d2e, and everything since is docs-only. The behaviour change it covers is the narrow one — an sql-mapped calendar shift combined with a string FILTER_PARAMS column now fails to plan where it previously returned silently empty stages.

· branch `claude/filter-params-date-injection-pm2qgo`

[paveltiunov]

CI: tests (mssql, false) failed — not this PR's

Reporting rather than fixing, with the evidence.

The failure is infrastructural. All 91 failures in the job are the same error:

ConnectionError: Login failed for user 'sa'.
    at connectListener (node_modules/mssql/lib/tedious/connection-pool.js:85:17)

Test Suites: 1 failed, 1 total / Tests: 91 failed, 56 skipped, 147 total — zero assertion failures. The MSSQL container never accepted a login, so no test body ran.

It cannot be this diff. The change is 10 files: the Rust planner (base_filter.rs, typed_filter.rs, sql_visitor.rs, visitor_context.rs, factory.rs, sql_call.rs), two test files, tests/mod.rs, and one docs page. Nothing touching MSSQL, any driver, Docker/compose, credentials, or .github/ — confirmed with git diff --name-only origin/master...HEAD.

I have not re-run it, deliberately. The failing run is on 1c57118, now six commits behind 30a9b6a. Re-running a superseded commit proves nothing; the current head is running its own tests (mssql, false), which is the copy that matters. If that one fails the same way it's the same container issue; if it fails differently I'll investigate as this PR's.

No fix exists to port — there is nothing to fix in this repo for a container that won't authenticate.

---

Docs: calendar join now shown (30a9b6a)

Separately, the review comment about the example binding retail_calendar.calendar_d without showing the join was right — unlike every other example on the page, it wasn't runnable as written. Added the joins: entry to both arms and a sentence naming what the calendar declares, since that join is also why the band needs widening. Thread reply follows.

---

Generated by Claude Code

[claude]

Claude finished @paveltiunov's task in 1m 5s —— View job

---

Re-review of 1fb4b70…30a9b6a: docs-only, the join is correct; 0 blocking, 1 low

Full review — expand for findings, verification and checklist

• Check what changed since the last review
• Verify the calendar join addition in the docs example
• Resolve stale threads / dedupe
• Post review

What changed

One commit, 30a9b6a, taking my last note about the cross-cube binding. Nothing executable changed — the planner fix, the folded calendar arm, the PK probe and both test files are untouched since 08f3495, so everything verified in the previous four passes still holds.

The join checks out

joins: [{ name: retail_calendar, sql: "{CUBE}.week_end_d = {retail_calendar.calendar_d}", relationship: many_to_one }] is the same shape as the fixture in filter_params_calendar_time_shift.rs ({fpc_margin}.week_end_d = {fpc_calendar.calendar_d}, many_to_one), so the docs example and the test now describe the same model. The JavaScript twin mirrors it with the ${CUBE} / ${retail_calendar.calendar_d} template forms the page uses elsewhere. The added sentence names what the calendar declares (calendar_d carrying prior_fiscal_year) without pulling a second cube into the fence, which keeps the snippet readable while making the binding resolvable in a reader's head — the right trade for a page whose subject is FILTER_PARAMS, not calendar cubes.

All three link refs used by the section (ref-measures-time-shift → measures#time_shift, ref-calendar-cubes, ref-env-tesseract) still resolve to definitions at :963-965.

Remaining

#	Severity	Where	Issue
1	Low	context-variables.mdx:409-422	The preamble is now four paragraphs before the fence, three of them caveats (cast the params, parenthesise the OR, the join) — stacked, they read as a gotcha list, and two of the three are visible in the snippet itself. Inline comment suggests folding the first two into one sentence after the fence. Judgement call.

All eleven of my prior threads are resolved; nothing to re-affirm and no duplicates skipped.

Verification

Read-only pass — I read the commit, compared the join against the Rust fixture and checked the link refs against the file's own definitions. No suites run: the commit touches prose and two code fences only, and nothing executable has changed since the numbers the PR reports (1352 passed, 0 failed).

Merge note

Settled since keydunov's approval on ce09d2e; the five commits since are all docs-only. The behaviour change under that approval is the narrow one — an sql-mapped calendar shift combined with a string FILTER_PARAMS column now fails to plan where it previously returned silently empty stages.

· branch `claude/filter-params-date-injection-pm2qgo`

[paveltiunov]

CI: four more failures, all on superseded commits, none this PR's

Following the tests (mssql, false) note above. All four are on commits behind the current head 30a9b6a.

check	commit	what actually failed
Build windows-2022 24.x	f259ace	Rust build succeeded — the log shows the planner running (SQL API Query Planning Success, Load Request Success). Tests: 11 skipped, 16 passed, 27 total — zero test failures. The suite failed on "A worker process has failed to exit gracefully… tests leaking due to improper teardown". A Jest teardown leak in cubejs-backend-native, not an assertion.
tests (mssql, false)	1c57118	91/91 ConnectionError: Login failed for user 'sa', zero assertions (detailed above).
tests (mssql, true)	f690585	Same mssql-full suite against the same container.
integration (firebolt, …) ×2	f690585, 6aa097b	@cubejs-backend/firebolt-driver:integration:firebolt — external cloud warehouse, credentials-dependent.

The Windows one is the only one that could plausibly have been mine, since it compiles the Rust I changed, so I opened it first. It compiled and my planner code ran successfully inside it — the failure is downstream of anything this diff touches.

On re-running: these jobs are wrapped in nick-fields/retry and each log ends Final attempt failed, so CI has already spent the retry. Re-running superseded commits proves nothing anyway; the current head is running its own copies, and those are the ones I'll act on. If any of them fails with an assertion rather than a connection or teardown error, I'll treat it as this PR's.

No fix to port: there is nothing in this repo to fix for a container that won't authenticate, a cloud warehouse without credentials, or a Jest worker that won't exit.

For the record on scope — git diff --name-only origin/master...HEAD is 10 files: six Rust planner sources, two test files, tests/mod.rs, and one docs page. Nothing under packages/cubejs-mssql-driver, packages/cubejs-firebolt-driver, .github/, or any Docker/compose file.

---

Generated by Claude Code

[claude]

Claude finished @paveltiunov's task in 36s —— View job

---

Re-review of 30a9b6a…3dea13a: docs-only, exactly the change requested; no findings

Full review — expand for verification and checklist

• Check what changed since the last review
• Verify the moved cast/paren notes read correctly in their new position
• Confirm nothing executable changed
• Resolve stale threads / dedupe
• Post review

What changed

One commit, 3dea13a, on docs-mintlify/reference/data-modeling/context-variables.mdx only — 4 insertions, 6 deletions. It takes my last note verbatim: the five-line "two details … are easy to miss" preamble is gone, and the cast and paren points are folded into a single sentence after the <CodeGroup> (:467-469). The join sentence stays before the fence, which is right — it's the one thing the snippet can't show, since the calendar cube isn't in it.

Net effect on the section: two paragraphs of explanation and one of setup before the example, then the example, then one sentence of after-the-fact detail and the narrowing caveat. That's the shape the rest of the page uses.

The relocated sentence still reads correctly out of its original order — "The values arrive as bare query parameters, so they are cast before use" lands better after a reader has seen DATE(TIMESTAMP({x})) than before, since it now explains something visible rather than pre-warning about it.

Verification

git diff 30a9b6a..HEAD --stat confirms the single file. Nothing under rust/ or packages/ has changed since 08f3495, so the planner fix, the folded calendar arm, the PK probe and both test files are as verified in the previous five passes. No suites run — the commit touches prose only.

All twelve of my prior inline threads are resolved, including the two on this file; nothing to re-affirm, nothing to dedupe against, and no new inline comments.

Merge note

Settled: keydunov approved on ce09d2e, and the six commits since are all docs-only. The behaviour change under that approval remains the narrow one — an sql-mapped calendar shift combined with a string FILTER_PARAMS column now fails to plan where it previously returned silently empty stages.

· branch `claude/filter-params-date-injection-pm2qgo`

[paveltiunov]

Addendum to the two CI notes above — Upload merged coverage to Codecov failed on f259ace and 1c57118, both superseded.

##[error]Unable to download artifact(s): Unable to download and extract artifact:
Artifact download failed after 5 retries.

GitHub's artifact service, not Codecov — the job never got as far as uploading. It downloads coverage artifacts produced by the earlier jobs, several of which failed for the container/credential reasons already reported, and the action exhausted its own five retries, so there's no re-run left to spend.

Same conclusion as before: nothing in this diff to fix, and both runs are behind the current head 3dea13a. That's seven failing checks now, every one either an environment failure (MSSQL auth, firebolt credentials, artifact download) or a harness failure (Jest worker teardown) — no assertion failures anywhere, on any commit.

---

Generated by Claude Code

[paveltiunov]

CI: tests (bigquery-export-bucket-gcs, false) — a real test failure, still not this PR's

Flagging this one separately because unlike the previous seven it is an actual test failure, not a connection or harness error, so it deserved a closer look.

Tests: 1 failed, 34 skipped, 115 passed, 150 total
Snapshots: 99 passed, 99 total

● Queries with the @cubejs-backend/bigquery-driver › must built pre-aggregations
  thrown: "Cube pre-aggregations build failed: failure: Not found: Table
  cube-open-source:dev_pre_aggregations.big_e_commerce__count_by_product_external20200401_…"

BigQuery connected, 115 tests passed, and all 99 snapshots matched — so SQL generation is unchanged. The one failure is a pre-aggregation build not finding its table in the shared cube-open-source:dev_pre_aggregations dataset.

Why it can't be this diff — the decisive check. My change is gated at the top of BaseFilter::to_sql:

if !filters_ctx.filter_params_columns.is_empty() { … }

Everything I touched lives inside that branch. And packages/cubejs-testing-drivers/fixtures/_schemas.json contains no FILTER_PARAMS at all — I walked every string value in the file. So filter_params_columns is empty for every query in this suite and the new code never executes.

Worth noting the fixtures do include a RetailCalendar cube with calendar: true and timeShift entries, which is why I checked rather than assuming — that's the half of the interaction my change cares about. But my change fires only where a calendar shift meets a FILTER_PARAMS binding, and the second half is absent.

dev_pre_aggregations is a shared dataset that concurrent CI runs build into, and a missing pre-agg table there is a build/cleanup collision rather than a query defect. The job is wrapped in nick-fields/retry and the log ends Final attempt failed, so the retry is already spent.

Both runs (f259ace, 1c57118) are behind the current head 3dea13a.

---

Generated by Claude Code

[paveltiunov]

CI: tests (bigquery-export-bucket-gcs, true) — same root cause, now with direct proof it's non-deterministic

Failed on ce09d2e, superseded (current head 3dea13a). This is the true sibling of the leg I wrote up above; same cause, but this run gives evidence the earlier one didn't, so it's worth recording.

The nick-fields/retry action ran the suite three times on the identical commit. Each attempt failed a different set of tests, on different pre-aggregations:

attempt	failed	pre-aggregations that went missing
1	4	ec__t_a_external20200201, …20200101 (x2), …20200501
2	5	ec__s_a_external, ec__t_a_external20200301, …20200601, big_e_commerce__multi_time_dim_for_count_external20200301, …20200401
3	3	big_e_commerce__multi_time_dim_for_count_external20200601, …20201201, big_e_commerce__category_flat_external

Every failure is the same shape — Not found: Table cube-open-source:dev_pre_aggregations.<name>_<hash>_<hash>_<token> — and in each attempt the first failure is the must built pre-aggregations step itself, with the query tests after it failing downstream on the table that step never left behind.

Two things follow. The trailing token increments across attempts (1l9mga4 → 1l9mgbn → 1l9mgcm → 1l9mgfv → 1l9mgn3 → 1l9mgol), so each attempt really did build fresh tables into the shared cube-open-source:dev_pre_aggregations dataset — and a different one vanished between build and query each time. A deterministic planner defect would fail the same tests on every attempt. This is concurrent runs colliding in one shared BigQuery dataset, which is exactly what many overlapping runs on this branch would produce.

Independently: this suite cannot reach the code I changed. My new branch sits inside the pre-existing if !filters_ctx.filter_params_columns.is_empty() guard in base_filter.rs, and packages/cubejs-testing-drivers/ contains no FILTER_PARAMS at all — grep -rn "FILTER_PARAMS\|filterParams" over the whole package returns nothing. I checked rather than assumed, because those fixtures do define a RetailCalendar cube with calendar: true and timeShift — half of the interaction this PR targets, but never combined with a filter param.

No re-run to spend: the retry action exhausted its own three attempts and reported Final attempt failed.

That's eight failing checks, none with an assertion failure attributable to this diff — environment (MSSQL auth, firebolt credentials, artifact download), harness (Jest worker teardown), or this shared-dataset race. A fresh full run is in flight on 3dea13a now.

---

Generated by Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.21%. Comparing base (1d08fc0) to head (3dea13a).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11772      +/-   ##
==========================================
- Coverage   80.21%   80.21%   -0.01%     
==========================================
  Files         500      500              
  Lines      105622   105622              
  Branches     3884     3884              
==========================================
- Hits        84728    84724       -4     
- Misses      20344    20348       +4     
  Partials      550      550              

Flag	Coverage Δ
cube-backend	60.11% <ø> (ø)
cubesql	84.67% <ø> (-0.01%)	⬇️

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.

[paveltiunov]

CI: tests (bigquery-export-bucket-gcs, true) is now red on the head 3dea13a — same race, and one failure name that needs pre-empting

Following up my earlier note only because this check has now reached the current head, where a reviewer will see it red.

Identical signature, and non-deterministic in the same way — three retry attempts on this one commit, each failing a different set:

attempt	failed	first failure
1	8	must built pre-aggregations → Not found: Table ec__s_a_external_o2swucpv_…
2	3	must built pre-aggregations → Not found: Table ec__t_a_external20201001_…
3	3	must built pre-aggregations → Not found: Table ec__t_a_external20201201_…

Every failure is Not found: Table cube-open-source:dev_pre_aggregations.…, with the build step failing first and the query tests failing downstream on tables it never left behind. Attempt 1 failed eight tests; two attempts later, on the same code, five of those eight passed.

The one thing worth calling out. Attempt 1's list includes:

● Tesseract: SQL API: Timeshift measure from cube
  error: Database Execution Error: Not found: Table ec__t_a_external20200801_odndd3lp_3pxplc0q_1l9mhab

Given this PR is about time shifts, that name deserves a second look rather than a wave-through — so: it is the same missing-table error, not a wrong-SQL or assertion failure, and it passed on attempts 2 and 3 of this identical commit. It is the dataset race catching one more test, not this diff.

The reachability argument is unchanged and independent of all of the above: the new branch sits inside the pre-existing if !filters_ctx.filter_params_columns.is_empty() guard, and grep -rn "FILTER_PARAMS\|filterParams" packages/cubejs-testing-drivers/ returns nothing — this suite has no filter params anywhere, so the changed code cannot execute in it.

No re-run to spend: the retry action exhausted its own three attempts and reported Final attempt failed.

Everything else on 3dea13a is green so far, including unit (24.x, 3.13) (which runs this PR's new schema-compiler test), unit-core, integration-cubestore, integration-smoke (24.x, 3.13, true), Check fmt/clippy, lint, build, codecov/patch and codecov/project.

---

Generated by Claude Code