Skip to content

Commit 52a664e

Browse files
KSDaemonKSDaemon
authored
fix(schema-compiler): Fix sql generation for rolling_window queries with multiple time dimensions (#9124)
* just typos in comments and moving code around * fill missing dateRanges for TDs * fix multiple TDs request SQL generation * add tests * fill daterange for all time dimension items in query * remove this.fillMissingDateRangesForTimeDimensions from gateway * Add guard for time series without daterange * cargo fmt * fix test for cubesql fix * fix notes after review
1 parent fa318a1 commit 52a664e

File tree

5 files changed

+265
-32
lines changed

5 files changed

+265
-32
lines changed

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

Lines changed: 68 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -945,8 +945,8 @@ export class BaseQuery {
945945
).concat(multiStageMembers.map(m => `SELECT * FROM ${m.alias}`));
946946
}
947947

948-
// Move regular measures to multiplied ones if there're same
949-
// cubes to calculate. Most of the times it'll be much faster to
948+
// Move regular measures to multiplied ones if there are same
949+
// cubes to calculate. Most of the time it'll be much faster to
950950
// calculate as there will be only single scan per cube.
951951
if (
952952
regularMeasures.length &&
@@ -1464,18 +1464,62 @@ export class BaseQuery {
14641464

14651465
overTimeSeriesQuery(baseQueryFn, cumulativeMeasure, fromRollup) {
14661466
const dateJoinCondition = cumulativeMeasure.dateJoinCondition();
1467+
const uniqDateJoinCondition = R.uniqBy(djc => djc[0].dimension, dateJoinCondition);
14671468
const cumulativeMeasures = [cumulativeMeasure];
14681469
if (!this.timeDimensions.find(d => d.granularity)) {
1469-
const filters = this.segments.concat(this.filters).concat(this.dateFromStartToEndConditionSql(dateJoinCondition, fromRollup, false));
1470+
const filters = this.segments
1471+
.concat(this.filters)
1472+
.concat(this.dateFromStartToEndConditionSql(
1473+
// If the same time dimension is passed more than once, no need to build the same
1474+
// filter condition again and again. Different granularities don't play role here,
1475+
// as rollingWindow.granularity is used for filtering.
1476+
uniqDateJoinCondition,
1477+
fromRollup,
1478+
false
1479+
));
14701480
return baseQueryFn(cumulativeMeasures, filters, false);
14711481
}
1472-
const dateSeriesSql = this.timeDimensions.map(d => this.dateSeriesSql(d)).join(', ');
1473-
const filters = this.segments.concat(this.filters).concat(this.dateFromStartToEndConditionSql(dateJoinCondition, fromRollup, true));
1482+
1483+
if (this.timeDimensions.filter(d => !d.dateRange && d.granularity).length > 0) {
1484+
throw new UserError('Time series queries without dateRange aren\'t supported');
1485+
}
1486+
1487+
// We can't do meaningful query if few time dimensions with different ranges passed,
1488+
// it won't be possible to join them together without losing some rows.
1489+
const rangedTimeDimensions = this.timeDimensions.filter(d => d.dateRange && d.granularity);
1490+
const uniqTimeDimensionWithRanges = R.uniqBy(d => d.dateRange, rangedTimeDimensions);
1491+
if (uniqTimeDimensionWithRanges.length > 1) {
1492+
throw new Error('Can\'t build query for time dimensions with different date ranges');
1493+
}
1494+
1495+
// We need to generate time series table for the lowest granularity among all time dimensions
1496+
const [dateSeriesDimension, dateSeriesGranularity] = this.timeDimensions.filter(d => d.granularity)
1497+
.reduce(([prevDim, prevGran], d) => {
1498+
const mg = this.minGranularity(prevGran, d.resolvedGranularity());
1499+
if (mg === d.resolvedGranularity()) {
1500+
return [d, mg];
1501+
}
1502+
return [prevDim, mg];
1503+
}, [null, null]);
1504+
1505+
const dateSeriesSql = this.dateSeriesSql(dateSeriesDimension);
1506+
1507+
// If the same time dimension is passed more than once, no need to build the same
1508+
// filter condition again and again. Different granularities don't play role here,
1509+
// as rollingWindow.granularity is used for filtering.
1510+
const filters = this.segments
1511+
.concat(this.filters)
1512+
.concat(this.dateFromStartToEndConditionSql(
1513+
uniqDateJoinCondition,
1514+
fromRollup,
1515+
true
1516+
));
14741517
const baseQuery = this.groupedUngroupedSelect(
14751518
() => baseQueryFn(cumulativeMeasures, filters),
14761519
cumulativeMeasure.shouldUngroupForCumulative(),
14771520
!cumulativeMeasure.shouldUngroupForCumulative() && this.minGranularity(
1478-
cumulativeMeasure.windowGranularity(), this.timeDimensions.find(d => d.granularity).resolvedGranularity()
1521+
cumulativeMeasure.windowGranularity(),
1522+
dateSeriesGranularity
14791523
) || undefined
14801524
);
14811525
const baseQueryAlias = this.cubeAlias('base');
@@ -1495,10 +1539,27 @@ export class BaseQuery {
14951539
dateSeriesSql,
14961540
baseQuery,
14971541
dateJoinConditionSql,
1498-
baseQueryAlias
1542+
baseQueryAlias,
1543+
dateSeriesDimension.granularity,
14991544
);
15001545
}
15011546

1547+
overTimeSeriesSelect(cumulativeMeasures, dateSeriesSql, baseQuery, dateJoinConditionSql, baseQueryAlias, dateSeriesGranularity) {
1548+
const forSelect = this.overTimeSeriesForSelect(cumulativeMeasures, dateSeriesGranularity);
1549+
return `SELECT ${forSelect} FROM ${dateSeriesSql}` +
1550+
` LEFT JOIN (${baseQuery}) ${this.asSyntaxJoin} ${baseQueryAlias} ON ${dateJoinConditionSql}` +
1551+
this.groupByClause();
1552+
}
1553+
1554+
overTimeSeriesForSelect(cumulativeMeasures, dateSeriesGranularity) {
1555+
return this.dimensions
1556+
.map(s => s.cumulativeSelectColumns())
1557+
.concat(this.timeDimensions.map(d => d.dateSeriesSelectColumn(null, dateSeriesGranularity)))
1558+
.concat(cumulativeMeasures.map(s => s.cumulativeSelectColumns()))
1559+
.filter(c => !!c)
1560+
.join(', ');
1561+
}
1562+
15021563
dateFromStartToEndConditionSql(dateJoinCondition, fromRollup, isFromStartToEnd) {
15031564
return dateJoinCondition.map(
15041565
// TODO these weird conversions to be strict typed for big query.
@@ -1523,24 +1584,6 @@ export class BaseQuery {
15231584
);
15241585
}
15251586

1526-
overTimeSeriesSelect(cumulativeMeasures, dateSeriesSql, baseQuery, dateJoinConditionSql, baseQueryAlias) {
1527-
const forSelect = this.overTimeSeriesForSelect(cumulativeMeasures);
1528-
return `SELECT ${forSelect} FROM ${dateSeriesSql}` +
1529-
` LEFT JOIN (${baseQuery}) ${this.asSyntaxJoin} ${baseQueryAlias} ON ${dateJoinConditionSql}` +
1530-
this.groupByClause();
1531-
}
1532-
1533-
overTimeSeriesForSelect(cumulativeMeasures) {
1534-
return this.dimensions.map(s => s.cumulativeSelectColumns()).concat(this.dateSeriesSelect()).concat(
1535-
cumulativeMeasures.map(s => s.cumulativeSelectColumns()),
1536-
).filter(c => !!c)
1537-
.join(', ');
1538-
}
1539-
1540-
dateSeriesSelect() {
1541-
return this.timeDimensions.map(d => d.dateSeriesSelectColumn());
1542-
}
1543-
15441587
/**
15451588
* @param {import('./BaseTimeDimension').BaseTimeDimension} timeDimension
15461589
* @return {string}

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,10 +85,18 @@ export class BaseTimeDimension extends BaseFilter {
8585
return this.query.escapeColumnName(`${this.dimension}_series`);
8686
}
8787

88-
public dateSeriesSelectColumn(dateSeriesAliasName) {
88+
public dateSeriesSelectColumn(dateSeriesAliasName: string | null, dateSeriesGranularity?: string) {
8989
if (!this.granularityObj) {
9090
return null;
9191
}
92+
93+
// In case of query with more than one granularity, the time series table was generated
94+
// with the minimal granularity among all. If this is our granularity, we can save
95+
// some cpu cycles without 'date_from' truncation. But if this is not our granularity,
96+
// we need to truncate it to desired.
97+
if (dateSeriesGranularity && this.granularityObj?.granularity !== dateSeriesGranularity) {
98+
return `${this.query.dimensionTimeGroupedColumn(`${dateSeriesAliasName || this.dateSeriesAliasName()}.${this.query.escapeColumnName('date_from')}`, this.granularityObj)} ${this.aliasName()}`;
99+
}
92100
return `${dateSeriesAliasName || this.dateSeriesAliasName()}.${this.query.escapeColumnName('date_from')} ${this.aliasName()}`;
93101
}
94102

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

Lines changed: 178 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,19 @@ describe('SQL Generation', () => {
7979
offset: 'start'
8080
}
8181
},
82+
countRollingUnbounded: {
83+
type: 'count',
84+
rollingWindow: {
85+
trailing: 'unbounded'
86+
}
87+
},
88+
countRollingWeekToDate: {
89+
type: 'count',
90+
rollingWindow: {
91+
type: 'to_date',
92+
granularity: 'week'
93+
}
94+
},
8295
revenue_qtd: {
8396
type: 'sum',
8497
sql: 'amount',
@@ -220,7 +233,14 @@ describe('SQL Generation', () => {
220233
},
221234
created_at: {
222235
type: 'time',
223-
sql: 'created_at'
236+
sql: 'created_at',
237+
granularities: {
238+
three_days: {
239+
interval: '3 days',
240+
title: '3 days',
241+
origin: '2017-01-01'
242+
}
243+
}
224244
},
225245
updated_at: {
226246
type: 'time',
@@ -778,6 +798,163 @@ describe('SQL Generation', () => {
778798
}
779799
]));
780800

801+
it('rolling window with two time dimension granularities', async () => runQueryTest({
802+
measures: [
803+
'visitors.countRollingWeekToDate'
804+
],
805+
timeDimensions: [
806+
{
807+
dimension: 'visitors.created_at',
808+
granularity: 'three_days',
809+
dateRange: ['2017-01-01', '2017-01-10']
810+
},
811+
{
812+
dimension: 'visitors.created_at',
813+
granularity: 'day',
814+
dateRange: ['2017-01-01', '2017-01-10']
815+
}
816+
],
817+
order: [{
818+
id: 'visitors.created_at'
819+
}],
820+
timezone: 'America/Los_Angeles'
821+
}, [
822+
{
823+
visitors__count_rolling_week_to_date: null,
824+
visitors__created_at_day: '2017-01-01T00:00:00.000Z',
825+
visitors__created_at_three_days: '2017-01-01T00:00:00.000Z',
826+
},
827+
{
828+
visitors__count_rolling_week_to_date: '1',
829+
visitors__created_at_day: '2017-01-02T00:00:00.000Z',
830+
visitors__created_at_three_days: '2017-01-01T00:00:00.000Z',
831+
},
832+
{
833+
visitors__count_rolling_week_to_date: '1',
834+
visitors__created_at_day: '2017-01-03T00:00:00.000Z',
835+
visitors__created_at_three_days: '2017-01-01T00:00:00.000Z',
836+
},
837+
{
838+
visitors__count_rolling_week_to_date: '2',
839+
visitors__created_at_day: '2017-01-04T00:00:00.000Z',
840+
visitors__created_at_three_days: '2017-01-04T00:00:00.000Z',
841+
},
842+
{
843+
visitors__count_rolling_week_to_date: '3',
844+
visitors__created_at_day: '2017-01-05T00:00:00.000Z',
845+
visitors__created_at_three_days: '2017-01-04T00:00:00.000Z',
846+
},
847+
{
848+
visitors__count_rolling_week_to_date: '5',
849+
visitors__created_at_day: '2017-01-06T00:00:00.000Z',
850+
visitors__created_at_three_days: '2017-01-04T00:00:00.000Z',
851+
},
852+
{
853+
visitors__count_rolling_week_to_date: '5',
854+
visitors__created_at_day: '2017-01-07T00:00:00.000Z',
855+
visitors__created_at_three_days: '2017-01-07T00:00:00.000Z',
856+
},
857+
{
858+
visitors__count_rolling_week_to_date: '5',
859+
visitors__created_at_day: '2017-01-08T00:00:00.000Z',
860+
visitors__created_at_three_days: '2017-01-07T00:00:00.000Z',
861+
},
862+
{
863+
visitors__count_rolling_week_to_date: null,
864+
visitors__created_at_day: '2017-01-09T00:00:00.000Z',
865+
visitors__created_at_three_days: '2017-01-07T00:00:00.000Z',
866+
},
867+
{
868+
visitors__count_rolling_week_to_date: null,
869+
visitors__created_at_day: '2017-01-10T00:00:00.000Z',
870+
visitors__created_at_three_days: '2017-01-10T00:00:00.000Z',
871+
}
872+
]));
873+
874+
it('two rolling windows with two time dimension granularities', async () => runQueryTest({
875+
measures: [
876+
'visitors.countRollingUnbounded',
877+
'visitors.countRollingWeekToDate'
878+
],
879+
timeDimensions: [
880+
{
881+
dimension: 'visitors.created_at',
882+
granularity: 'three_days',
883+
dateRange: ['2017-01-01', '2017-01-10']
884+
},
885+
{
886+
dimension: 'visitors.created_at',
887+
granularity: 'day',
888+
dateRange: ['2017-01-01', '2017-01-10']
889+
}
890+
],
891+
order: [{
892+
id: 'visitors.created_at'
893+
}],
894+
timezone: 'America/Los_Angeles'
895+
}, [
896+
{
897+
visitors__count_rolling_unbounded: '1',
898+
visitors__count_rolling_week_to_date: null,
899+
visitors__created_at_day: '2017-01-01T00:00:00.000Z',
900+
visitors__created_at_three_days: '2017-01-01T00:00:00.000Z',
901+
},
902+
{
903+
visitors__count_rolling_unbounded: '2',
904+
visitors__count_rolling_week_to_date: '1',
905+
visitors__created_at_day: '2017-01-03T00:00:00.000Z',
906+
visitors__created_at_three_days: '2017-01-01T00:00:00.000Z',
907+
},
908+
{
909+
visitors__count_rolling_unbounded: '2',
910+
visitors__count_rolling_week_to_date: '1',
911+
visitors__created_at_day: '2017-01-02T00:00:00.000Z',
912+
visitors__created_at_three_days: '2017-01-01T00:00:00.000Z',
913+
},
914+
{
915+
visitors__count_rolling_unbounded: '3',
916+
visitors__count_rolling_week_to_date: '2',
917+
visitors__created_at_day: '2017-01-04T00:00:00.000Z',
918+
visitors__created_at_three_days: '2017-01-04T00:00:00.000Z',
919+
},
920+
{
921+
visitors__count_rolling_unbounded: '4',
922+
visitors__count_rolling_week_to_date: '3',
923+
visitors__created_at_day: '2017-01-05T00:00:00.000Z',
924+
visitors__created_at_three_days: '2017-01-04T00:00:00.000Z',
925+
},
926+
{
927+
visitors__count_rolling_unbounded: '6',
928+
visitors__count_rolling_week_to_date: '5',
929+
visitors__created_at_day: '2017-01-06T00:00:00.000Z',
930+
visitors__created_at_three_days: '2017-01-04T00:00:00.000Z',
931+
},
932+
{
933+
visitors__count_rolling_unbounded: '6',
934+
visitors__count_rolling_week_to_date: '5',
935+
visitors__created_at_day: '2017-01-08T00:00:00.000Z',
936+
visitors__created_at_three_days: '2017-01-07T00:00:00.000Z',
937+
},
938+
{
939+
visitors__count_rolling_unbounded: '6',
940+
visitors__count_rolling_week_to_date: '5',
941+
visitors__created_at_day: '2017-01-07T00:00:00.000Z',
942+
visitors__created_at_three_days: '2017-01-07T00:00:00.000Z',
943+
},
944+
{
945+
visitors__count_rolling_unbounded: '6',
946+
visitors__count_rolling_week_to_date: null,
947+
visitors__created_at_day: '2017-01-09T00:00:00.000Z',
948+
visitors__created_at_three_days: '2017-01-07T00:00:00.000Z',
949+
},
950+
{
951+
visitors__count_rolling_unbounded: '6',
952+
visitors__count_rolling_week_to_date: null,
953+
visitors__created_at_day: '2017-01-10T00:00:00.000Z',
954+
visitors__created_at_three_days: '2017-01-10T00:00:00.000Z',
955+
}
956+
]));
957+
781958
it('rolling month', async () => runQueryTest({
782959
measures: [
783960
'visitors.revenueRollingThreeDay'

‎rust/cubesql/cubesql/src/compile/mod.rs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2219,7 +2219,10 @@ limit
22192219
V1LoadRequestQueryTimeDimension {
22202220
dimension: "KibanaSampleDataEcommerce.order_date".to_string(),
22212221
granularity: Some("month".to_string()),
2222-
date_range: None,
2222+
date_range: Some(json!(vec![
2223+
"2023-07-08T00:00:00.000Z".to_string(),
2224+
"2023-10-07T23:59:59.999Z".to_string()
2225+
])),
22232226
}
22242227
]),
22252228
order: Some(vec![]),

‎rust/cubesql/cubesql/src/compile/rewrite/converter.rs‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1823,16 +1823,18 @@ impl LanguageToLogicalPlanConverter {
18231823
let values =
18241824
match_data_node!(node_by_id, params[2], FilterMemberValues);
18251825
if !is_in_or && op == "inDateRange" {
1826-
let existing_time_dimension =
1827-
query_time_dimensions.iter_mut().find_map(|td| {
1826+
let existing_time_dimensions: Vec<_> = query_time_dimensions
1827+
.iter_mut()
1828+
.filter_map(|td| {
18281829
if td.dimension == member && td.date_range.is_none() {
18291830
td.date_range = Some(json!(values));
18301831
Some(td)
18311832
} else {
18321833
None
18331834
}
1834-
});
1835-
if existing_time_dimension.is_none() {
1835+
})
1836+
.collect();
1837+
if existing_time_dimensions.len() == 0 {
18361838
let dimension = V1LoadRequestQueryTimeDimension {
18371839
dimension: member.to_string(),
18381840
granularity: None,

0 commit comments

Comments
 (0)