Skip to content

Commit

Permalink
Allow optimizer hints (knex#4243)
Browse files Browse the repository at this point in the history 
Conflicts:
	lib/query/builder.js
	test/unit/query/builder.js
  • Loading branch information
@martinmacko47
martinmacko47 committed Jan 28, 2021
1 parent b260856 commit bde27d2
Show file tree
Hide file tree
Showing 6 changed files with 269 additions and 4 deletions.
3 changes: 2 additions & 1 deletion lib/dialects/mssql/query/compiler.js
View file
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
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
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
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
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
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

0 comments on commit bde27d2

Please sign in to comment.