Skip to content

Commit be6bf96

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 7d890ff commit be6bf96

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';
@@ -739,6 +740,27 @@ SELECT 1 AS revenue, cast('2024-01-01' AS timestamp) as time UNION ALL
739740
);
740741
}
741742

743+
type QueryWithParams = [string, Array<unknown>];
744+
745+
async function testWithPreAgg(
746+
preAggregationsDescription: { loadSql: QueryWithParams, invalidateKeyQueries: Array<QueryWithParams> },
747+
query: BaseQuery,
748+
) {
749+
const preAggSql = preAggregationsDescription
750+
.loadSql[0]
751+
// Without `ON COMMIT DROP` temp tables are session-bound, and can live across multiple transactions
752+
.replace(/CREATE TABLE (.+) AS SELECT/, 'CREATE TEMP TABLE $1 ON COMMIT DROP AS SELECT');
753+
const preAggParams = preAggregationsDescription.loadSql[1];
754+
755+
const queries = [
756+
...preAggregationsDescription.invalidateKeyQueries,
757+
[preAggSql, preAggParams],
758+
query.buildSqlAndParams(),
759+
];
760+
761+
return dbRunner.testQueries(queries);
762+
}
763+
742764
it('simple join total', async () => runQueryTest({
743765
measures: [
744766
'visitors.visitor_revenue',
@@ -1892,6 +1914,40 @@ SELECT 1 AS revenue, cast('2024-01-01' AS timestamp) as time UNION ALL
18921914
});
18931915
});
18941916

1917+
/// Test that query with segment member expression, that references dimension, that is covered by pre-agg
1918+
/// would _not_ trigger stuff like `path.split is not a function` due to unexpected member expression
1919+
it('pre-aggregation with segment member expression', async () => {
1920+
await compiler.compile();
1921+
1922+
const query = new PostgresQuery({ joinGraph, cubeEvaluator, compiler }, {
1923+
segments: [
1924+
{
1925+
// eslint-disable-next-line no-new-func
1926+
expression: new Function(
1927+
'visitor_checkins',
1928+
// eslint-disable-next-line no-template-curly-in-string
1929+
'return `${visitor_checkins.source} IS NOT NULL`'
1930+
),
1931+
expressionName: 'source_is_some',
1932+
// eslint-disable-next-line no-template-curly-in-string
1933+
definition: '${visitor_checkins.source} IS NOT NULL',
1934+
cubeName: 'visitor_checkins',
1935+
},
1936+
],
1937+
timezone: 'America/Los_Angeles',
1938+
order: [],
1939+
preAggregationsSchema: ''
1940+
});
1941+
1942+
const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription()[0];
1943+
1944+
const res = await testWithPreAgg(preAggregationsDescription, query);
1945+
expect(res).toEqual(
1946+
// Empty result set, only segments in query
1947+
[{}]
1948+
);
1949+
});
1950+
18951951
it('join rollup pre-aggregation', async () => {
18961952
await compiler.compile();
18971953

@@ -1926,21 +1982,17 @@ SELECT 1 AS revenue, cast('2024-01-01' AS timestamp) as time UNION ALL
19261982
const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription()[0];
19271983
console.log(preAggregationsDescription);
19281984

1929-
return dbRunner.testQueries(preAggregationsDescription.invalidateKeyQueries.concat([
1930-
[preAggregationsDescription.loadSql[0].replace('CREATE TABLE', 'CREATE TEMP TABLE'), preAggregationsDescription.loadSql[1]],
1931-
query.buildSqlAndParams()
1932-
])).then(res => {
1933-
console.log(JSON.stringify(res));
1934-
expect(res).toEqual(
1935-
[
1936-
{
1937-
vc__source: 'google',
1938-
visitors__created_at_day: '2017-01-02T00:00:00.000Z',
1939-
visitors__per_visitor_revenue: '100'
1940-
}
1941-
]
1942-
);
1943-
});
1985+
const res = await testWithPreAgg(preAggregationsDescription, query);
1986+
console.log(JSON.stringify(res));
1987+
expect(res).toEqual(
1988+
[
1989+
{
1990+
vc__source: 'google',
1991+
visitors__created_at_day: '2017-01-02T00:00:00.000Z',
1992+
visitors__per_visitor_revenue: '100'
1993+
}
1994+
]
1995+
);
19441996
});
19451997

19461998
it('join rollup total pre-aggregation', async () => {
@@ -1971,22 +2023,15 @@ SELECT 1 AS revenue, cast('2024-01-01' AS timestamp) as time UNION ALL
19712023
const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription()[0];
19722024
console.log(preAggregationsDescription);
19732025

1974-
return dbRunner.testQueries(preAggregationsDescription.invalidateKeyQueries.concat([
1975-
[
1976-
preAggregationsDescription.loadSql[0].replace('CREATE TABLE', 'CREATE TEMP TABLE'),
1977-
preAggregationsDescription.loadSql[1]
1978-
],
1979-
query.buildSqlAndParams()
1980-
])).then(res => {
1981-
console.log(JSON.stringify(res));
1982-
expect(res).toEqual(
1983-
[{
1984-
vc__source: 'google',
1985-
visitors__created_at_day: '2017-01-02T00:00:00.000Z',
1986-
visitors__visitor_revenue: '100'
1987-
}]
1988-
);
1989-
});
2026+
const res = await testWithPreAgg(preAggregationsDescription, query);
2027+
console.log(JSON.stringify(res));
2028+
expect(res).toEqual(
2029+
[{
2030+
vc__source: 'google',
2031+
visitors__created_at_day: '2017-01-02T00:00:00.000Z',
2032+
visitors__visitor_revenue: '100'
2033+
}]
2034+
);
19902035
});
19912036

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

0 commit comments

Comments
 (0)