fix(schema-compiler): handle sqlTable in originalSql pre-aggregation

[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

When a cube uses sqlTable instead of sql, the preAggregationSql method for originalSql type pre-aggregations crashes with:

TypeError: Cannot read properties of undefined (reading 'apply')
    at CubeEvaluator.resolveSymbolsCall (CubeSymbols.ts:1015:24)
    at RedshiftQuery.evaluateSql (BaseQuery.js:3448:26)
    ...

This happens during /cubejs-system/v1/pre-aggregations/partitions calls when the cube definition uses sqlTable instead of sql.

Root cause: In BaseQuery.preAggregationSql(), the originalSql branch called evaluateSql(cube, cubeFromPath.sql) directly, but cubeFromPath.sql is undefined for cubes that use sqlTable.

Fix: For the sqlTable case, reuse the existing cubeSql() method (which already handles sqlTable correctly) to resolve the table name and wrap it in SELECT * FROM. For the sql case, keep the existing evaluateSql() call unchanged:

if (cubeFromPath.sqlTable) {
  return `SELECT * FROM ${originalSqlPreAggregationQuery.cubeSql(cube)}`;
}
return originalSqlPreAggregationQuery.evaluateSql(cube, cubeFromPath.sql);

Tests added:

• Unit tests: Two tests covering non-partitioned and partitioned originalSql pre-aggregations on cubes that use sqlTable, verifying preAggregationSql() produces valid SELECT * FROM SQL.
• Integration test: A Postgres integration test that runs the full pre-aggregation query via dbRunner.evaluateQueryWithPreAggregations(), verifying correct data retrieval through the originalSql pre-aggregation with sqlTable.

  

[claude]

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

---

Code Review: PR #10824

• Gather context and review the diff
• Review the fix in BaseQuery.js
• Review the tests in pre-aggregations.test.ts
• Check for edge cases and related code paths
• Post final review

---

Verdict: This PR looks good. The fix is correct, minimal, and well-tested. A few observations below.

---

Fix Analysis (BaseQuery.js:4264-4265)

The root cause is accurately identified: preAggregationSql() unconditionally accessed cubeFromPath(cube).sql, which is undefined when a cube uses sqlTable instead of sql. This undefined gets passed to evaluateSql() → resolveSymbolsCall(undefined, ...) → undefined.apply(...), causing the crash.

The fix:

const cubeFromPath = this.cubeEvaluator.cubeFromPath(cube);
const cubeSqlFn = cubeFromPath.sqlTable || cubeFromPath.sql;

This correctly mirrors the existing pattern in cubeSql() at line 2636-2641, which already handles sqlTable with the same precedence. The xor('sql', 'sqlTable') validation in CubeValidator.ts:1113 guarantees exactly one of the two is always defined, so the || fallback is safe and doesn't need additional null-checking.

One minor observation: The cubeSql() method (line 2624) has additional logic that wraps sql-based cubes in parentheses (for subqueries like SELECT * FROM ...) vs returning sqlTable values directly. The preAggregationSql path doesn't need this distinction because evaluateSql + resolveSymbolsCall handles the function evaluation uniformly — both sql and sqlTable are functions that get .apply()'d. So the simpler fallback here is correct.

Test Review

The two new tests cover the right scenarios:

1. Non-partitioned originalSql with sqlTable — validates the basic fix path
2. Partitioned originalSql with sqlTable + timeDimension — validates the partition variant

Both assert that loadSql contains public.orders, confirming the sqlTable value flows through correctly.

Minor suggestions (non-blocking):

• The tests could additionally verify originalSqlDesc.loadSql[0] does NOT contain a wrapping subquery (i.e., no SELECT *), which would confirm sqlTable is treated as a table reference rather than a SQL expression. But the current assertions are sufficient to prevent the regression described.

No Other Instances Found

I checked for other places in BaseQuery.js that access cubeFromPath(...).sql without considering sqlTable — no other vulnerable call sites exist. The only other cubeFromPath.*.sql reference (line 4105-4107) is for refreshKey.sql, which is a different property.

Summary

Clean, targeted fix that resolves a real crash. The approach is consistent with existing patterns, the validation layer guarantees safety of the || fallback, and test coverage is adequate.

[claude]

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

---

Code Review: PR #10824

• Gather context and review the diff
• Review the fix in BaseQuery.js
• Review unit tests
• Review integration tests
• Check for edge cases and related code paths
• Post final review

---

Verdict: This PR looks good. The fix is correct, minimal, and well-tested. Detailed analysis below.

---

Fix Analysis (BaseQuery.js:4264-4273)

The root cause is accurately identified: preAggregationSql() unconditionally accessed cubeFromPath(cube).sql, which is undefined when a cube uses sqlTable instead of sql. The undefined gets passed through evaluateSql() → resolveSymbolsCall(undefined, ...) → undefined.apply(...), causing the TypeError.

The fix correctly:

1. Extracts cubeFromPath once (line 4264) — avoids a redundant call on line 4275 that existed before. Good refactor.

2. Checks sqlTable first (line 4265) — mirrors the precedence in cubeSql() at line 2637. The xor('sql', 'sqlTable') validation in CubeValidator.ts:1113 guarantees exactly one is always defined, so the branching is safe.

3. Wraps with SELECT * FROM (line 4269) — semantically correct. The sql branch produces a full SELECT query (since cubeFromPath.sql is a function like () => `SELECT * FROM orders`). The sqlTable branch only yields a table name, so wrapping it with SELECT * FROM produces equivalent output for the pre-aggregation's CREATE TABLE AS statement.

4. Preserves evaluateSymbolSqlWithContext wrapper (line 4266) — correctly maintains the preAggregationQuery and collectOriginalSqlPreAggregations context, which is needed for the symbol resolution to work properly in nested pre-aggregation scenarios.

No Other Vulnerable Call Sites

I checked all cubeFromPath(...).sql accesses in BaseQuery.js:

• Line 4105: cubeFromPath.refreshKey.sql — this is a different property (refreshKey.sql, not cube-level sql), so it's unaffected.
• Line 4275: The original cubeFromPath.sql access, now only reached when sqlTable is not set — safe due to the xor validation.

Test Review

Unit tests — Two well-structured tests covering both non-partitioned and partitioned originalSql pre-aggregations with sqlTable. The assertions check that:

• sql[0] contains SELECT * FROM public.orders
• loadSql[0] contains CREATE TABLE
• loadSql[0] contains SELECT * FROM public.orders

This is sufficient to verify the fix and prevent regression.

Integration test — Runs the full pre-aggregation pipeline via dbRunner.evaluateQueryWithPreAggregations(), confirming correct data retrieval through the originalSql pre-aggregation with sqlTable. The console.log calls follow existing patterns in the file. The use of timeDimensionReference (instead of the newer timeDimension) is consistent with the rest of the integration test file's conventions.

Minor Observations (Non-blocking)

1. The sql path (line 4274-4277) and the sqlTable path (line 4265-4273) share the same buildSqlAndParams + evaluateSymbolSqlWithContext wrapping structure. These could theoretically be unified by computing the SQL callback first and sharing the wrapping, but the current code is clear and the duplication is minimal — not worth changing.

2. If a cube had both sql and sqlTable defined, the sqlTable branch would take precedence. But this is an impossible state due to the xor validation, so there's no practical concern.

---

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.16%. Comparing base (2baf0bc) to head (1237bb5).
⚠️ Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10824      +/-   ##
==========================================
+ Coverage   58.09%   58.16%   +0.06%     
==========================================
  Files         216      216              
  Lines       16899    16942      +43     
  Branches     3426     3436      +10     
==========================================
+ Hits         9818     9854      +36     
- Misses       6578     6585       +7     
  Partials      503      503              

Flag	Coverage Δ
cube-backend	58.16% <100.00%> (+0.06%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.