[pull] master from cube-js:master - #705
Merged
Merged
Conversation
… ignores the calendar (#11709) * fix(schema-compiler): reject sql granularities under a name of their own (CORE-780) A granularity declared with `sql` carries no interval — the validator forbids combining `sql` with `interval`. `resolveGranularity()` supplies a synthetic `1 <name>` interval for predefined names only, so a `sql` granularity named after something else resolved without one, and `Granularity` read it unguarded. `granularityHierarchies()` builds a `Granularity` for every custom granularity of every time dimension in the model, and pre-aggregation matching runs it on every request regardless of the SQL planner. A single such granularity anywhere in the model therefore failed every query of the deployment, including queries that never referenced the cube declaring it. Such a granularity is now rejected at compile time, naming the granularity and the predefined names it may take. The check is reported outside the cube schema so it stands on its own: rejecting the granularity within the schema lists it among the reasons every other dimension alternative failed, which buries it. As defense in depth, `granularityHierarchies()` skips granularities that have no interval to derive a hierarchy from — rollups can only match those by name, which a missing hierarchy entry already expresses — and `Granularity` reports a readable error instead of reading the interval unguarded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tesseract): bound a to_date window by the calendar, not by interval math A calendar cube's granularity is honored where the time dimension is projected, but a `to_date` rolling window bounded itself with `date_bin(interval, point, origin)` — the granularity's synthetic `1 <name>` interval anchored at the start of the current year. A retail week-to-date therefore reset on the weekday that year started on rather than on the week the calendar defines, silently, with no error to notice. The period a point belongs to cannot be derived from the point: only the calendar knows it. The series driving such a window is now read off the calendar cube, pairing every point with the period it falls into, and both bounds take it from there — the window's join condition and the lower bound widening the scan of its source. The period ends where the next one starts, read from the next point of the series, because a 4-5-4 month runs 28 or 35 days against a nominal `1 month`. Rolling windows share one series, so its points carry one boundary column per granularity and each window reads its own: week-to-date and month-to-date over the same calendar resolve independently. A regular trailing window on that series is unaffected. Series that no calendar window drives are untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: revert the calendar granularity note Documenting the naming rule belongs with the docs owners, not here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tesseract): let the calendar series look past its own range restriction A period ends where the next one starts, and the point opening that next period may sit outside the queried range — a range closing exactly on a period end left the last point with no next row to read, falling back to the nominal interval it was there to replace. A 35-day retail month closing the range came back four days short. The period bounds are now derived over the unrestricted calendar and the range is applied outside that select, so the point past the range still bounds the last one inside it. Both selects render through the sql templates, so dialects overriding statement rendering apply to the calendar series too. Also addresses review notes: - a granularity named `Week` with `sql` resolves, because predefined names are matched case-insensitively; the new rejection matched case-sensitively and would have failed a model that works today - the predefined names quoted in that message now come from the set backing the check, rather than a fourth copy of it - a cube whose granularities were rejected is no longer recorded as valid - `granularityHierarchies()` skips a granularity that resolves without an interval, but keeps reporting one that does not resolve at all - `Granularity` no longer blames `sql` for an interval missing for another reason - a calendar period dimension without a granularity is an internal error rather than a column name the consuming side cannot match - the ordering the time series' granularity list rests on is now stated where the list is declared and where the loops it depends on run Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(tesseract): cover calendar to_date windows in the planner suite The feature was only covered from the schema compiler's Postgres suite, which leaves the planner's own integration suite silent about it. The retail calendar fixture there already runs 4-5-4 months, so the case a nominal interval cannot reproduce needs nothing but exposing that column as a granularity. `month` is exposed on the primary key dimension only: an existing test reads `retail_date` at `month` and must keep seeing a natural one. Five cases, all failing before the window learned to read its bounds off the calendar — the retail month then bucketed the rolling measure by calendar months while the plain measure kept retail ones, so the two axes never met and half of each row came back NULL: - week-to-date resetting on the retail week rather than the ISO one - month-to-date resetting on the retail month rather than the first of the month - a 5-week month grouped by itself, where a nominal bound folds the next one in - a range closing exactly on a period end, whose bounding point is outside it - both windows at once, the weekly one restarting while the monthly accumulates Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tesseract): keep the calendar period a range opens inside of The series restricted itself with the aligned date range, whose start `get_range_for_time_series` snaps to the granularity's interval — for a calendar granularity, anchored at the start of the current year, the very arithmetic the period bounds no longer use. That point lands inside the period the range opens in, and `date_from >= range_from` then drops that period from the series: a month-to-date over a range opening on 2024-04-01 lost the retail month running 2024-03-03..2024-04-06 entirely, reporting NULL where the period had five orders. The calendar branch now takes the range as stated and keeps a period by overlap rather than by where it starts, which is also what the generated series does — it starts at the aligned point and labels it with that period's start. Two of the existing cases had nothing but NULLs after the boundary they were pinning, so a bound that collapsed to an empty window for the rest of the range would have satisfied them; both now extend far enough for the new period to count again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
#11712) * fix(tesseract): convert a view's raw time dimension timezone only once A `type: time` dimension requested without a granularity was converted into the query timezone twice when it was reached through a view: the view member applied the conversion and then rendered through the owning cube's dimension, which applied it again. On BigQuery this produced `TIMESTAMP(DATETIME(TIMESTAMP(DATETIME(col, tz)), tz))`, shifting the value by the offset twice and moving timestamps across day boundaries. Gate the conversion on `owned_by_cube()`: only the cube that owns the column reads it from the database, so only there is a conversion needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(tesseract): address review notes on the timezone conversion tests Move the plain-cube guard next to the other raw time dimension coverage in time_dimensions.rs, leaving views.rs to the cases that involve a view. Say in the date-bound table which reading of a bare date is the consistent one, so a later change to either planner is self-explanatory. Drop an eslint suppression the file does not need. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(tesseract): pin the converted values, not just how many conversions The count says how many timezone conversions a query applies, but not where they land: moving one onto the granularity wrapper, or applying it with the offset reversed, keeps the count intact. Run the two view queries against Postgres and snapshot the rows as well, so the column the conversion lands on and the direction of the shift are pinned too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(tesseract): let the environment pick the planner for the tz tests The suite already runs under both planners through CUBEJS_TESSERACT_SQL_PLANNER, so selecting one inside the test duplicated that coverage and made the file carry a per-planner expectation table. Write each case once and let the environment decide. The date-bound cases are dropped with the table: their expected value differs per planner, so they cannot be stated once. The bound a single-date operator picks is covered on the Rust side. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Alex Qyoun-ae <4062971+MazterQyou@users.noreply.github.com>
github-actions
Bot
added
rust
data source driver
cubejs-jdbc-driver
driver:athena
driver:bigquery
driver:clickhouse
driver:crate
driver:databricks
driver:dremio
driver:druid
driver:duckdb
driver:firebolt
driver:hive
driver:materialize
driver:mongodb
driver:mssql
driver:mysql
driver:mysql-aurora-serverless
driver:oracle
driver:pinot
driver:postgres
driver:prestodb
labels
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )