Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions packages/cubejs-api-gateway/test/normalize-query.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,3 +161,15 @@ describe('cubeSqlRequestSchema', () => {
expect(cubeSqlRequestSchema.validate({ ...baseBody, timezone: tz }).error).toBeDefined();
});
});

describe('limit normalization', () => {
test('keeps an explicit limit of 0 instead of applying the default limit', () => {
const result = normalizeQuery({ ...baseQuery, limit: 0 }, false);
expect(result.limit).toBe(0);
});

test('applies the default limit when no limit is given', () => {
const result = normalizeQuery({ ...baseQuery }, false);
expect(result.limit).toBeGreaterThan(0);
});
});
39 changes: 35 additions & 4 deletions packages/cubejs-schema-compiler/src/adapter/BaseQuery.js
Original file line number Diff line number Diff line change
Expand Up @@ -958,8 +958,10 @@ export class BaseQuery {
securityContext: this.contextSymbols.securityContext,
order,
filters: this.options.filters,
limit: this.options.limit ? this.options.limit.toString() : null,
rowLimit: this.options.rowLimit ? this.options.rowLimit.toString() : null,
limit: this.options.limit != null ? this.options.limit.toString() : null,
// `rowLimit: 0` is a valid limit (BI tools use `LIMIT 0` as a schema probe),
// so it must not be collapsed into `null` (no limit) here
rowLimit: this.options.rowLimit != null ? this.options.rowLimit.toString() : null,
offset: this.options.offset ? this.options.offset.toString() : null,
baseTools: this,
ungrouped: this.options.ungrouped,
Expand Down Expand Up @@ -1015,8 +1017,10 @@ export class BaseQuery {
cubeEvaluator: this.cubeEvaluator,
order,
filters: this.options.filters,
limit: this.options.limit ? this.options.limit.toString() : null,
rowLimit: this.options.rowLimit ? this.options.rowLimit.toString() : null,
limit: this.options.limit != null ? this.options.limit.toString() : null,
// `rowLimit: 0` is a valid limit (BI tools use `LIMIT 0` as a schema probe),
// so it must not be collapsed into `null` (no limit) here
rowLimit: this.options.rowLimit != null ? this.options.rowLimit.toString() : null,
offset: this.options.offset ? this.options.offset.toString() : null,
baseTools: this,
ungrouped: this.options.ungrouped,
Expand Down Expand Up @@ -3212,6 +3216,33 @@ export class BaseQuery {
return '';
}

/**
* Row limit as a number, or `null` when it is not set at all. Unlike a truthy check
* this keeps `0` (a valid limit that returns no rows) distinct from "no limit", and
* unlike a bare `parseInt` it keeps a non-numeric `rowLimit` out of the rendered SQL.
* @protected
* @returns {number|null}
*/
parsedRowLimit() {
if (this.rowLimit == null) {
return null;
}
const parsed = parseInt(this.rowLimit, 10);
return Number.isNaN(parsed) ? null : parsed;
}

/**
* Leading row-limit clause for statements that do not render `topLimit()` -- the legacy
* rollup query in `PreAggregations` is the one such statement. Only dialects that cannot
* express a zero row limit as a trailing clause (T-SQL, where FETCH NEXT must be >= 1)
* return anything here; every other dialect renders `LIMIT 0` and gets `''`.
* @public
* @returns {string}
*/
zeroRowLimitTopClause() {
return '';
}

baseSelect() {
return R.flatten(this.forSelect().map(s => s.selectColumns())).filter(s => !!s).join(', ');
}
Expand Down
42 changes: 38 additions & 4 deletions packages/cubejs-schema-compiler/src/adapter/MssqlQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,11 @@ export class MssqlQuery extends BaseQuery {

// TODO replace with limitOffsetClause override
public groupByDimensionLimit() {
// T-SQL requires FETCH NEXT to be greater than zero, so a zero row limit is
// rendered as `TOP 0` by topLimit() instead, and OFFSET is redundant for it
if (this.parsedRowLimit() === 0) {
return '';
}
Comment thread
claude[bot] marked this conversation as resolved.
if (this.rowLimit) {
return this.offset ? ` OFFSET ${parseInt(this.offset, 10)} ROWS FETCH NEXT ${parseInt(this.rowLimit, 10)} ROWS ONLY` : '';
} else {
Expand All @@ -160,10 +165,32 @@ export class MssqlQuery extends BaseQuery {
}

public topLimit() {
// Deliberately a strict null check: an explicit `rowLimit: null` means "no limit",
// while an absent one keeps the historical TOP 10000 default below, since T-SQL has
// no LIMIT clause to fall back on
if (this.rowLimit === null) {
return '';
}
const rowLimit = this.parsedRowLimit();
// `TOP 0` is the only way to express an empty result in T-SQL, and it takes
// precedence over the offset branch below: OFFSET without FETCH would return rows
if (rowLimit === 0) {
return ' TOP 0';
}
if (this.offset) {
return '';
}
return this.rowLimit === null ? '' : ` TOP ${this.rowLimit && parseInt(this.rowLimit, 10) || 10000}`;
return ` TOP ${rowLimit ?? 10000}`;
}

/**
* The legacy rollup query in `PreAggregations` renders no `topLimit()`, so a zero row
* limit would otherwise emit no row-limiting clause there at all (groupByDimensionLimit()
* cannot express it: FETCH NEXT must be >= 1 in T-SQL) and scan the whole rollup.
* @override
*/
public zeroRowLimitTopClause() {
return this.parsedRowLimit() === 0 ? ' TOP 0' : '';
}

/**
Expand Down Expand Up @@ -344,7 +371,9 @@ export class MssqlQuery extends BaseQuery {
templates.statements.select = '{% if ctes %} WITH \n' +
'{{ ctes | join(\',\n\') }}\n' +
'{% endif %}' +
'SELECT {% if limit is not none and not order_by %}TOP {{ limit }} {% endif %}{% if distinct %}DISTINCT {% endif %}' +
// T-SQL clause order is SELECT [ALL | DISTINCT] [TOP (expr)], so DISTINCT has to come
// first: `SELECT TOP 0 DISTINCT ...` is a syntax error
'SELECT {% if distinct %}DISTINCT {% endif %}{% if limit is not none and (not order_by or limit == 0) %}TOP {{ limit }} {% endif %}' +
'{{ select_concat | map(attribute=\'aliased\') | join(\', \') }} {% if from %}\n' +
'FROM (\n' +
'{{ from | indent(2, true) }}\n' +
Expand All @@ -354,8 +383,13 @@ export class MssqlQuery extends BaseQuery {
'{% if filter %}\nWHERE {{ filter }}{% endif %}' +
'{% if group_by %}\nGROUP BY {{ group_by }}{% endif %}' +
'{% if having %}\nHAVING {{ having }}{% endif %}' +
'{% if order_by %}\nORDER BY {{ order_by | map(attribute=\'expr\') | join(\', \') }}\nOFFSET {% if offset is not none %}{{ offset }}{% else %}0{% endif %} ROWS' +
'\nFETCH NEXT {% if limit is not none %}{{ limit }}{% else %}2147483647{% endif %} ROWS ONLY{% endif %}' +
'{% if order_by %}\nORDER BY {{ order_by | map(attribute=\'expr\') | join(\', \') }}' +
// FETCH NEXT must be greater than zero in T-SQL, so `LIMIT 0` is rendered as
// `TOP 0` above and the OFFSET/FETCH tail is dropped entirely. `limit` is always a
// number here (both renderers pass Option<usize>); `limit | int` would not work as a
// guard, since `none | int` is 0 and that would drop the 2147483647 fallback below
'{% if limit != 0 %}\nOFFSET {% if offset is not none %}{{ offset }}{% else %}0{% endif %} ROWS' +
'\nFETCH NEXT {% if limit is not none %}{{ limit }}{% else %}2147483647{% endif %} ROWS ONLY{% endif %}{% endif %}' +
'{% if ctes %}\nOPTION (MAXRECURSION 0){% endif %}';
// MSSQL has no boolean type — a segment projected as a dimension must be a BIT.
templates.expressions.wrap_segment_select = 'CAST((CASE WHEN {{ expr }} THEN 1 ELSE 0 END) AS BIT)';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,11 @@ export class OracleQuery extends BaseQuery {
* TODO replace with limitOffsetClause override
*/
public groupByDimensionLimit() {
const limitClause = this.rowLimit === null ? '' : ` FETCH NEXT ${this.rowLimit && parseInt(this.rowLimit, 10) || 10000} ROWS ONLY`;
// `rowLimit: 0` is a valid limit that returns no rows, so it must not fall back to the
// default below the way a truthy check would. Same null policy as MssqlQuery#topLimit:
// an explicit `rowLimit: null` means "no limit", an absent one keeps the 10000 default
const rowLimit = this.parsedRowLimit() ?? 10000;
const limitClause = this.rowLimit === null ? '' : ` FETCH NEXT ${rowLimit} ROWS ONLY`;
const offsetClause = this.offset ? ` OFFSET ${parseInt(this.offset, 10)} ROWS` : '';
return `${offsetClause}${limitClause}`;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1624,8 +1624,10 @@ export class PreAggregations {

return this.query.evaluateSymbolSqlWithContext(
() => {
// zeroRowLimitTopClause() is empty for every dialect that can express a zero row
// limit as a trailing clause; T-SQL can not, and this statement has no topLimit()
// eslint-disable-next-line prefer-template
const query = `SELECT ${this.query.selectAllDimensionsAndMeasures(measures)} FROM ${from} ${this.query.baseWhere(replacedFilters)}` +
const query = `SELECT${this.query.zeroRowLimitTopClause()} ${this.query.selectAllDimensionsAndMeasures(measures)} FROM ${from} ${this.query.baseWhere(replacedFilters)}` +
this.query.groupByClause();
return isFullSimpleQuery ?
this.query.baseHaving(query, this.query.measureFilters) +
Expand Down
188 changes: 188 additions & 0 deletions packages/cubejs-schema-compiler/test/unit/mssql-query.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,194 @@ describe('MssqlQuery', () => {
expect(/GROUP BY/.test(queryString)).toEqual(false);
}));

it('renders rowLimit: 0 as TOP 0 without an invalid FETCH NEXT 0', async () => {
await compiler.compile();

// With an ORDER BY the template would normally emit OFFSET/FETCH NEXT, but T-SQL
// rejects `FETCH NEXT 0 ROWS ONLY`, so a zero limit has to go through TOP
const query = new MssqlQuery({ joinGraph, cubeEvaluator, compiler }, {
measures: ['visitors.count'],
dimensions: ['visitors.source'],
order: [{ id: 'visitors.source', desc: false }],
timezone: 'UTC',
rowLimit: 0,
});

const sql = query.buildSqlAndParams()[0];

expect(sql).toContain('TOP 0');
expect(sql).not.toContain('FETCH NEXT');
});

it('renders rowLimit: 0 with an offset as TOP 0 and no OFFSET tail', async () => {
await compiler.compile();

// T-SQL forbids TOP together with OFFSET/FETCH, and a zero limit yields no rows
// whatever the offset is, so the whole OFFSET/FETCH tail has to go
const query = new MssqlQuery({ joinGraph, cubeEvaluator, compiler }, {
measures: ['visitors.count'],
dimensions: ['visitors.source'],
order: [{ id: 'visitors.source', desc: false }],
timezone: 'UTC',
rowLimit: 0,
offset: 10,
});

const sql = query.buildSqlAndParams()[0];

expect(sql).toContain('TOP 0');
expect(sql).not.toContain('FETCH NEXT');
expect(sql).not.toContain('OFFSET');
});

it('still renders OFFSET/FETCH NEXT for a non-zero rowLimit with an offset', async () => {
await compiler.compile();

const query = new MssqlQuery({ joinGraph, cubeEvaluator, compiler }, {
measures: ['visitors.count'],
dimensions: ['visitors.source'],
order: [{ id: 'visitors.source', desc: false }],
timezone: 'UTC',
rowLimit: 5,
offset: 10,
});

const sql = query.buildSqlAndParams()[0];

expect(sql).toContain('OFFSET 10 ROWS');
expect(sql).toContain('FETCH NEXT 5 ROWS ONLY');
expect(sql).not.toContain('TOP');
});

it('renders DISTINCT before TOP in the select template', async () => {
await compiler.compile();

const query = new MssqlQuery({ joinGraph, cubeEvaluator, compiler }, {
measures: ['visitors.count'],
timezone: 'UTC',
rowLimit: 0,
});

// T-SQL clause order is SELECT [ALL | DISTINCT] [TOP (expr)], so `SELECT TOP 0 DISTINCT`
// is a syntax error. A single select carrying both is reachable through the cubesql
// wrapper (`SELECT DISTINCT ... LIMIT 0`), which can't be built from here, so the
// template itself is what gets pinned
const { select } = query.sqlTemplates().statements;

expect(select).toContain('DISTINCT');
expect(select).toContain('TOP');
expect(select.indexOf('DISTINCT')).toBeLessThan(select.indexOf('TOP'));
});

it('keeps rowLimit: 0 out of the legacy limit clauses', async () => {
await compiler.compile();

const query = new MssqlQuery({ joinGraph, cubeEvaluator, compiler }, {
measures: ['visitors.count'],
timezone: 'UTC',
rowLimit: 0,
offset: 10,
});

expect(query.topLimit()).toEqual(' TOP 0');
expect(query.groupByDimensionLimit()).toEqual('');
// The legacy rollup query in PreAggregations renders no topLimit(), so the zero limit
// has to come from this hook or that statement would scan the whole rollup
expect(query.zeroRowLimitTopClause()).toEqual(' TOP 0');
});

it('renders no leading zero-limit clause for a non-zero rowLimit', async () => {
await compiler.compile();

const query = new MssqlQuery({ joinGraph, cubeEvaluator, compiler }, {
measures: ['visitors.count'],
timezone: 'UTC',
rowLimit: 5,
});

expect(query.zeroRowLimitTopClause()).toEqual('');
});

it('renders TOP 0 in the legacy-planner pre-aggregation rollup query', async () => {
// The rollup statement in PreAggregations renders no topLimit(), and T-SQL cannot put a
// zero limit in a trailing clause, so without zeroRowLimitTopClause() a `rowLimit: 0`
// query served from a pre-aggregation would scan the whole rollup
const preAggCompilers = prepareJsCompiler(`
cube('visits', {
sql: 'SELECT * FROM visits',

preAggregations: {
bySource: {
measures: [CUBE.count],
dimensions: [CUBE.source],
},
},

measures: {
count: { type: 'count' },
},

dimensions: {
id: { sql: 'id', type: 'number', primaryKey: true },
source: { sql: 'source', type: 'string' },
},
});
`);
await preAggCompilers.compiler.compile();

const queryOptions = {
measures: ['visits.count'],
dimensions: ['visits.source'],
timezone: 'UTC',
useNativeSqlPlanner: false,
preAggregationsSchema: '',
};

const zeroLimit = new MssqlQuery({
joinGraph: preAggCompilers.joinGraph,
cubeEvaluator: preAggCompilers.cubeEvaluator,
compiler: preAggCompilers.compiler,
}, { ...queryOptions, rowLimit: 0 });

const zeroLimitSql = zeroLimit.buildSqlAndParams()[0];

expect(zeroLimit.preAggregations.findPreAggregationForQuery()).toBeDefined();
expect(zeroLimitSql).toContain('TOP 0');

const nonZeroLimit = new MssqlQuery({
joinGraph: preAggCompilers.joinGraph,
cubeEvaluator: preAggCompilers.cubeEvaluator,
compiler: preAggCompilers.compiler,
}, { ...queryOptions, rowLimit: 5 });

// Non-zero limits keep their existing rendering on this path
expect(nonZeroLimit.buildSqlAndParams()[0]).not.toContain('TOP 0');
});

it('keeps DISTINCT and TOP 0 in a valid order for a multiplied-measure query', async () => {
await joinedSchemaCompilers.compiler.compile();

// Multiplied measures make the full-key-aggregate path emit DISTINCT keys sub-selects
// alongside the TOP 0 outer select
const query = new MssqlQuery({
joinGraph: joinedSchemaCompilers.joinGraph,
cubeEvaluator: joinedSchemaCompilers.cubeEvaluator,
compiler: joinedSchemaCompilers.compiler,
}, {
measures: ['B.bval_sum', 'C.count'],
dimensions: ['B.bid'],
order: [{ id: 'B.bid', desc: false }],
timezone: 'UTC',
rowLimit: 0,
});

const sql = query.buildSqlAndParams()[0];

expect(sql).toContain('TOP 0');
expect(sql).toContain('DISTINCT');
expect(sql).not.toMatch(/TOP\s+0\s+DISTINCT/);
});

it('aggregating on top of sub-queries', async () => {
await joinedSchemaCompilers.compiler.compile();
const query = new MssqlQuery({
Expand Down
Loading
Loading