Skip to content

Commit cb7b643

Browse files
mcheshkovmcheshkov
authored
fix(schema-compiler): Use segments as-is in rollupPreAggregation (#9391)
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 a48963d commit cb7b643

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';
@@ -825,6 +826,27 @@ SELECT 1 AS revenue, cast('2024-01-01' AS timestamp) as time UNION ALL
825826
);
826827
}
827828

829+
type QueryWithParams = [string, Array<unknown>];
830+
831+
async function testWithPreAgg(
832+
preAggregationsDescription: { loadSql: QueryWithParams, invalidateKeyQueries: Array<QueryWithParams> },
833+
query: BaseQuery,
834+
) {
835+
const preAggSql = preAggregationsDescription
836+
.loadSql[0]
837+
// Without `ON COMMIT DROP` temp tables are session-bound, and can live across multiple transactions
838+
.replace(/CREATE TABLE (.+) AS SELECT/, 'CREATE TEMP TABLE $1 ON COMMIT DROP AS SELECT');
839+
const preAggParams = preAggregationsDescription.loadSql[1];
840+
841+
const queries = [
842+
...preAggregationsDescription.invalidateKeyQueries,
843+
[preAggSql, preAggParams],
844+
query.buildSqlAndParams(),
845+
];
846+
847+
return dbRunner.testQueries(queries);
848+
}
849+
828850
it('simple join total', async () => runQueryTest({
829851
measures: [
830852
'visitors.visitor_revenue',
@@ -1978,6 +2000,40 @@ SELECT 1 AS revenue, cast('2024-01-01' AS timestamp) as time UNION ALL
19782000
});
19792001
});
19802002

2003+
/// Test that query with segment member expression, that references dimension, that is covered by pre-agg
2004+
/// would _not_ trigger stuff like `path.split is not a function` due to unexpected member expression
2005+
it('pre-aggregation with segment member expression', async () => {
2006+
await compiler.compile();
2007+
2008+
const query = new PostgresQuery({ joinGraph, cubeEvaluator, compiler }, {
2009+
segments: [
2010+
{
2011+
// eslint-disable-next-line no-new-func
2012+
expression: new Function(
2013+
'visitor_checkins',
2014+
// eslint-disable-next-line no-template-curly-in-string
2015+
'return `${visitor_checkins.source} IS NOT NULL`'
2016+
),
2017+
expressionName: 'source_is_some',
2018+
// eslint-disable-next-line no-template-curly-in-string
2019+
definition: '${visitor_checkins.source} IS NOT NULL',
2020+
cubeName: 'visitor_checkins',
2021+
},
2022+
],
2023+
timezone: 'America/Los_Angeles',
2024+
order: [],
2025+
preAggregationsSchema: ''
2026+
});
2027+
2028+
const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription()[0];
2029+
2030+
const res = await testWithPreAgg(preAggregationsDescription, query);
2031+
expect(res).toEqual(
2032+
// Empty result set, only segments in query
2033+
[{}]
2034+
);
2035+
});
2036+
19812037
it('join rollup pre-aggregation', async () => {
19822038
await compiler.compile();
19832039

@@ -2012,21 +2068,17 @@ SELECT 1 AS revenue, cast('2024-01-01' AS timestamp) as time UNION ALL
20122068
const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription()[0];
20132069
console.log(preAggregationsDescription);
20142070

2015-
return dbRunner.testQueries(preAggregationsDescription.invalidateKeyQueries.concat([
2016-
[preAggregationsDescription.loadSql[0].replace('CREATE TABLE', 'CREATE TEMP TABLE'), preAggregationsDescription.loadSql[1]],
2017-
query.buildSqlAndParams()
2018-
])).then(res => {
2019-
console.log(JSON.stringify(res));
2020-
expect(res).toEqual(
2021-
[
2022-
{
2023-
vc__source: 'google',
2024-
visitors__created_at_day: '2017-01-02T00:00:00.000Z',
2025-
visitors__per_visitor_revenue: '100'
2026-
}
2027-
]
2028-
);
2029-
});
2071+
const res = await testWithPreAgg(preAggregationsDescription, query);
2072+
console.log(JSON.stringify(res));
2073+
expect(res).toEqual(
2074+
[
2075+
{
2076+
vc__source: 'google',
2077+
visitors__created_at_day: '2017-01-02T00:00:00.000Z',
2078+
visitors__per_visitor_revenue: '100'
2079+
}
2080+
]
2081+
);
20302082
});
20312083

20322084
it('join rollup total pre-aggregation', async () => {
@@ -2057,22 +2109,15 @@ SELECT 1 AS revenue, cast('2024-01-01' AS timestamp) as time UNION ALL
20572109
const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription()[0];
20582110
console.log(preAggregationsDescription);
20592111

2060-
return dbRunner.testQueries(preAggregationsDescription.invalidateKeyQueries.concat([
2061-
[
2062-
preAggregationsDescription.loadSql[0].replace('CREATE TABLE', 'CREATE TEMP TABLE'),
2063-
preAggregationsDescription.loadSql[1]
2064-
],
2065-
query.buildSqlAndParams()
2066-
])).then(res => {
2067-
console.log(JSON.stringify(res));
2068-
expect(res).toEqual(
2069-
[{
2070-
vc__source: 'google',
2071-
visitors__created_at_day: '2017-01-02T00:00:00.000Z',
2072-
visitors__visitor_revenue: '100'
2073-
}]
2074-
);
2075-
});
2112+
const res = await testWithPreAgg(preAggregationsDescription, query);
2113+
console.log(JSON.stringify(res));
2114+
expect(res).toEqual(
2115+
[{
2116+
vc__source: 'google',
2117+
visitors__created_at_day: '2017-01-02T00:00:00.000Z',
2118+
visitors__visitor_revenue: '100'
2119+
}]
2120+
);
20762121
});
20772122

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

0 commit comments

Comments
 (0)