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.

Sign up for GitHub

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

Jump to bottom

Allow optimizer hints - Backport to 0.21 #4258

Merged
merged 1 commit into from
Jan 30, 2021
Merged

Allow optimizer hints - Backport to 0.21 #4258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/dialects/mssql/query/compiler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ class QueryCompiler_MSSQL extends QueryCompiler {
let distinctClause = '';
if (this.onlyUnions()) return '';
const top = this.top();
const hints = this._hintComments()
const columns = this.grouped.columns || [];
let i = -1,
sql = [];
Expand All @@ -285,7 +286,7 @@ class QueryCompiler_MSSQL extends QueryCompiler {
if (sql.length === 0) sql = ['*'];

return (
`select ${distinctClause}` +
`select ${hints}${distinctClause}` +
(top ? top + ' ' : '') +
sql.join(', ') +
(this.tableName ? ` from ${this.tableName}` : '')
Expand Down
21 changes: 20 additions & 1 deletion lib/query/builder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,25 @@ assign(Builder.prototype, {
return this;
},

// Adds a single hint or an array of hits to the list of "hintComments" on the query.
hintComment(hints) {
hints = Array.isArray(hints) ? hints : [hints]
if (hints.some((hint) => !isString(hint))) {
throw new Error('Hint comment must be a string');
}
if (hints.some((hint) => hint.includes('/*') || hint.includes('*/'))) {
throw new Error('Hint comment cannot include "/*" or "*/"');
}
if (hints.some((hint) => hint.includes('?'))) {
throw new Error('Hint comment cannot include "?"');
}
this._statements.push({
grouping: 'hintComments',
value: hints,
})
return this;
},

// Prepends the `schemaName` on `tableName` defined by `.table` and `.join`.
withSchema(schemaName) {
this._single.schema = schemaName;
Expand Down Expand Up @@ -1064,7 +1083,7 @@ assign(Builder.prototype, {

// Remove everything from statement clause
clear(statement) {
if (!['with', 'select', 'columns', 'where', 'union', 'join', 'group', 'order', 'having', 'limit', 'offset', 'counter', 'counters'].includes(statement))
if (!['with', 'select', 'columns', 'hintComments', 'where', 'union', 'join', 'group', 'order', 'having', 'limit', 'offset', 'counter', 'counters'].includes(statement))
throw new Error(`Knex Error: unknown statement '${statement}'`);
if (statement.startsWith('counter')) return this.clearCounters();
if (statement === 'select') { statement = 'columns'; }
Expand Down
10 changes: 9 additions & 1 deletion lib/query/compiler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,10 +182,18 @@ class QueryCompiler {
);
}

_hintComments() {
let hints = this.grouped.hintComments || [];
hints = hints.map((hint) => compact(hint.value).join(' '));
hints = compact(hints).join(' ');
return hints ? `/*+ ${hints} */ ` : ''
}

// Compiles the columns in the query, specifying if an item was distinct.
columns() {
let distinctClause = '';
if (this.onlyUnions()) return '';
const hints = this._hintComments()
const columns = this.grouped.columns || [];
let i = -1,
sql = [];
Expand All @@ -208,7 +216,7 @@ class QueryCompiler {
}
if (sql.length === 0) sql = ['*'];
return (
`select ${distinctClause}` +
`select ${hints}${distinctClause}` +
sql.join(', ') +
(this.tableName
? ` from ${this.single.only ? 'only ' : ''}${this.tableName}`
Expand Down
72 changes: 72 additions & 0 deletions test/integration/builder/selects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,78 @@ module.exports = function (knex) {
});
});

it('#4199 - adheres to hint comments', async function () {
const expectedErrors = {
mysql: {
code: 'ER_QUERY_TIMEOUT',
errno: 3024,
sqlMessage: 'Query execution was interrupted, maximum statement execution time exceeded',
},
mysql2: {
errno: 3024,
sqlMessage: 'Query execution was interrupted, maximum statement execution time exceeded',
},
}
if (!expectedErrors[knex.client.driverName]) {
return this.skip();
}
const baseQuery = knex('accounts')
.select('id', knex.raw('sleep(0.1)'))
.limit(2)
await expect(
baseQuery.clone()
).to.eventually.be.fulfilled.and.to.have.lengthOf(2)
await expect(
baseQuery.clone().hintComment('max_execution_time(10)')
).to.eventually.be.rejected.and.to.deep.include(expectedErrors[knex.client.driverName])
});

it('#4199 - ignores invalid hint comments', async function () {
return knex
.select('id')
.orderBy('id')
.from('accounts')
.hintComment('invalid()')
.testSql(function (tester) {
tester(
'mysql',
'select /*+ invalid() */ `id` from `accounts` order by `id` asc',
[],
[{id: 1}, {id: 2}, {id: 3}, {id: 4}, {id: 5}, {id: 7}]
);
tester(
'pg',
'select /*+ invalid() */ "id" from "accounts" order by "id" asc',
[],
[{id: '1'}, {id: '2'}, {id: '3'}, {id: '4'}, {id: '5'}, {id: '7'}]
);
tester(
'pg-redshift',
'select /*+ invalid() */ "id" from "accounts" order by "id" asc',
[],
[{id: '1'}, {id: '2'}, {id: '3'}, {id: '4'}, {id: '5'}, {id: '6'}]
);
tester(
'sqlite3',
'select /*+ invalid() */ `id` from `accounts` order by `id` asc',
[],
[{id: 1}, {id: 2}, {id: 3}, {id: 4}, {id: 5}, {id: 6}]
);
tester(
'oracledb',
'select /*+ invalid() */ "id" from "accounts" order by "id" asc',
[],
[{id: 1}, {id: 2}, {id: 3}, {id: 4}, {id: 5}, {id: 7}]
);
tester(
'mssql',
'select /*+ invalid() */ [id] from [accounts] order by [id] asc',
[],
[{id: '1'}, {id: '2'}, {id: '3'}, {id: '4'}, {id: '5'}, {id: '7'}]
);
});
});

it('returns a single entry with first', function () {
return knex
.first('id', 'first_name')
Expand Down
159 changes: 159 additions & 0 deletions test/unit/query/builder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,7 @@ describe('QueryBuilder', () => {
})
.join('tableJoin','id', 'id')
.select(['id'])
.hintComment('hint()')
.where('id', '<', 10)
.groupBy('id')
.groupBy('id', 'desc')
Expand All @@ -770,6 +771,7 @@ describe('QueryBuilder', () => {
.clear('columns')
.select(['id'])
.clear('select')
.clear('hintComments')
.clear('where')
.clear('group')
.clear('order')
Expand Down Expand Up @@ -8324,6 +8326,163 @@ describe('QueryBuilder', () => {
});
});

it('#4199 - allows an hint comment', () => {
testsql(
qb()
.from('testtable')
.hintComment('hint()'),
{
mysql: {
sql: 'select /*+ hint() */ * from `testtable`',
bindings: [],
},
oracledb: {
sql: 'select /*+ hint() */ * from "testtable"',
bindings: [],
},
mssql: {
sql: 'select /*+ hint() */ * from [testtable]',
bindings: [],
},
pg: {
sql: 'select /*+ hint() */ * from "testtable"',
bindings: [],
},
'pg-redshift': {
sql: 'select /*+ hint() */ * from "testtable"',
bindings: [],
},
},
)
});

it('#4199 - allows multiple hint comments', () => {
testsql(
qb()
.from('testtable')
.hintComment(['hint1()', 'hint2()'])
.hintComment('hint3()'),
{
mysql: {
sql: 'select /*+ hint1() hint2() hint3() */ * from `testtable`',
bindings: [],
},
oracledb: {
sql: 'select /*+ hint1() hint2() hint3() */ * from "testtable"',
bindings: [],
},
mssql: {
sql: 'select /*+ hint1() hint2() hint3() */ * from [testtable]',
bindings: [],
},
pg: {
sql: 'select /*+ hint1() hint2() hint3() */ * from "testtable"',
bindings: [],
},
'pg-redshift': {
sql: 'select /*+ hint1() hint2() hint3() */ * from "testtable"',
bindings: [],
},
},
)
});

it('#4199 - allows hint comments in subqueries', () => {
testsql(
qb()
.select({
c1: 'c1',
c2: qb().select('c2').from('t2').hintComment('hint2()').limit(1),
})
.from('t1')
.hintComment('hint1()'),
{
mysql: {
sql: 'select /*+ hint1() */ `c1` as `c1`, (select /*+ hint2() */ `c2` from `t2` limit ?) as `c2` from `t1`',
bindings: [1],
},
oracledb: {
sql: 'select /*+ hint1() */ "c1" "c1", (select * from (select /*+ hint2() */ "c2" from "t2") where rownum <= ?) "c2" from "t1"',
bindings: [1],
},
mssql: {
sql: 'select /*+ hint1() */ [c1] as [c1], (select /*+ hint2() */ top (?) [c2] from [t2]) as [c2] from [t1]',
bindings: [1],
},
pg: {
sql: 'select /*+ hint1() */ "c1" as "c1", (select /*+ hint2() */ "c2" from "t2" limit ?) as "c2" from "t1"',
bindings: [1],
},
'pg-redshift': {
sql: 'select /*+ hint1() */ "c1" as "c1", (select /*+ hint2() */ "c2" from "t2" limit ?) as "c2" from "t1"',
bindings: [1],
},
},
)
});

it('#4199 - allows hint comments in unions', () => {
testsql(
qb()
.from('t1')
.hintComment('hint1()')
.unionAll(qb().from('t2').hintComment('hint2()')),
{
mysql: {
sql: 'select /*+ hint1() */ * from `t1` union all select /*+ hint2() */ * from `t2`',
bindings: [],
},
oracledb: {
sql: 'select /*+ hint1() */ * from "t1" union all select /*+ hint2() */ * from "t2"',
bindings: [],
},
mssql: {
sql: 'select /*+ hint1() */ * from [t1] union all select /*+ hint2() */ * from [t2]',
bindings: [],
},
pg: {
sql: 'select /*+ hint1() */ * from "t1" union all select /*+ hint2() */ * from "t2"',
bindings: [],
},
'pg-redshift': {
sql: 'select /*+ hint1() */ * from "t1" union all select /*+ hint2() */ * from "t2"',
bindings: [],
},
},
)
});

it('#4199 - forbids "/*", "*/" and "?" in hint comments', () => {
expect(() => {
qb().from('testtable').hintComment('hint() /*').toString();
}).to.throw(
'Hint comment cannot include "/*" or "*/"'
);
expect(() => {
qb().from('testtable').hintComment('hint() */').toString();
}).to.throw(
'Hint comment cannot include "/*" or "*/"'
);
expect(() => {
qb().from('testtable').hintComment('hint(?)').toString();
}).to.throw(
'Hint comment cannot include "?"'
);
});

it('#4199 - forbids non-strings as hint comments', () => {
expect(() => {
qb().from('testtable').hintComment(47).toString();
}).to.throw(
'Hint comment must be a string'
);
expect(() => {
qb().from('testtable').hintComment(raw('hint(?)', [47])).toString();
}).to.throw(
'Hint comment must be a string'
);
});

it('Any undefined binding in a SELECT query should throw an error', () => {
const qbuilders = [
{
Expand Down
8 changes: 7 additions & 1 deletion types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,13 +460,14 @@ declare namespace Knex {
//
// QueryInterface
//
type ClearStatements = "with" | "select" | "columns" | "where" | "union" | "join" | "group" | "order" | "having" | "limit" | "offset" | "counter" | "counters";
type ClearStatements = "with" | "select" | "columns" | "hintComments" | "where" | "union" | "join" | "group" | "order" | "having" | "limit" | "offset" | "counter" | "counters";

interface QueryInterface<TRecord extends {} = any, TResult = any> {
select: Select<TRecord, TResult>;
as: As<TRecord, TResult>;
columns: Select<TRecord, TResult>;
column: Select<TRecord, TResult>;
hintComment: HintComment<TRecord, TResult>;
from: Table<TRecord, TResult>;
into: Table<TRecord, TResult>;
table: Table<TRecord, TResult>;
Expand Down Expand Up @@ -978,6 +979,11 @@ declare namespace Knex {
): QueryBuilder<TRecord, TResult2>;
}

interface HintComment<TRecord extends {} = any, TResult extends {} = any> {
(hint: string): QueryBuilder<TRecord, TResult>;
(hints: readonly string[]): QueryBuilder<TRecord, TResult>;
}

interface Table<TRecord extends {} = any, TResult extends {} = any> {
<
TTable extends TableNames,
Expand Down