Skip to content

Commit 589d20f

Browse files
waralexromwaralexrom
authored
fix(tesseract): Measure with string type ends up with SUM aggregation (#10516)
1 parent 244c04b commit 589d20f

File tree

5 files changed

+294
-0
lines changed

5 files changed

+294
-0
lines changed

‎packages/cubejs-schema-compiler/test/integration/postgres/pre-aggregations-string-measure.test.ts‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
import { getEnv } from '@cubejs-backend/shared';
2+
import { PostgresQuery } from '../../../src/adapter/PostgresQuery';
3+
import { prepareJsCompiler } from '../../unit/PrepareCompiler';
4+
import { dbRunner } from './PostgresDBRunner';
5+
6+
describe('PreAggregations string type measure', () => {
7+
jest.setTimeout(200000);
8+
9+
const { compiler, joinGraph, cubeEvaluator } = prepareJsCompiler(`
10+
cube(\`visitors_str\`, {
11+
sql: \`select * from visitors\`,
12+
sqlAlias: 'vstr',
13+
14+
measures: {
15+
count: {
16+
type: 'count'
17+
},
18+
sources_list: {
19+
sql: \`STRING_AGG(\${CUBE}.source, ', ' ORDER BY \${CUBE}.source)\`,
20+
type: 'string'
21+
}
22+
},
23+
24+
dimensions: {
25+
id: {
26+
type: 'number',
27+
sql: 'id',
28+
primaryKey: true
29+
},
30+
status: {
31+
type: 'number',
32+
sql: 'status'
33+
},
34+
createdAt: {
35+
type: 'time',
36+
sql: 'created_at',
37+
}
38+
},
39+
40+
preAggregations: {
41+
stringMeasureRollup: {
42+
type: 'rollup',
43+
measures: [CUBE.sources_list, CUBE.count],
44+
dimensions: [CUBE.status],
45+
}
46+
}
47+
})
48+
`);
49+
50+
it('string type measure without pre-aggregation', async () => {
51+
await compiler.compile();
52+
53+
const query = new PostgresQuery(
54+
{ joinGraph, cubeEvaluator, compiler },
55+
{
56+
measures: [
57+
'visitors_str.sources_list',
58+
'visitors_str.count',
59+
],
60+
dimensions: ['visitors_str.status'],
61+
timeDimensions: [{
62+
dimension: 'visitors_str.createdAt',
63+
granularity: 'day',
64+
dateRange: ['2017-01-01', '2017-01-10'],
65+
}],
66+
timezone: 'America/Los_Angeles',
67+
order: [{ id: 'visitors_str.status' }],
68+
}
69+
);
70+
71+
const sqlAndParams = query.buildSqlAndParams();
72+
// Should not use pre-aggregation since timeDimension is not in the rollup
73+
expect(sqlAndParams[0]).not.toContain('string_measure_rollup');
74+
75+
return dbRunner.testQuery(sqlAndParams).then(res => {
76+
expect(res).toEqual([
77+
{
78+
vstr__status: 1,
79+
vstr__created_at_day: '2017-01-02T00:00:00.000Z',
80+
vstr__sources_list: 'some',
81+
vstr__count: '1',
82+
},
83+
{
84+
vstr__status: 1,
85+
vstr__created_at_day: '2017-01-04T00:00:00.000Z',
86+
vstr__sources_list: 'some',
87+
vstr__count: '1',
88+
},
89+
{
90+
vstr__status: 2,
91+
vstr__created_at_day: '2017-01-05T00:00:00.000Z',
92+
vstr__sources_list: 'google',
93+
vstr__count: '1',
94+
},
95+
{
96+
vstr__status: 2,
97+
vstr__created_at_day: '2017-01-06T00:00:00.000Z',
98+
vstr__sources_list: null,
99+
vstr__count: '2',
100+
},
101+
]);
102+
});
103+
});
104+
105+
if (getEnv('nativeSqlPlanner') && getEnv('nativeSqlPlannerPreAggregations')) {
106+
it('string type measure with pre-aggregation', async () => {
107+
await compiler.compile();
108+
109+
const query = new PostgresQuery(
110+
{ joinGraph, cubeEvaluator, compiler },
111+
{
112+
measures: [
113+
'visitors_str.sources_list',
114+
'visitors_str.count',
115+
],
116+
dimensions: ['visitors_str.status'],
117+
timezone: 'America/Los_Angeles',
118+
preAggregationsSchema: '',
119+
}
120+
);
121+
122+
const preAggregationsDescription: any = query.preAggregations?.preAggregationsDescription();
123+
const sqlAndParams = query.buildSqlAndParams();
124+
expect(preAggregationsDescription[0].tableName).toEqual('vstr_string_measure_rollup');
125+
expect(sqlAndParams[0]).toContain('vstr_string_measure_rollup');
126+
127+
return dbRunner.evaluateQueryWithPreAggregations(query).then(res => {
128+
expect(res).toEqual([
129+
{
130+
vstr__status: 1,
131+
vstr__sources_list: 'some, some',
132+
vstr__count: '2',
133+
},
134+
{
135+
vstr__status: 2,
136+
vstr__sources_list: 'google',
137+
vstr__count: '4',
138+
},
139+
]);
140+
});
141+
});
142+
} else {
143+
it.skip('string type measure with pre-aggregation', async () => {
144+
// This fixed only in Tesseract
145+
});
146+
}
147+
});

‎rust/cubesqlplanner/cubesqlplanner/src/planner/sql_evaluator/symbols/measure_kinds/mod.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,10 @@ impl MeasureKind {
230230
AggregationType::Max => AggregateWrap::Function("max"),
231231
_ => AggregateWrap::Function("sum"),
232232
},
233+
Self::Calculated(c) => match c.calc_type() {
234+
CalculatedMeasureType::Number => AggregateWrap::Function("sum"),
235+
_ => AggregateWrap::Function("max"),
236+
},
233237
_ => AggregateWrap::Function("sum"),
234238
}
235239
}

‎rust/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/string_measures.yaml‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
cubes:
2+
- name: visitors_str
3+
sql: "SELECT * FROM visitors"
4+
5+
dimensions:
6+
- name: id
7+
type: number
8+
sql: id
9+
primary_key: true
10+
- name: status
11+
type: number
12+
sql: status
13+
- name: created_at
14+
type: time
15+
sql: created_at
16+
measures:
17+
- name: count
18+
type: count
19+
sql: "COUNT(*)"
20+
- name: sources_list
21+
type: string
22+
sql: "STRING_AGG({CUBE}.source, ', ' ORDER BY {CUBE}.source)"
23+
pre_aggregations:
24+
- name: string_measure_rollup
25+
type: rollup
26+
measures:
27+
- sources_list
28+
- count
29+
dimensions:
30+
- status

‎rust/cubesqlplanner/cubesqlplanner/src/tests/mod.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,5 +8,6 @@ mod measure_symbol;
88
mod member_expressions_on_views;
99
mod pre_aggregation_sql_generation;
1010
mod rolling_window_sql_generation;
11+
mod string_measures;
1112
mod subquery_dimensions;
1213
mod utils;

‎rust/cubesqlplanner/cubesqlplanner/src/tests/string_measures.rs‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
use crate::test_fixtures::cube_bridge::MockSchema;
2+
use crate::test_fixtures::test_utils::TestContext;
3+
use indoc::indoc;
4+
5+
#[test]
6+
fn test_string_measure_without_pre_aggregation() {
7+
let schema = MockSchema::from_yaml_file("common/string_measures.yaml");
8+
let test_context = TestContext::new(schema).unwrap();
9+
10+
let query_yaml = indoc! {"
11+
measures:
12+
- visitors_str.sources_list
13+
- visitors_str.count
14+
dimensions:
15+
- visitors_str.status
16+
time_dimensions:
17+
- dimension: visitors_str.created_at
18+
granularity: day
19+
dateRange:
20+
- \"2017-01-01\"
21+
- \"2017-01-10\"
22+
order:
23+
- id: visitors_str.status
24+
"};
25+
26+
let (sql, pre_aggrs) = test_context
27+
.build_sql_with_used_pre_aggregations(query_yaml)
28+
.expect("Should generate SQL");
29+
30+
assert!(
31+
pre_aggrs.is_empty(),
32+
"Should not use pre-aggregation since timeDimension is not in the rollup"
33+
);
34+
35+
assert!(
36+
!sql.contains("string_measure_rollup"),
37+
"SQL should not contain pre-aggregation table reference, got: {sql}"
38+
);
39+
40+
assert!(
41+
sql.contains("STRING_AGG"),
42+
"SQL should contain STRING_AGG for string measure, got: {sql}"
43+
);
44+
45+
assert!(
46+
!sql.contains("sum(STRING_AGG"),
47+
"SQL should not wrap STRING_AGG in sum, got: {sql}"
48+
);
49+
}
50+
51+
#[test]
52+
fn test_string_measure_with_pre_aggregation() {
53+
let schema = MockSchema::from_yaml_file("common/string_measures.yaml");
54+
let test_context = TestContext::new(schema).unwrap();
55+
56+
let query_yaml = indoc! {"
57+
measures:
58+
- visitors_str.sources_list
59+
- visitors_str.count
60+
dimensions:
61+
- visitors_str.status
62+
"};
63+
64+
let (sql, pre_aggrs) = test_context
65+
.build_sql_with_used_pre_aggregations(query_yaml)
66+
.expect("Should generate SQL");
67+
68+
assert_eq!(pre_aggrs.len(), 1, "Should use one pre-aggregation");
69+
assert_eq!(pre_aggrs[0].name(), "string_measure_rollup");
70+
71+
assert!(
72+
sql.contains("string_measure_rollup"),
73+
"SQL should contain pre-aggregation table reference, got: {sql}"
74+
);
75+
76+
assert!(
77+
sql.contains("visitors_str__sources_list"),
78+
"SQL should reference sources_list column, got: {sql}"
79+
);
80+
81+
assert!(
82+
!sql.contains("sum(\"visitors_str__sources_list\")"),
83+
"SQL should not wrap string measure in sum, got: {sql}"
84+
);
85+
}
86+
87+
#[test]
88+
fn test_string_measure_no_match_without_dimension() {
89+
let schema = MockSchema::from_yaml_file("common/string_measures.yaml");
90+
let test_context = TestContext::new(schema).unwrap();
91+
92+
let query_yaml = indoc! {"
93+
measures:
94+
- visitors_str.sources_list
95+
- visitors_str.count
96+
"};
97+
98+
let (sql, pre_aggrs) = test_context
99+
.build_sql_with_used_pre_aggregations(query_yaml)
100+
.expect("Should generate SQL");
101+
102+
assert!(
103+
pre_aggrs.is_empty(),
104+
"Should not match pre-aggregation when status dimension is missing from query, got: {} pre-aggregations",
105+
pre_aggrs.len()
106+
);
107+
108+
assert!(
109+
!sql.contains("string_measure_rollup"),
110+
"SQL should not contain pre-aggregation table reference, got: {sql}"
111+
);
112+
}

0 commit comments

Comments
 (0)