Skip to content

Commit a3f6592

Browse files
mcheshkovmcheshkov
committed
fix(schema-compiler): Use segments as-is in rollupPreAggregation
This is to support member expression segments, as BaseFilter does not expect member expressions in it, i.e. in `filterToWhere` is always call `dimensionSql`
1 parent 8c4566c commit a3f6592

File tree

4 files changed

+113
-42
lines changed

4 files changed

+113
-42
lines changed

‎packages/cubejs-schema-compiler/src/adapter/MssqlQuery.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import moment from 'moment-timezone';
44
import { QueryAlias, parseSqlInterval } from '@cubejs-backend/shared';
55
import { BaseQuery } from './BaseQuery';
66
import { BaseFilter } from './BaseFilter';
7+
import { BaseSegment } from './BaseSegment';
78
import { ParamAllocator } from './ParamAllocator';
89

910
const abbrs = {
@@ -53,11 +54,33 @@ class MssqlFilter extends BaseFilter {
5354
}
5455
}
5556

57+
class MssqlSegment extends BaseSegment {
58+
public filterToWhere(): string {
59+
const where = super.filterToWhere();
60+
61+
const context = this.query.safeEvaluateSymbolContext();
62+
if (context.rollupQuery) {
63+
// Segment itself will be rendered as reference for rollupQuery
64+
// In MSSQL using just `WHERE (segment_column) AND (other_filter)` is incorrect, because
65+
// `segment_column` is not of boolean type, but of `BIT` type
66+
// Correct way to work with them is to use `WHERE segment_column = 1`
67+
// This relies on `wrapSegmentForDimensionSelect` mapping segment to a `BIT` data type
68+
return `${where} = 1`;
69+
}
70+
71+
return where;
72+
}
73+
}
74+
5675
export class MssqlQuery extends BaseQuery {
5776
public newFilter(filter) {
5877
return new MssqlFilter(this, filter);
5978
}
6079

80+
public newSegment(segment): BaseSegment {
81+
return new MssqlSegment(this, segment);
82+
}
83+
6184
public castToString(sql) {
6285
return `CAST(${sql} as VARCHAR)`;
6386
}

‎packages/cubejs-schema-compiler/src/adapter/PreAggregations.js‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1256,11 +1256,8 @@ export class PreAggregations {
12561256

12571257
const from = this.query.joinSql(toJoin);
12581258

1259-
const segmentFilters = this.query.segments.map(
1260-
s => this.query.newFilter({ dimension: s.segment, operator: 'equals', values: [true] })
1261-
);
12621259
const replacedFilters =
1263-
filters || segmentFilters
1260+
filters || this.query.segments
12641261
.concat(this.query.filters).concat(
12651262
this.query.timeDimensions.map(dimension => dimension.dateRange && ({
12661263
filterToWhere: () => this.query.timeRangeFilter(

‎packages/cubejs-schema-compiler/test/integration/postgres/PostgresDBRunner.js‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,19 @@ export class PostgresDBRunner extends BaseDbRunner {
2525
return {
2626
testQueries(queries, prepareDataSet) {
2727
prepareDataSet = prepareDataSet || defaultFixture;
28-
return db.tx(tx => tx.query('SET TIME ZONE \'UTC\'')
29-
.then(() => prepareDataSet(tx)
30-
.then(() => queries
31-
.map(([query, params]) => () => tx.query(query, params).catch(e => {
32-
throw new Error(`Execution failed for '${query}', params: ${params}: ${e.stack || e}`);
33-
})).reduce((a, b) => a.then(b), Promise.resolve()))
34-
.then(r => JSON.parse(JSON.stringify(r)))));
28+
return db.tx(async tx => {
29+
await tx.query('SET TIME ZONE \'UTC\'');
30+
await prepareDataSet(tx);
31+
let lastResult;
32+
for (const [query, params] of queries) {
33+
try {
34+
lastResult = await tx.query(query, params);
35+
} catch (e) {
36+
throw new Error(`Execution failed for '${query}', params: ${params}: ${e.stack || e}`);
37+
}
38+
}
39+
return JSON.parse(JSON.stringify(lastResult));
40+
});
3541
},
3642
close() {
3743
return pgp.end();

‎packages/cubejs-schema-compiler/test/integration/postgres/sql-generation.test.ts‎

Lines changed: 76 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { getEnv } from '@cubejs-backend/shared';
22
import { UserError } from '../../../src/compiler/UserError';
3+
import type { BaseQuery } from '../../../src';
34
import { PostgresQuery } from '../../../src/adapter/PostgresQuery';
45
import { BigqueryQuery } from '../../../src/adapter/BigqueryQuery';
56
import { PrestodbQuery } from '../../../src/adapter/PrestodbQuery';
@@ -732,6 +733,27 @@ SELECT 1 AS revenue, cast('2024-01-01' AS timestamp) as time UNION ALL
732733
);
733734
}
734735

736+
type QueryWithParams = [string, Array<unknown>];
737+
738+
async function testWithPreAgg(
739+
preAggregationsDescription: { loadSql: QueryWithParams, invalidateKeyQueries: Array<QueryWithParams> },
740+
query: BaseQuery,
741+
) {
742+
const preAggSql = preAggregationsDescription
743+
.loadSql[0]
744+
// Without `ON COMMIT DROP` temp tables are session-bound, and can live across multiple transactions
745+
.replace(/CREATE TABLE (.+) AS SELECT/, 'CREATE TEMP TABLE $1 ON COMMIT DROP AS SELECT');
746+
const preAggParams = preAggregationsDescription.loadSql[1];
747+
748+
const queries = [
749+
...preAggregationsDescription.invalidateKeyQueries,
750+
[preAggSql, preAggParams],
751+
query.buildSqlAndParams(),
752+
];
753+
754+
return dbRunner.testQueries(queries);
755+
}
756+
735757
it('simple join total', async () => runQueryTest({
736758
measures: [
737759
'visitors.visitor_revenue',
@@ -1783,6 +1805,40 @@ SELECT 1 AS revenue, cast('2024-01-01' AS timestamp) as time UNION ALL
17831805
});
17841806
});
17851807

1808+
/// Test that query with segment member expression, that references dimension, that is covered by pre-agg
1809+
/// would _not_ trigger stuff like `path.split is not a function` due to unexpected member expression
1810+
it('pre-aggregation with segment member expression', async () => {
1811+
await compiler.compile();
1812+
1813+
const query = new PostgresQuery({ joinGraph, cubeEvaluator, compiler }, {
1814+
segments: [
1815+
{
1816+
// eslint-disable-next-line no-new-func
1817+
expression: new Function(
1818+
'visitor_checkins',
1819+
// eslint-disable-next-line no-template-curly-in-string
1820+
'return `${visitor_checkins.source} IS NOT NULL`'
1821+
),
1822+
expressionName: 'source_is_some',
1823+
// eslint-disable-next-line no-template-curly-in-string
1824+
definition: '${visitor_checkins.source} IS NOT NULL',
1825+
cubeName: 'visitor_checkins',
1826+
},
1827+
],
1828+
timezone: 'America/Los_Angeles',
1829+
order: [],
1830+
preAggregationsSchema: ''
1831+
});
1832+
1833+
const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription()[0];
1834+
1835+
const res = await testWithPreAgg(preAggregationsDescription, query);
1836+
expect(res).toEqual(
1837+
// Empty result set, only segments in query
1838+
[{}]
1839+
);
1840+
});
1841+
17861842
it('join rollup pre-aggregation', async () => {
17871843
await compiler.compile();
17881844

@@ -1817,21 +1873,17 @@ SELECT 1 AS revenue, cast('2024-01-01' AS timestamp) as time UNION ALL
18171873
const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription()[0];
18181874
console.log(preAggregationsDescription);
18191875

1820-
return dbRunner.testQueries(preAggregationsDescription.invalidateKeyQueries.concat([
1821-
[preAggregationsDescription.loadSql[0].replace('CREATE TABLE', 'CREATE TEMP TABLE'), preAggregationsDescription.loadSql[1]],
1822-
query.buildSqlAndParams()
1823-
])).then(res => {
1824-
console.log(JSON.stringify(res));
1825-
expect(res).toEqual(
1826-
[
1827-
{
1828-
vc__source: 'google',
1829-
visitors__created_at_day: '2017-01-02T00:00:00.000Z',
1830-
visitors__per_visitor_revenue: '100'
1831-
}
1832-
]
1833-
);
1834-
});
1876+
const res = await testWithPreAgg(preAggregationsDescription, query);
1877+
console.log(JSON.stringify(res));
1878+
expect(res).toEqual(
1879+
[
1880+
{
1881+
vc__source: 'google',
1882+
visitors__created_at_day: '2017-01-02T00:00:00.000Z',
1883+
visitors__per_visitor_revenue: '100'
1884+
}
1885+
]
1886+
);
18351887
});
18361888

18371889
it('join rollup total pre-aggregation', async () => {
@@ -1862,22 +1914,15 @@ SELECT 1 AS revenue, cast('2024-01-01' AS timestamp) as time UNION ALL
18621914
const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription()[0];
18631915
console.log(preAggregationsDescription);
18641916

1865-
return dbRunner.testQueries(preAggregationsDescription.invalidateKeyQueries.concat([
1866-
[
1867-
preAggregationsDescription.loadSql[0].replace('CREATE TABLE', 'CREATE TEMP TABLE'),
1868-
preAggregationsDescription.loadSql[1]
1869-
],
1870-
query.buildSqlAndParams()
1871-
])).then(res => {
1872-
console.log(JSON.stringify(res));
1873-
expect(res).toEqual(
1874-
[{
1875-
vc__source: 'google',
1876-
visitors__created_at_day: '2017-01-02T00:00:00.000Z',
1877-
visitors__visitor_revenue: '100'
1878-
}]
1879-
);
1880-
});
1917+
const res = await testWithPreAgg(preAggregationsDescription, query);
1918+
console.log(JSON.stringify(res));
1919+
expect(res).toEqual(
1920+
[{
1921+
vc__source: 'google',
1922+
visitors__created_at_day: '2017-01-02T00:00:00.000Z',
1923+
visitors__visitor_revenue: '100'
1924+
}]
1925+
);
18811926
});
18821927

18831928
it('security context', async () => {

0 commit comments

Comments
 (0)