Skip to content

Fix mathematical, bitwise and logical operator precedence in SQL #4221

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

Open
hazefully wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix mathematical, bitwise and logical operator precedence in SQL #4221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
51b5fda
Fix mathematical, bitwise and logical operator precedence in SQL
hazefully May 26, 2026
d2e65d5
Give higher precedence to bitwise operators
hazefully May 26, 2026
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
8 changes: 7 additions & 1 deletion fdb-relational-core/src/main/antlr/RelationalLexer.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1248,7 +1248,7 @@ OR_ASSIGN: '|=';

STAR: '*';
DIVIDE: '/';
MODULE: '%';
MODULO: '%';
PLUS: '+';
MINUS: '-';
DIV: 'DIV';
Expand All @@ -1269,7 +1269,13 @@ BIT_NOT_OP: '~';
BIT_OR_OP: '|';
BIT_AND_OP: '&';
BIT_XOR_OP: '^';
BIT_SHIFT_LEFT_OP: '<<';
BIT_SHIFT_RIGHT_OP: '>>';

// Operators. Logical

LOGICAL_AND_AND: '&&';
LOGICAL_OR_OR: '||';

// Constructors symbols

Expand Down
43 changes: 19 additions & 24 deletions fdb-relational-core/src/main/antlr/RelationalParser.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1221,12 +1221,16 @@ namedFunctionArg
;

// Expressions, predicates
// Mathemtical and logical expressions must be listed as different alternatives to the left-recursive

@hatyo hatyo Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please rewrite the comment to better illustrate the intentions and guide future modifications to the rules below, something like:

put comparison and logical operators higher so they have less precedence than below bitwise and arithmetic operators. Order of rules matter to leverage inter-rule precedence rules as supported by Antlr4.

// rules to represent the precedence of the underlying operators.

expression
: notOperator=(NOT | '!') expression #notExpression // done
: notOperator=(NOT | EXCLAMATION_SYMBOL) expression #notExpression // done
| EXISTS '(' query ')' #existsExpressionAtom // done
| expressionAtom predicate? #predicatedExpression
| expression logicalOperator expression #logicalExpression // done
| left=expressionAtom comparisonOperator right=expressionAtom #binaryComparisonExpression // done

@hatyo hatyo Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be expression comparisonOperator expression so we take advantage of left-recursion elimination and define clear precedence rules for groups of logical operators?

| left=expression operator=(AND | LOGICAL_AND_AND) right=expression #logicalExpression // done
| left=expression operator=(XOR | OR | LOGICAL_OR_OR) expression #logicalExpression // done
;

predicate
Expand All @@ -1237,16 +1241,19 @@ predicate
;

expressionAtom
: constant #constantExpressionAtom // done
| fullColumnName #fullColumnNameExpressionAtom // done
| functionCall #functionCallExpressionAtom // done
| preparedStatementParameter #preparedStatementParameterAtom // done
| recordConstructor #recordConstructorExpressionAtom // done
| arrayConstructor #arrayConstructorExpressionAtom // done
| base=expressionAtom LEFT_SQUARE_BRACKET index=expressionAtom RIGHT_SQUARE_BRACKET #subscriptExpression // done
| left=expressionAtom bitOperator right=expressionAtom #bitExpressionAtom // done
| left=expressionAtom mathOperator right=expressionAtom #mathExpressionAtom // done
| left=expressionAtom comparisonOperator right=expressionAtom #binaryComparisonPredicate // done
: constant #constantExpressionAtom // done
| fullColumnName #fullColumnNameExpressionAtom // done
| functionCall #functionCallExpressionAtom // done
| preparedStatementParameter #preparedStatementParameterAtom // done
| recordConstructor #recordConstructorExpressionAtom // done
| arrayConstructor #arrayConstructorExpressionAtom // done
| base=expressionAtom LEFT_SQUARE_BRACKET index=expressionAtom RIGHT_SQUARE_BRACKET #subscriptExpression // done
| left=expressionAtom operator=(BIT_SHIFT_LEFT_OP | BIT_SHIFT_RIGHT_OP) right=expressionAtom #bitExpressionAtom // done
| left=expressionAtom operator=BIT_AND_OP right=expressionAtom #bitExpressionAtom // done
| left=expressionAtom operator=(BIT_XOR_OP | BIT_OR_OP) right=expressionAtom #bitExpressionAtom // done
Comment on lines +1251 to +1253

@hazefully hazefully May 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hatyo Do you know if it is intentional that we used to have a higher precedence for bitwise operators over arithmetic operators? I think most languages (with few exceptions like Pascal) give a higher precedence to arithmetic operators so the expression 6 & 4 + 4 would be zero instead of 8, but it looks like we have some unit tests that assert we actually give precedence to the bitwise operators:

fdb-record-layer/fdb-relational-core/src/test/java/com/apple/foundationdb/relational/recordlayer/query/cache/ConstraintValidityTests.java

Lines 327 to 348 in 9d21654

void cachingQueryWithComplexGroupByExpressionSubsumingIndexExpressionBehavesCorrectlyCase2() throws Exception {
final var ticker = new FakeTicker();
final var cache = getCache(ticker);
final var c17Equals2 = equalsConstraint(17, 2);
final var c17Int = ofTypeInt(17);
final var c10Int = ofTypeInt(10);
final var c8Int = ofTypeInt(8);
final var c17IntNotNull = isNotNullInt(17);
final var c8IntNotNull = isNotNullInt(8);
final var c10IntNotNull = isNotNullInt(10);
final var c8Equalsc15 = covsEqualsConstraints(8, 17);
planQuery(cache, "SELECT MAX(score), game & 2 + 10 FROM score GROUP BY game & 2", BitAndScore2);
cacheShouldBe(cache, Map.of("SELECT MAX ( \"SCORE\" ) , \"GAME\" & ? + ? FROM \"SCORE\" GROUP BY \"GAME\" & ? ",
Map.of(ppe(cons(and(and(c17Equals2, c17Equals2), and(c17Int, c8Int, c10Int, c8Equalsc15, c17IntNotNull, c8IntNotNull, c10IntNotNull)))), BitAndScore2)));
final var c17Equals4 = equalsConstraint(17, 4);
planQuery(cache, "SELECT MAX(score), game & 4 + 10 FROM score GROUP BY game & 4", BitAndScore4);
cacheShouldBe(cache, Map.of("SELECT MAX ( \"SCORE\" ) , \"GAME\" & ? + ? FROM \"SCORE\" GROUP BY \"GAME\" & ? ",
Map.of(
ppe(cons(and(and(c17Equals2, c17Equals2), and(c17Int, c8Int, c10Int, c8Equalsc15, c17IntNotNull, c8IntNotNull, c10IntNotNull)))), BitAndScore2,
ppe(cons(and(and(c17Equals4, c17Equals4), and(c17Int, c8Int, c10Int, c8Equalsc15, c17IntNotNull, c8IntNotNull, c10IntNotNull)))), BitAndScore4)));
}

@hatyo hatyo Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not intentional, put differently, we did not make an explicit decision to give bitwise operations higher precedence over the other operators. The referenced unit test does not verify precedence, it just verifies that plan constraints are created exactly as they should be. That's all.

@hatyo hatyo Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most languages (with few exceptions like Pascal) give a higher precedence to arithmetic operators so the expression 6 & 4 + 4 would be zero instead of 8

I mean sure, but we should align with DB vendors (since bitwise operators are not part of the SQL standard) who for the most part, align with programming languages operator precedence conventions.

Bitwise operators are usually lower than basic arithmetic operator, and higher than logical operators. We should align with that. For example in PgSQL:

SELECT
1 + 3 & 5       AS actual,       -- 4 (proves + binds first)
(1 + 3) & 5     AS arith_first,  -- 4 (same as actual)
1 + (3 & 5)     AS bitwise_first -- 2 (different)
;
 actual | arith_first | bitwise_first 
--------+-------------+---------------
      4 |           4 |             2


SELECT
3 & 2 != 0                      AS actual,          -- true (proves & binds first)
(3 & 2) != 0                    AS bitwise_first,   -- true (same as actual)
3 & (2 != 0)::int               AS comparison_first -- 1 (different)

actual | bitwise_first | comparison_first
--------+---------------+------------------
 true   | true          | 1

Doing so, may break some customer queries though, so we have to be extra careful.

(once #4199 is ready, we can use it to introduce these illustrative examples).

| left=expressionAtom operator=(STAR|DIVIDE) right=expressionAtom #mathExpressionAtom // done
| left=expressionAtom operator=(MODULO|DIV|MOD) right=expressionAtom #mathExpressionAtom // done
| left=expressionAtom operator=(PLUS|MINUS) right=expressionAtom #mathExpressionAtom // done
;

inList
Expand All @@ -1270,18 +1277,6 @@ comparisonOperator
| IS NOT? DISTINCT FROM
;

logicalOperator
: AND | '&' '&' | XOR | OR | '|' '|'
;

bitOperator
: '<' '<' | '>' '>' | '&' | '^' | '|'
;

mathOperator
: '*' | '/' | '%' | DIV | MOD | '+' | '-'
;

jsonOperator
: '-' '>' | '-' '>' '>'
;
Expand Down
2 changes: 2 additions & 0 deletions ...com/apple/foundationdb/relational/recordlayer/query/functions/SqlFunctionCatalogImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,10 @@
.put("cosine_distance", argumentsCount -> BuiltInFunctionCatalog.resolve("cosine_distance", argumentsCount))
.put("dot_product_distance", argumentsCount -> BuiltInFunctionCatalog.resolve("dot_product_distance", argumentsCount))
.put("not", argumentsCount -> BuiltInFunctionCatalog.resolve("not", argumentsCount))
.put("and", argumentsCount -> BuiltInFunctionCatalog.resolve("and", argumentsCount))

Check notice on line 134 in fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/functions/SqlFunctionCatalogImpl.java

View workflow job for this annotation

GitHub Actions / coverage 

File coverage: 97.6% (81/83 lines) | Changed lines: 100.0% (2/2 lines)
.put("&&", argumentsCount -> BuiltInFunctionCatalog.resolve("and", argumentsCount))

@hatyo hatyo Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem relevant to this PR, can you please revert it (and line 137)? We can decide later if we want to add support to the non-standard && and ||.

.put("or", argumentsCount -> BuiltInFunctionCatalog.resolve("or", argumentsCount))
.put("||", argumentsCount -> BuiltInFunctionCatalog.resolve("or", argumentsCount))
.put("count", argumentsCount -> BuiltInFunctionCatalog.resolve("COUNT", argumentsCount))
.put("max", argumentsCount -> BuiltInFunctionCatalog.resolve("MAX", argumentsCount))
.put("min", argumentsCount -> BuiltInFunctionCatalog.resolve("MIN", argumentsCount))
Expand Down
22 changes: 2 additions & 20 deletions ...c/main/java/com/apple/foundationdb/relational/recordlayer/query/visitors/BaseVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1586,9 +1586,9 @@
}

@Nonnull
@Override

Check notice on line 1589 in fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/visitors/BaseVisitor.java

View workflow job for this annotation

GitHub Actions / coverage 

File coverage: 47.8% (143/299 lines) | Changed lines: 100.0% (1/1 lines)
public Expression visitBinaryComparisonPredicate(@Nonnull RelationalParser.BinaryComparisonPredicateContext ctx) {
return expressionVisitor.visitBinaryComparisonPredicate(ctx);
public Expression visitBinaryComparisonExpression(@Nonnull RelationalParser.BinaryComparisonExpressionContext ctx) {
return expressionVisitor.visitBinaryComparisonExpression(ctx);
}

@Override
Expand Down Expand Up @@ -1674,24 +1674,6 @@
return visitChildren(ctx);
}

@Nonnull
@Override
public Object visitLogicalOperator(@Nonnull RelationalParser.LogicalOperatorContext ctx) {
return visitChildren(ctx);
}

@Nonnull
@Override
public Object visitBitOperator(@Nonnull RelationalParser.BitOperatorContext ctx) {
return visitChildren(ctx);
}

@Nonnull
@Override
public Object visitMathOperator(@Nonnull RelationalParser.MathOperatorContext ctx) {
return visitChildren(ctx);
}

@Nonnull
@Override
public Object visitJsonOperator(@Nonnull RelationalParser.JsonOperatorContext ctx) {
Expand Down
22 changes: 2 additions & 20 deletions .../java/com/apple/foundationdb/relational/recordlayer/query/visitors/DelegatingVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1524,9 +1524,9 @@
}

@Nonnull
@Override

Check notice on line 1527 in fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/visitors/DelegatingVisitor.java

View workflow job for this annotation

GitHub Actions / coverage 

File coverage: 21.6% (61/283 lines) | Changed lines: 100.0% (1/1 lines)
public Expression visitBinaryComparisonPredicate(@Nonnull RelationalParser.BinaryComparisonPredicateContext ctx) {
return getDelegate().visitBinaryComparisonPredicate(ctx);
public Expression visitBinaryComparisonExpression(@Nonnull RelationalParser.BinaryComparisonExpressionContext ctx) {
return getDelegate().visitBinaryComparisonExpression(ctx);
}

@Override
Expand Down Expand Up @@ -1632,24 +1632,6 @@
return getDelegate().visitComparisonOperator(ctx);
}

@Nonnull
@Override
public Object visitLogicalOperator(@Nonnull RelationalParser.LogicalOperatorContext ctx) {
return getDelegate().visitLogicalOperator(ctx);
}

@Nonnull
@Override
public Object visitBitOperator(@Nonnull RelationalParser.BitOperatorContext ctx) {
return getDelegate().visitBitOperator(ctx);
}

@Nonnull
@Override
public Object visitMathOperator(@Nonnull RelationalParser.MathOperatorContext ctx) {
return getDelegate().visitMathOperator(ctx);
}

@Nonnull
@Override
public Object visitJsonOperator(@Nonnull RelationalParser.JsonOperatorContext ctx) {
Expand Down
8 changes: 4 additions & 4 deletions .../java/com/apple/foundationdb/relational/recordlayer/query/visitors/ExpressionVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,8 +502,8 @@
@Override
public Expression visitLogicalExpression(@Nonnull RelationalParser.LogicalExpressionContext ctx) {
final var left = Assert.castUnchecked(ctx.expression(0).accept(this), Expression.class);
final var right = Assert.castUnchecked(ctx.expression(1).accept(this), Expression.class);

Check notice on line 505 in fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/visitors/ExpressionVisitor.java

View workflow job for this annotation

GitHub Actions / coverage 

File coverage: 94.7% (463/489 lines) | Changed lines: 100.0% (3/3 lines)
return getDelegate().resolveFunction(ctx.logicalOperator().getText(), left, right);
return getDelegate().resolveFunction(ctx.operator.getText(), left, right);
}

@Nonnull
Expand Down Expand Up @@ -683,12 +683,12 @@
public Expression visitBitExpressionAtom(@Nonnull RelationalParser.BitExpressionAtomContext ctx) {
final var left = Assert.castUnchecked(ctx.left.accept(this), Expression.class);
final var right = Assert.castUnchecked(ctx.right.accept(this), Expression.class);
return getDelegate().resolveFunction(ctx.bitOperator().getText(), left, right);
return getDelegate().resolveFunction(ctx.operator.getText(), left, right);
}

@Nonnull
@Override
public Expression visitBinaryComparisonPredicate(@Nonnull RelationalParser.BinaryComparisonPredicateContext ctx) {
public Expression visitBinaryComparisonExpression(@Nonnull RelationalParser.BinaryComparisonExpressionContext ctx) {
final var left = Assert.castUnchecked(ctx.left.accept(this), Expression.class);
final var right = Assert.castUnchecked(ctx.right.accept(this), Expression.class);
return getDelegate().resolveFunction(ctx.comparisonOperator().getText(), left, right);
Expand Down Expand Up @@ -723,7 +723,7 @@
public Expression visitMathExpressionAtom(@Nonnull RelationalParser.MathExpressionAtomContext ctx) {
final var left = Assert.castUnchecked(ctx.left.accept(this), Expression.class);
final var right = Assert.castUnchecked(ctx.right.accept(this), Expression.class);
return getDelegate().resolveFunction(ctx.mathOperator().getText(), left, right);
return getDelegate().resolveFunction(ctx.operator.getText(), left, right);
}

@Nonnull
Expand Down
14 changes: 1 addition & 13 deletions .../main/java/com/apple/foundationdb/relational/recordlayer/query/visitors/TypedVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -912,8 +912,8 @@
Expression visitPredicatedExpression(@Nonnull RelationalParser.PredicatedExpressionContext ctx);

@Nonnull
@Override

Check notice on line 915 in fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/visitors/TypedVisitor.java

View workflow job for this annotation

GitHub Actions / coverage 

File coverage: N/A (no executable lines) | Changed lines: N/A (no executable lines)
Expression visitBinaryComparisonPredicate(@Nonnull RelationalParser.BinaryComparisonPredicateContext ctx);
Expression visitBinaryComparisonExpression(@Nonnull RelationalParser.BinaryComparisonExpressionContext ctx);

@Override
Expression visitSubscriptExpression(@Nonnull RelationalParser.SubscriptExpressionContext ctx);
Expand Down Expand Up @@ -970,18 +970,6 @@
@Override
Object visitComparisonOperator(@Nonnull RelationalParser.ComparisonOperatorContext ctx);

@Nonnull
@Override
Object visitLogicalOperator(@Nonnull RelationalParser.LogicalOperatorContext ctx);

@Nonnull
@Override
Object visitBitOperator(@Nonnull RelationalParser.BitOperatorContext ctx);

@Nonnull
@Override
Object visitMathOperator(@Nonnull RelationalParser.MathOperatorContext ctx);

@Nonnull
@Override
Object visitJsonOperator(@Nonnull RelationalParser.JsonOperatorContext ctx);
Expand Down
5 changes: 5 additions & 0 deletions yaml-tests/src/test/java/YamlIntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,4 +493,9 @@ public void recordTypeKeyTest(YamlTest.Runner runner) throws Exception {
public void filterIndexTest(YamlTest.Runner runner) throws Exception {
runner.runYamsql("filter-index.yamsql");
}

@TestTemplate
public void operatorPrecedenceTest(YamlTest.Runner runner) throws Exception {
runner.runYamsql("operator-precedence.yamsql");
}
}
Loading
Loading