Skip to content

Commit

Permalink
fix(cubesql): SQL push down can be incorrectly routed to Cube Store p…
Browse files Browse the repository at this point in the history
…re-aggregations
  • Loading branch information
paveltiunov committed Feb 13, 2024
1 parent 768325c commit bb27698
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 5 deletions.
18 changes: 15 additions & 3 deletions packages/cubejs-api-gateway/src/gateway.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1172,7 +1172,16 @@ class ApiGateway {
return [queryType, normalizedQueries];
}

public async sql({ query, context, res, memberToAlias, exportAnnotatedSql, memberExpressions, expressionParams }: QueryRequest) {
public async sql({
query,
context,
res,
memberToAlias,
exportAnnotatedSql,
memberExpressions,
expressionParams,
disableExternalPreAggregations
}: QueryRequest) {
const requestStarted = new Date();

try {
Expand All @@ -1188,7 +1197,7 @@ class ApiGateway {

const sqlQueries = await Promise.all<any>(
normalizedQueries.map(async (normalizedQuery) => (await this.getCompilerApi(context)).getSql(
this.coerceForSqlQuery({ ...normalizedQuery, memberToAlias, expressionParams }, context),
this.coerceForSqlQuery({ ...normalizedQuery, memberToAlias, expressionParams, disableExternalPreAggregations }, context),
{
includeDebugInfo: getEnv('devMode') || context.signedWithPlaygroundAuthSecret,
exportAnnotatedSql,
Expand Down Expand Up @@ -1708,7 +1717,10 @@ class ApiGateway {
metaConfigResult = this.filterVisibleItemsInMeta(context, metaConfigResult);

const sqlQueries = await this
.getSqlQueriesInternal(context, normalizedQueries);
.getSqlQueriesInternal(
context,
normalizedQueries.map(q => ({ ...q, disableExternalPreAggregations: request.sqlQuery }))

Check warning on line 1722 in packages/cubejs-api-gateway/src/gateway.ts

View check run for this annotation

Codecov / codecov/patch

packages/cubejs-api-gateway/src/gateway.ts#L1722

Added line #L1722 was not covered by tests
);

let results;

Expand Down
1 change: 1 addition & 0 deletions packages/cubejs-api-gateway/src/sql-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ export class SQLServer {
expressionParams,
exportAnnotatedSql: true,
memberExpressions: true,
disableExternalPreAggregations: true,
queryType: 'multi',
context,
res: (message) => {
Expand Down
1 change: 1 addition & 0 deletions packages/cubejs-api-gateway/src/types/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ type QueryRequest = BaseRequest & {
expressionParams?: string[];
exportAnnotatedSql?: boolean;
memberExpressions?: boolean;
disableExternalPreAggregations?: boolean;
};

type SqlApiRequest = BaseRequest & {
Expand Down
8 changes: 6 additions & 2 deletions packages/cubejs-schema-compiler/src/adapter/BaseQuery.js
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ export class BaseQuery {
className: this.constructor.name,
externalClassName: this.options.externalQueryClass && this.options.externalQueryClass.name,
preAggregationQuery: this.options.preAggregationQuery,
disableExternalPreAggregations: this.options.disableExternalPreAggregations,
useOriginalSqlPreAggregationsInPreAggregation: this.options.useOriginalSqlPreAggregationsInPreAggregation,
cubeLatticeCache: this.options.cubeLatticeCache, // TODO too heavy for key
historyQueries: this.options.historyQueries, // TODO too heavy for key
Expand Down Expand Up @@ -467,6 +468,9 @@ export class BaseQuery {
if (!this.options.preAggregationQuery && !this.ungrouped) {
preAggForQuery =
this.preAggregations.findPreAggregationForQuery();
if (this.options.disableExternalPreAggregations && preAggForQuery.preAggregation.external) {
preAggForQuery = undefined;

Check warning on line 472 in packages/cubejs-schema-compiler/src/adapter/BaseQuery.js

View check run for this annotation

Codecov / codecov/patch

packages/cubejs-schema-compiler/src/adapter/BaseQuery.js#L472

Added line #L472 was not covered by tests
}
}
if (preAggForQuery) {
const {
Expand Down Expand Up @@ -537,7 +541,7 @@ export class BaseQuery {
}

externalPreAggregationQuery() {
if (!this.options.preAggregationQuery && this.externalQueryClass) {
if (!this.options.preAggregationQuery && !this.options.disableExternalPreAggregations && this.externalQueryClass) {
const preAggregationForQuery = this.preAggregations.findPreAggregationForQuery();
if (preAggregationForQuery && preAggregationForQuery.preAggregation.external) {
return true;
Expand All @@ -556,7 +560,7 @@ export class BaseQuery {
* @returns {Array<string>}
*/
buildSqlAndParams(exportAnnotatedSql) {
if (!this.options.preAggregationQuery && this.externalQueryClass) {
if (!this.options.preAggregationQuery && !this.options.disableExternalPreAggregations && this.externalQueryClass) {
if (this.externalPreAggregationQuery()) { // TODO performance
return this.externalQuery().buildSqlAndParams(exportAnnotatedSql);
}
Expand Down

0 comments on commit bb27698

Please sign in to comment.