Skip to content

Wrap expressions with parenthesis based on their precedence #228

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Wrap expressions with parenthesis based on their precedence #228

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 43 additions & 15 deletions src/AbstractSQLRules2SQL.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@
return aSql + ' IS ' + bSql;
} else {
throw new SyntaxError(
'IsDistinctFrom/IsNotDistinctFrom not supported on: ' + engine,

Check warning on line 293 in src/AbstractSQLRules2SQL.ts

View workflow job for this annotation

GitHub Actions / Flowzone / Test npm (20.x) 

Invalid operand for a '+' operation. Operands must each be a number or string, allowing a string + any of: `any`, `boolean`, `null`, `RegExp`, `undefined`. Got `never`

Check warning on line 293 in src/AbstractSQLRules2SQL.ts

View workflow job for this annotation

GitHub Actions / Flowzone / Test npm (22.x) 

Invalid operand for a '+' operation. Operands must each be a number or string, allowing a string + any of: `any`, `boolean`, `null`, `RegExp`, `undefined`. Got `never`
);
}
};
Expand Down Expand Up @@ -387,14 +387,41 @@
};
export type MathOps = keyof typeof mathOps;

const mathOperatorNodeTypes = new Set([
...Object.keys(mathOps),
'AddDateDuration',
'AddDateNumber',
'SubtractDateDate',
'SubtractDateDuration',
'SubtractDateNumber',
]);
const opsPrecedence = (() => {
const operatorsByPrecedence = [
['Multiply', 'Divide'],
[
'Add',
'AddDateDuration',
'AddDateNumber',
'Subtract',
'SubtractDateDate',
'SubtractDateDuration',
'SubtractDateNumber',
],
['Between', 'Like'],
[
'Equals',
'NotEquals',
'GreaterThan',
'GreaterThanOrEqual',
'LessThan',
'LessThanOrEqual',
],
// In, Exists, NotExists, 'IsDistinctFrom', 'IsNotDistinctFrom', Not,
// And, Or are already adding parenthesis.
] as const;

const operatorPrecedence = {} as Record<string, number>;
let precedence = 0;
for (const samePrecedenceOps of operatorsByPrecedence) {
for (const op of samePrecedenceOps) {
operatorPrecedence[op] = precedence;
}
precedence++;
}
return operatorPrecedence;
})();

const precedenceSafeOpValue = (
parentNodeType: string,
Expand All @@ -406,11 +433,12 @@
const operandAbstractSql = getAbstractSqlQuery(args, index);
const valueExpr = valueMatchFn(operandAbstractSql, indent);
const [childNodeType] = operandAbstractSql;
const parentOperatorPrecedence = opsPrecedence[parentNodeType];
const childOperatorPrecedence = opsPrecedence[childNodeType];
if (
(mathOperatorNodeTypes.has(parentNodeType) &&
mathOperatorNodeTypes.has(childNodeType)) ||
// We need parenthesis for chained boolean comparisons, otherwise PostgreSQL complains.
(parentNodeType in comparisons && childNodeType in comparisons)
childOperatorPrecedence != null &&
parentOperatorPrecedence != null &&
parentOperatorPrecedence <= childOperatorPrecedence
) {
return `(${valueExpr})`;
}
Expand Down Expand Up @@ -1016,9 +1044,9 @@
},
Between: (args, indent) => {
checkArgs('Between', args, 3);
const v = AnyValue(getAbstractSqlQuery(args, 0), indent);
const a = AnyValue(getAbstractSqlQuery(args, 1), indent);
const b = AnyValue(getAbstractSqlQuery(args, 2), indent);
const v = precedenceSafeOpValue('Between', AnyValue, args, 0, indent);
const a = precedenceSafeOpValue('Between', AnyValue, args, 1, indent);
const b = precedenceSafeOpValue('Between', AnyValue, args, 2, indent);
return `${v} BETWEEN ${a} AND (${b})`;
},
Add: MathOp('Add'),
Expand Down
59 changes: 57 additions & 2 deletions test/abstract-sql/comparisons.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,9 @@ describe('Comparison Operator Precedence', () => {
it('should produce a valid NotEquals statement when the operands are math expressions with nested parenthesis', () => {
sqlEquals(
result.query,
// ideally we shouldn't have any parenthesis here
stripIndent`
SELECT 1 + (2 + (3 * 4)) != 1 + 0
SELECT 1 + (2 + 3 * 4) != 1 + 0
`,
);
});
Expand Down Expand Up @@ -159,7 +160,7 @@ describe('Comparison Operator Precedence', () => {
sqlEquals(
result.query,
stripIndent`
SELECT 1 + (2 * 5) >= 12 + 2
SELECT 1 + 2 * 5 >= 12 + 2
`,
);
});
Expand Down Expand Up @@ -330,6 +331,33 @@ describe('Comparison Operator Precedence', () => {
},
);

test(
[
'SelectQuery',
[
'Select',
[
[
'Between',
['Equals', ['Integer', 1], ['Integer', 0]],
['LessThan', ['Integer', 1], ['Integer', 0]],
['GreaterThan', ['Integer', 1], ['Integer', 0]],
],
],
],
],
(result, sqlEquals) => {
it('should produce a valid Between statement when the operands are comparison expressions', () => {
sqlEquals(
result.query,
stripIndent`
SELECT (1 = 0) BETWEEN (1 < 0) AND ((1 > 0))
`,
);
});
},
);

test(
[
'SelectQuery',
Expand All @@ -356,4 +384,31 @@ describe('Comparison Operator Precedence', () => {
});
},
);

test(
[
'SelectQuery',
[
'Select',
[
[
'Between',
['Equals', ['Integer', 1], ['Integer', 0]],
['LessThan', ['Integer', 1], ['Integer', 0]],
['GreaterThan', ['Integer', 1], ['Integer', 0]],
],
],
],
],
(result, sqlEquals) => {
it('should produce a valid Between statement when the operands are comparison expressions', () => {
sqlEquals(
result.query,
stripIndent`
SELECT (1 = 0) BETWEEN (1 < 0) AND ((1 > 0))
`,
);
});
},
);
});
10 changes: 5 additions & 5 deletions test/abstract-sql/math.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ describe('Math Operator Precedence', () => {
],
(result, sqlEquals) => {
it('should produce a valid Add statement when the first operand is a Multiply', () => {
sqlEquals(result.query, 'SELECT (2 * 3) + 4');
sqlEquals(result.query, 'SELECT 2 * 3 + 4');
});
},
);
Expand All @@ -143,7 +143,7 @@ describe('Math Operator Precedence', () => {
],
(result, sqlEquals) => {
it('should produce a valid Add statement when the second operand is a Multiply', () => {
sqlEquals(result.query, 'SELECT 2 + (3 * 4)');
sqlEquals(result.query, 'SELECT 2 + 3 * 4');
});
},
);
Expand All @@ -164,7 +164,7 @@ describe('Math Operator Precedence', () => {
],
(result, sqlEquals) => {
it('should produce a valid Add statement of two Multiplications', () => {
sqlEquals(result.query, 'SELECT (2 * 3) + (4 * 5)');
sqlEquals(result.query, 'SELECT 2 * 3 + 4 * 5');
});
},
);
Expand Down Expand Up @@ -236,7 +236,7 @@ describe('Math Operator Precedence', () => {
],
(result, sqlEquals) => {
it('should produce a valid Subtract statement of two Multiplications', () => {
sqlEquals(result.query, 'SELECT (2 * 3) - (4 * 5)');
sqlEquals(result.query, 'SELECT 2 * 3 - 4 * 5');
});
},
);
Expand All @@ -257,7 +257,7 @@ describe('Math Operator Precedence', () => {
],
(result, sqlEquals) => {
it('should produce a valid Subtract statement of two Divisions', () => {
sqlEquals(result.query, 'SELECT (2 / 3) - (4 / 5)');
sqlEquals(result.query, 'SELECT 2 / 3 - 4 / 5');
});
},
);
Expand Down
Loading