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: Reuse the existing cubeSql() method which already handles both sqlTable and sql cubes correctly, instead of accessing cubeFromPath.sql directly:

return this.paramAllocator.buildSqlAndParams(originalSqlPreAggregationQuery.evaluateSymbolSqlWithContext(
  () => `SELECT * FROM ${originalSqlPreAggregationQuery.cubeSql(cube)}`,
  { preAggregationQuery: true, collectOriginalSqlPreAggregations }
));

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.

---