Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(schema-compiler): Fix queries with time dimensions without granularities don't hit pre-aggregations with allow_non_strict_date_range_match=true #9102

Merged
merged 11 commits into from
Feb 6, 2025
8 changes: 4 additions & 4 deletions packages/cubejs-backend-shared/src/time.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export function subtractInterval(date: moment.Moment, interval: ParsedInterval):
/**
* Returns the closest date prior to date parameter aligned with the origin point
*/
function alignToOrigin(startDate: moment.Moment, interval: ParsedInterval, origin: moment.Moment): moment.Moment {
export const alignToOrigin = (startDate: moment.Moment, interval: ParsedInterval, origin: moment.Moment): moment.Moment => {
let alignedDate = startDate.clone();
let intervalOp;
let isIntervalNegative = false;
Expand Down Expand Up @@ -111,17 +111,17 @@ function alignToOrigin(startDate: moment.Moment, interval: ParsedInterval, origi
}

return alignedDate;
}
};

function parsedSqlIntervalToDuration(parsedInterval: ParsedInterval): moment.Duration {
export const parsedSqlIntervalToDuration = (parsedInterval: ParsedInterval): moment.Duration => {
const duration = moment.duration();

Object.entries(parsedInterval).forEach(([key, value]) => {
duration.add(value, key as unitOfTime.DurationConstructor);
});

return duration;
}
};

function checkSeriesForDateRange(intervalStr: string, [startStr, endStr]: QueryDateRange): void {
const intervalParsed = parseSqlInterval(intervalStr);
Expand Down
46 changes: 44 additions & 2 deletions packages/cubejs-schema-compiler/src/adapter/BaseQuery.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { BaseTimeDimension } from './BaseTimeDimension';
import { ParamAllocator } from './ParamAllocator';
import { PreAggregations } from './PreAggregations';
import { SqlParser } from '../parser/SqlParser';
import { Granularity } from './Granularity';

const DEFAULT_PREAGGREGATIONS_SCHEMA = 'stb_pre_aggregations';

Expand Down Expand Up @@ -1379,7 +1380,47 @@ export class BaseQuery {
}

granularityHierarchies() {
return R.fromPairs(Object.keys(standardGranularitiesParents).map(k => [k, this.granularityParentHierarchy(k)]));
return this.cacheValue(
// If time dimension custom granularity in data model is defined without
// timezone information they are treated in query timezone.
// Because of that it's not possible to correctly precalculate
// granularities hierarchies on startup as they are specific for each timezone.
['granularityHierarchies', this.timezone],
() => R.reduce(
(hierarchies, cube) => R.reduce(
(acc, [tdName, td]) => {
const dimensionKey = `${cube}.${tdName}`;

// constructing standard granularities for time dimension
const standardEntries = R.fromPairs(
R.keys(standardGranularitiesParents).map(gr => [
`${dimensionKey}.${gr}`,
standardGranularitiesParents[gr],
]),
);

// If we have custom granularities in time dimension
const customEntries = td.granularities
? R.fromPairs(
R.keys(td.granularities).map(granularityName => {
const grObj = new Granularity(this, { dimension: dimensionKey, granularity: granularityName });
return [
`${dimensionKey}.${granularityName}`,
[granularityName, ...standardGranularitiesParents[grObj.minGranularity()]],
];
}),
)
: {};

return { ...acc, ...standardEntries, ...customEntries };
},
hierarchies,
R.toPairs(this.cubeEvaluator.timeDimensionsForCube(cube)),
),
{},
R.keys(this.cubeEvaluator.evaluatedCubes),
),
);
}

granularityParentHierarchy(granularity) {
Expand Down Expand Up @@ -1646,7 +1687,8 @@ export class BaseQuery {

/**
*
* @param {{sql: string, on: {cubeName: string, expression: Function}, joinType: 'LEFT' | 'INNER', alias: string}} customJoin
* @param {{sql: string, on: {cubeName: string, expression: Function}, joinType: 'LEFT' | 'INNER', alias: string}}
* customJoin
* @returns {JoinItem}
*/
customSubQueryJoin(customJoin) {
Expand Down
27 changes: 17 additions & 10 deletions packages/cubejs-schema-compiler/src/adapter/BaseTimeDimension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export class BaseTimeDimension extends BaseFilter {
}

// TODO: find and fix all hidden references to granularity to rely on granularityObj instead?
public get granularity(): string | undefined {
public get granularity(): string | null | undefined {
return this.granularityObj?.granularity;
}

Expand Down Expand Up @@ -217,7 +217,7 @@ export class BaseTimeDimension extends BaseFilter {
return this.query.dateTimeCast(this.query.paramAllocator.allocateParam(this.dateRange ? this.dateToFormatted() : BUILD_RANGE_END_LOCAL));
}

public dateRangeGranularity() {
public dateRangeGranularity(): string | null {
if (!this.dateRange) {
return null;
}
Expand All @@ -229,9 +229,9 @@ export class BaseTimeDimension extends BaseFilter {
);
}

protected rollupGranularityValue: any | null = null;
protected rollupGranularityValue: string | null = null;

public rollupGranularity() {
public rollupGranularity(): string | null {
if (!this.rollupGranularityValue) {
this.rollupGranularityValue =
this.query.cacheValue(
Expand All @@ -241,7 +241,18 @@ export class BaseTimeDimension extends BaseFilter {
return this.dateRangeGranularity();
}

return this.query.minGranularity(this.granularityObj.minGranularity(), this.dateRangeGranularity());
if (!this.dateRange) {
return this.granularityObj.minGranularity();
}

// If we have granularity and date range, we need to check
// that the interval and the granularity offset are stacked/fits with date range
if (this.granularityObj.isPredefined() ||
!this.granularityObj.isAlignedWithDateRange([this.dateFromFormatted(), this.dateToFormatted()])) {
return this.query.minGranularity(this.granularityObj.minGranularity(), this.dateRangeGranularity());
}

return this.granularityObj.granularity;
}
);
}
Expand All @@ -262,11 +273,7 @@ export class BaseTimeDimension extends BaseFilter {
}

public resolvedGranularity() {
return this.granularityObj?.resolvedGranularity();
}

public isPredefinedGranularity(): boolean {
return this.granularityObj?.isPredefined() || false;
return this.granularityObj ? this.granularityObj.resolvedGranularity() : this.dateRangeGranularity();
}

public wildcardRange() {
Expand Down
31 changes: 29 additions & 2 deletions packages/cubejs-schema-compiler/src/adapter/Granularity.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
import moment from 'moment-timezone';
import {
addInterval,
isPredefinedGranularity, parseSqlInterval,
QueryDateRange, timeSeries,
alignToOrigin,
isPredefinedGranularity,
parsedSqlIntervalToDuration,
parseSqlInterval,
QueryDateRange,
timeSeries,
timeSeriesFromCustomInterval,
TimeSeriesOptions
} from '@cubejs-backend/shared';
Expand Down Expand Up @@ -159,6 +163,29 @@ export class Granularity {
}
}

public isAlignedWithDateRange([startStr, endStr]: QueryDateRange): boolean {
const intervalParsed = parseSqlInterval(this.granularityInterval);
const grIntervalDuration = parsedSqlIntervalToDuration(intervalParsed);
const msFrom = moment.tz(startStr, this.queryTimezone);
const msTo = moment.tz(endStr, this.queryTimezone).add(1, 'ms');

// We can't simply compare interval milliseconds because of DSTs.
const testDate = msFrom.clone();
while (testDate.isBefore(msTo)) {
testDate.add(grIntervalDuration);
}
if (!testDate.isSame(msTo)) {
return false;
}

const closestDate = alignToOrigin(msFrom, intervalParsed, this.origin);
if (!msFrom.isSame(closestDate)) {
return false;
}

return true;
}

public isNaturalAligned(): boolean {
const intParsed = this.granularityInterval.split(' ');

Expand Down
68 changes: 18 additions & 50 deletions packages/cubejs-schema-compiler/src/adapter/PreAggregations.js
Original file line number Diff line number Diff line change
Expand Up @@ -439,18 +439,14 @@ export class PreAggregations {
static sortTimeDimensionsWithRollupGranularity(timeDimensions) {
return timeDimensions && R.sortBy(
R.prop(0),
timeDimensions.map(d => (d.isPredefinedGranularity() ?
[d.expressionPath(), d.rollupGranularity(), null] :
// For custom granularities we need to add its name to the list (for exact matches)
[d.expressionPath(), d.rollupGranularity(), d.granularity]
))
timeDimensions.map(d => [d.expressionPath(), d.rollupGranularity()])
) || [];
}

static timeDimensionsAsIs(timeDimensions) {
return timeDimensions && R.sortBy(
R.prop(0),
timeDimensions.map(d => [d.expressionPath(), d.granularity]),
timeDimensions.map(d => [d.expressionPath(), d.resolvedGranularity()]),
) || [];
}

Expand Down Expand Up @@ -485,26 +481,6 @@ export class PreAggregations {
);
}

canUsePreAggregationAndCheckIfRefValid(query) {
const transformedQuery = PreAggregations.transformQueryToCanUseForm(query);
return (refs) => PreAggregations.canUsePreAggregationForTransformedQueryFn(
transformedQuery, refs
);
}

checkAutoRollupPreAggregationValid(refs) {
try {
this.autoRollupPreAggregationQuery(null, refs); // TODO null
return true;
} catch (e) {
if (e instanceof UserError || e.toString().indexOf('ReferenceError') !== -1) {
return false;
} else {
throw e;
}
}
}

/**
* Returns function to determine whether pre-aggregation can be used or not
* for specified query, or its value for `refs` if specified.
Expand Down Expand Up @@ -560,9 +536,7 @@ export class PreAggregations {
backAlias(references.sortedTimeDimensions || sortTimeDimensions(references.timeDimensions));
const qryTimeDimensions = references.allowNonStrictDateRangeMatch
? transformedQuery.timeDimensions
: transformedQuery.sortedTimeDimensions.map(t => t.slice(0, 2));
// slice above is used to exclude possible custom granularity returned from sortTimeDimensionsWithRollupGranularity()

: transformedQuery.sortedTimeDimensions;
const backAliasMeasures = backAlias(references.measures);
const backAliasSortedDimensions = backAlias(references.sortedDimensions || references.dimensions);
const backAliasDimensions = backAlias(references.dimensions);
Expand All @@ -589,12 +563,13 @@ export class PreAggregations {
};

/**
* Wrap granularity string into an array.
* @param {string} granularity
* Expand granularity into array of granularity hierarchy.
* @param {string} dimension Dimension in the form of `cube.timeDimension`
* @param {string} granularity Granularity
* @returns {Array<string>}
*/
const expandGranularity = (granularity) => (
transformedQuery.granularityHierarchies[granularity] ||
const expandGranularity = (dimension, granularity) => (
transformedQuery.granularityHierarchies[`${dimension}.${granularity}`] ||
[granularity]
);

Expand All @@ -611,15 +586,15 @@ export class PreAggregations {
references.sortedTimeDimensions ||
sortTimeDimensions(references.timeDimensions);

return expandGranularity(transformedQuery.windowGranularity)
.map(
windowGranularity => R.all(
td => td[1] === windowGranularity,
sortedTimeDimensions,
return sortedTimeDimensions
.map(td => expandGranularity(td[0], transformedQuery.windowGranularity))
.some(
expandedGranularities => expandedGranularities.some(
windowGranularity => sortedTimeDimensions.every(
td => td[1] === windowGranularity
)
)
)
.filter(x => !!x)
.length > 0;
);
};

/**
Expand All @@ -628,16 +603,9 @@ export class PreAggregations {
* @returns {Array<Array<string>>}
*/
const expandTimeDimension = (timeDimension) => {
const [dimension, granularity, customGranularity] = timeDimension;
const res = expandGranularity(granularity)
const [dimension, resolvedGranularity] = timeDimension;
return expandGranularity(dimension, resolvedGranularity)
.map((newGranularity) => [dimension, newGranularity]);

if (customGranularity) {
// For custom granularities we add it upfront to the list (for exact matches)
res.unshift([dimension, customGranularity]);
}

return res;
};

/**
Expand Down
13 changes: 10 additions & 3 deletions packages/cubejs-schema-compiler/src/compiler/CubeEvaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -448,8 +448,8 @@ export class CubeEvaluator extends CubeSymbols {

public timeDimensionPathsForCube(cube: any) {
return R.compose(
R.map(nameToDefinition => `${cube}.${nameToDefinition[0]}`),
R.toPairs,
R.map(dimName => `${cube}.${dimName}`),
R.keys,
// @ts-ignore
R.filter((d: any) => d.type === 'time')
// @ts-ignore
Expand All @@ -460,12 +460,19 @@ export class CubeEvaluator extends CubeSymbols {
return this.cubeFromPath(cube).measures || {};
}

public timeDimensionsForCube(cube) {
return R.filter(
(d: any) => d.type === 'time',
this.cubeFromPath(cube).dimensions || {}
);
}

public preAggregationsForCube(path: string) {
return this.cubeFromPath(path).preAggregations || {};
}

/**
* Returns pre-aggregations filtered by the spcified selector.
* Returns pre-aggregations filtered by the specified selector.
* @param {{
* scheduled: boolean,
* dataSource: Array<string>,
Expand Down
Loading
Loading