fix(schema-compiler): resolve time dimension granularity columns in pre-aggregation indexes#10894
fix(schema-compiler): resolve time dimension granularity columns in pre-aggregation indexes#10894paveltiunov wants to merge 5 commits into
Conversation
…re-aggregation indexes When using time dimension columns with granularity in pre-aggregation indexes (e.g., CUBE.timestamp.hour), the generated column alias used double underscores (orders__timestamp__hour) instead of the correct format with a single underscore before the granularity (orders__timestamp_hour), causing a column name mismatch with the actual pre-aggregation table column. The fix detects 3-part column paths (cube.dimension.granularity) in evaluateIndexColumns and generates the alias consistent with BaseTimeDimension.unescapedAliasName(). Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
|
Claude finished @paveltiunov's task in 3m 9s —— View job
Review SummaryOverall: This is a clean, well-targeted fix. The bug diagnosis is accurate and the fix is minimal. CorrectnessThe root cause is correctly identified: The fix correctly mirrors the
The guard conditions are solid:
Both consumers of I left one inline comment about a minor Test CoverageThe two new tests are well-structured:
Negative assertions ( Potential additions to consider (non-blocking):
Security & PerformanceNo concerns. The fix adds a few extra checks (length check, |
…index columns Update both Mintlify and legacy docs to show the new CUBE.<time_dimension>.<granularity> syntax as the primary way to reference time dimension columns in pre-aggregation indexes. The previous string-based format (e.g., orders__created_at_day) is kept as an alternative approach. Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
| if (path.length === 3 && this.cubeEvaluator.isDimension(path.slice(0, 2))) { | ||
| const dimensionPath = `${path[0]}.${path[1]}`; | ||
| const dimensionDef = this.cubeEvaluator.dimensionByPath(dimensionPath); | ||
| if (dimensionDef.type === 'time' && | ||
| this.cubeEvaluator.resolveGranularity([path[0], path[1], 'granularities', path[2]])) { | ||
| return `${this.aliasName(dimensionPath)}_${path[2]}`; | ||
| } |
There was a problem hiding this comment.
Minor: BaseTimeDimension.unescapedAliasName() checks this.query.options.memberToAlias?.[fullName] (where fullName = dimension + '.' + granularity) before falling through to the aliasName(dimension) + '_' + granularity format. This new code path skips that check.
In practice this likely doesn't matter for pre-aggregation index columns since memberToAlias is typically set for query-level aliasing, not pre-aggregation evaluation. But if you want exact parity with BaseTimeDimension.unescapedAliasName():
| if (path.length === 3 && this.cubeEvaluator.isDimension(path.slice(0, 2))) { | |
| const dimensionPath = `${path[0]}.${path[1]}`; | |
| const dimensionDef = this.cubeEvaluator.dimensionByPath(dimensionPath); | |
| if (dimensionDef.type === 'time' && | |
| this.cubeEvaluator.resolveGranularity([path[0], path[1], 'granularities', path[2]])) { | |
| return `${this.aliasName(dimensionPath)}_${path[2]}`; | |
| } | |
| if (path.length === 3 && this.cubeEvaluator.isDimension(path.slice(0, 2))) { | |
| const dimensionPath = `${path[0]}.${path[1]}`; | |
| const dimensionDef = this.cubeEvaluator.dimensionByPath(dimensionPath); | |
| if (dimensionDef.type === 'time' && | |
| this.cubeEvaluator.resolveGranularity([path[0], path[1], 'granularities', path[2]])) { | |
| const fullName = `${dimensionPath}.${path[2]}`; | |
| if (this.options.memberToAlias?.[fullName]) { | |
| return this.options.memberToAlias[fullName]; | |
| } | |
| return `${this.aliasName(dimensionPath)}_${path[2]}`; | |
| } | |
| } |
Nit — feel free to disregard if memberToAlias is never populated for index column evaluation paths.
…th granularity columns Add an E2E test that verifies time dimension columns with granularity (CUBE.createdAt.day) work correctly in pre-aggregation index definitions. The test: - Starts a Postgres container with Birdbox - Defines a cube with a pre-aggregation that has indexes using CUBE.createdAt.day syntax - Queries data through the pre-aggregation to verify it builds and returns correct results - Checks the system API partitions endpoint to verify the index SQL uses the correct column alias (single underscore before granularity) Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10894 +/- ##
==========================================
+ Coverage 58.36% 58.38% +0.01%
==========================================
Files 216 216
Lines 16944 16949 +5
Branches 3435 3435
==========================================
+ Hits 9890 9895 +5
Misses 6552 6552
Partials 502 502
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…test The cube name OrdersPAIndexGranularity gets underscored as orders_p_a_index_granularity (not orders_pa_index_granularity) by inflection.underscore. Fixed the expected strings to match. Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
…tgres Merge the index granularity E2E tests into the existing smoke-postgres.test.ts instead of having a separate test file. Both share the same Birdbox setup (smoke/schema dir, smoke/cube.js). Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
Problem
When using time dimension columns with granularity references in pre-aggregation indexes (e.g.,
CUBE.timestamp.hour), the generated column alias was incorrect:orders__created_at_hour(single underscore before granularity, matching the pre-aggregation table column name)orders__created_at__hour(double underscore before granularity)This happened because
evaluateIndexColumnsinBaseQuery.jstreated the full 3-part dotted path throughaliasName(), which converts all dots to double underscores. However, the actual pre-aggregation table column is generated byBaseTimeDimension.unescapedAliasName(), which uses the format{dimensionAlias}_{granularity}with a single underscore separator.The mismatch caused index creation to fail or reference non-existent columns.
Fix
In
evaluateIndexColumns, when a 3-part column path is detected where:...the alias is now generated as
aliasName(cube.dimension) + '_' + granularity, matching the format used byBaseTimeDimension.unescapedAliasName().Documentation
Updated both Mintlify and legacy docs to show the new
CUBE.<time_dimension>.<granularity>syntax as the primary way to reference time dimension columns in pre-aggregation indexes. The previous string-based format (orders__created_at_day) is kept as a documented alternative.Tests
Unit tests (
cubejs-schema-compiler):CUBE.created_at.hourin index columns produces"orders__created_at_hour"created_at.hourin YAML index columns produces the same correct aliasindexesSqlandcreateTableIndexesE2E tests (added to existing
smoke-postgres.test.tsincubejs-testing):All unit and E2E tests pass.