Patch for custom binary operators that use standard comparison symbols#426
Merged
roji merged 2 commits intonpgsql:devfrom May 26, 2018
Merged
Conversation
When a custom binary operator uses a standard comparison operator (`=`, `<>`, `<`, `>`, `<=`, `>=`) EF Core will generate a pattern that causes a syntax error in PostgreSQL. Example SQL: ```sql WHERE x."Inet" < @__inet_1 = TRUE ``` This patch adds logic to check if the custom binary operator is one of the standard comparison operators. If so, it wraps the expression in parenthesis. Example SQL (patched): ```sql WHERE (x."Inet" < @__inet_1) = TRUE ``` While it seems like this could be avoided by having the custom translator construct a standard `Expression.LessThan(Expression,Expression)`, this causes an error to be thrown...because the binary operator isn't defined. Example stack trace: ``` System.InvalidOperationException : The binary operator LessThanOrEqual is not defined for the types 'System.Net.IPAddress' and 'System.Net.IPAddress'. at System.Linq.Expressions.Expression.GetUserDefinedBinaryOperatorOrThrow(...) at System.Linq.Expressions.Expression.GetComparisonOperator(...) at System.Linq.Expressions.Expression.LessThanOrEqual(...) ``` Related: - dotnet/efcore#9143 - npgsql#323 (review)
austindrenski
added a commit
to austindrenski/npgsql-efcore.pg
that referenced
this pull request
May 26, 2018
- Includes functional tests convering network address operators. - Includes the network address function `ContainsOrContainedBy(...)` (previously omitted by mistake). - Includes an inline patch for npgsql#426 - Reordering functions/operators to reflect the docs for version 10. - TODO: - Add operators for `macaddr` and `macaddr8`. - Add some basic info to the docs.
roji
requested changes
May 26, 2018
Member
roji
left a comment
There was a problem hiding this comment.
Thanks for this... I remember thinking about this when originally implementing CustomBinaryExpression but not having enough time to investigate.
Regardless of the comments below, I'm actually leaning towards always putting parentheses, and not just in the case of comparison operators. The problem is that this problem could be triggered in another expressions - the custom binary expression could be embedded anywhere (especially due its very generic nature). This would result in slightly heavier SQL but we know it will always work.
PS For reference, here is PostgreSQL's operator precedence table.
| /// <remarks> | ||
| /// See: https://github.com/aspnet/EntityFrameworkCore/issues/9143 | ||
| /// </remarks> | ||
| [NotNull] static readonly HashSet<string> StandardComparisonSymbols = |
Member
There was a problem hiding this comment.
Perf isn't a huge factor here (because generated SQL is cached by EF Core), however:
In theory of course a contains operation is more efficient on a HashSet. In practice, however, for small sets it's much more efficient to simply use an array, because scanning through a local array and doing equality is faster than the overhead required by HashSet (calculating the hashcode, traversing the internal data structure...). You can do a quick BenchmarkDotNet test to see.
| // | ||
| // See: https://github.com/aspnet/EntityFrameworkCore/issues/9143 | ||
| //-------------------------------------------------------------------------------------------- | ||
| var operatorIsComparisonSymbol = StandardComparisonSymbols.Contains(expression.Operator); |
Member
There was a problem hiding this comment.
Let's rename this to operatorRequiresParentheses
| /// <remarks> | ||
| /// See: https://github.com/aspnet/EntityFrameworkCore/issues/9143 | ||
| /// </remarks> | ||
| [NotNull] static readonly HashSet<string> StandardComparisonSymbols = |
Member
There was a problem hiding this comment.
Let's rename this to OperatorsRequiringParentheses
Contributor
Author
@roji I updated it to always wrap. I think that's the right decision for now... but it would be nice to revisit this after the next release. Perhaps with some type of comprehensive precedence handling we could eliminate unnecessary wraps for the cleaner SQL. |
Member
|
@austindrenski thanks, although note the build failures. You need to update some tests which assert again SQLs (which now have additional parentheses). Unfortunately I can see some other unrelated failures as well (which you shouldn't worry about in this PR). Please also remove the big comment as this isn't really a "bug" related to dotnet/efcore#9143, it's just required behavior for an operator whose precedence via-à-vis the containing expression's operator we don't know. |
- A future PR could look more thoughtfully at operator precedence overall - Either at generation time or as an analytical stage. - See:https://www.postgresql.org/docs/current/static/sql-syntax-lexical.html#SQL-PRECEDENCE-TABLE - Updates tests that check for SQL affected by this PR.
bcb7011 to
87a418b
Compare
Contributor
Author
|
@roji Okay, tests should be updated now + dropped the comment. Not sure what to make of the Also, is |
Member
|
Thanks, merged. |
austindrenski
added a commit
to austindrenski/npgsql-efcore.pg
that referenced
this pull request
May 26, 2018
- Includes functional tests convering network address operators. - Includes the network address function `ContainsOrContainedBy(...)` (previously omitted by mistake). - Includes an inline patch for npgsql#426 - Reordering functions/operators to reflect the docs for version 10. - TODO: - Add operators for `macaddr` and `macaddr8`. - Add some basic info to the docs.
austindrenski
added a commit
to austindrenski/npgsql-efcore.pg
that referenced
this pull request
May 26, 2018
- Updates tests for new parentheses in custom binary operators.
- Renames `NetworkAddress*` to `Network*` to reflect the feature scope.
- Issues:
- Tests for `macaddr8` are tricky.
- Entity properties with column type `macaddr8` work fine.
- But parameters of type `PhysicalAddress` are treated as a `macaddr`.
- Causing: `System.FormatException : MAC addresses must have length 6 in PostgreSQL`
- Perhaps Npgsql could detect and upgrade `PhysicalAddress` values to `macaddr8` when they have 8 bytes?
- Opened npgsql#428 to discuss possible solutions.
austindrenski
added a commit
to austindrenski/npgsql-efcore.pg
that referenced
this pull request
May 27, 2018
- Includes functional tests convering network address operators. - Includes the network address function `ContainsOrContainedBy(...)` (previously omitted by mistake). - Includes an inline patch for npgsql#426 - Reordering functions/operators to reflect the docs for version 10. - TODO: - Add operators for `macaddr` and `macaddr8`. - Add some basic info to the docs.
austindrenski
added a commit
to austindrenski/npgsql-efcore.pg
that referenced
this pull request
May 27, 2018
- Updates tests for new parentheses in custom binary operators.
- Renames `NetworkAddress*` to `Network*` to reflect the feature scope.
- Issues:
- Tests for `macaddr8` are tricky.
- Entity properties with column type `macaddr8` work fine.
- But parameters of type `PhysicalAddress` are treated as a `macaddr`.
- Causing: `System.FormatException : MAC addresses must have length 6 in PostgreSQL`
- Perhaps Npgsql could detect and upgrade `PhysicalAddress` values to `macaddr8` when they have 8 bytes?
- Opened npgsql#428 to discuss possible solutions.
austindrenski
added a commit
to austindrenski/npgsql-efcore.pg
that referenced
this pull request
May 29, 2018
- Includes functional tests convering network address operators. - Includes the network address function `ContainsOrContainedBy(...)` (previously omitted by mistake). - Includes an inline patch for npgsql#426 - Reordering functions/operators to reflect the docs for version 10. - TODO: - Add operators for `macaddr` and `macaddr8`. - Add some basic info to the docs.
austindrenski
added a commit
to austindrenski/npgsql-efcore.pg
that referenced
this pull request
May 29, 2018
- Updates tests for new parentheses in custom binary operators.
- Renames `NetworkAddress*` to `Network*` to reflect the feature scope.
- Issues:
- Tests for `macaddr8` are tricky.
- Entity properties with column type `macaddr8` work fine.
- But parameters of type `PhysicalAddress` are treated as a `macaddr`.
- Causing: `System.FormatException : MAC addresses must have length 6 in PostgreSQL`
- Perhaps Npgsql could detect and upgrade `PhysicalAddress` values to `macaddr8` when they have 8 bytes?
- Opened npgsql#428 to discuss possible solutions.
austindrenski
added a commit
to austindrenski/npgsql-efcore.pg
that referenced
this pull request
May 29, 2018
- Includes functional tests convering network address operators. - Includes the network function `ContainsOrContainedBy(...)`. - Includes patch for npgsql#426 - Reordering functions/operators to reflect the docs for version 10. - TODO: - Add operators for `macaddr` and `macaddr8`. - Add some basic info to the docs.
austindrenski
added a commit
to austindrenski/npgsql-efcore.pg
that referenced
this pull request
May 29, 2018
- Updates tests for new parentheses in custom binary operators.
- Renames `NetworkAddress*` to `Network*` to reflect the feature scope.
- Issues:
- Tests for `macaddr8` are tricky.
- Entity properties with column type `macaddr8` work fine.
- But parameters of type `PhysicalAddress` are treated as a `macaddr`.
- `System.FormatException : MAC addresses must have length 6 in PostgreSQL`
- Perhaps Npgsql could detect and upgrade `PhysicalAddress` values to `macaddr8`?
- But only when they have 8 bytes?
- Opened npgsql#428 to discuss possible solutions.
austindrenski
added a commit
to austindrenski/npgsql-efcore.pg
that referenced
this pull request
May 31, 2018
- Includes functional tests convering network address operators. - Includes the network function `ContainsOrContainedBy(...)`. - Includes patch for npgsql#426 - Reordering functions/operators to reflect the docs for version 10. - TODO: - Add operators for `macaddr` and `macaddr8`. - Add some basic info to the docs.
austindrenski
added a commit
to austindrenski/npgsql-efcore.pg
that referenced
this pull request
May 31, 2018
- Updates tests for new parentheses in custom binary operators.
- Renames `NetworkAddress*` to `Network*` to reflect the feature scope.
- Issues:
- Tests for `macaddr8` are tricky.
- Entity properties with column type `macaddr8` work fine.
- But parameters of type `PhysicalAddress` are treated as a `macaddr`.
- `System.FormatException : MAC addresses must have length 6 in PostgreSQL`
- Perhaps Npgsql could detect and upgrade `PhysicalAddress` values to `macaddr8`?
- But only when they have 8 bytes?
- Opened npgsql#428 to discuss possible solutions.
austindrenski
added a commit
to austindrenski/npgsql-efcore.pg
that referenced
this pull request
Jun 3, 2018
- Includes functional tests convering network address operators. - Includes the network function `ContainsOrContainedBy(...)`. - Includes patch for npgsql#426 - Reordering functions/operators to reflect the docs for version 10. - TODO: - Add operators for `macaddr` and `macaddr8`. - Add some basic info to the docs.
austindrenski
added a commit
to austindrenski/npgsql-efcore.pg
that referenced
this pull request
Jul 15, 2018
- Includes functional tests convering network address operators. - Includes the network function `ContainsOrContainedBy(...)`. - Includes patch for npgsql#426 - Reordering functions/operators to reflect the docs for version 10. - TODO: - Add operators for `macaddr` and `macaddr8`. - Add some basic info to the docs.
austindrenski
added a commit
to austindrenski/npgsql-efcore.pg
that referenced
this pull request
Jul 21, 2018
- Includes functional tests convering network address operators. - Includes the network function `ContainsOrContainedBy(...)`. - Includes patch for npgsql#426 - Reordering functions/operators to reflect the docs for version 10. - TODO: - Add operators for `macaddr` and `macaddr8`. - Add some basic info to the docs.
austindrenski
added a commit
to austindrenski/npgsql-efcore.pg
that referenced
this pull request
Jul 22, 2018
- Includes functional tests convering network address operators. - Includes the network function `ContainsOrContainedBy(...)`. - Includes patch for npgsql#426 - Reordering functions/operators to reflect the docs for version 10. - TODO: - Add operators for `macaddr` and `macaddr8`. - Add some basic info to the docs.
austindrenski
added a commit
to austindrenski/npgsql-efcore.pg
that referenced
this pull request
Jul 22, 2018
- Includes functional tests convering network address operators. - Includes the network function `ContainsOrContainedBy(...)`. - Includes patch for npgsql#426 - Reordering functions/operators to reflect the docs for version 10. - TODO: - Add operators for `macaddr` and `macaddr8`. - Add some basic info to the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a custom binary operator uses a standard comparison operator (
=,<>,<,>,<=,>=) EF Core will generate a pattern that causes a syntax error in PostgreSQL.Example SQL:
This patch adds logic to check if the custom binary operator is one of the standard comparison operators. If so, it wraps the expression in parenthesis.
Example SQL (patched):
While it seems like this could be avoided by having the custom translator construct a standard
Expression.LessThan(Expression,Expression), this causes an error to be thrown...because the binary operator isn't defined.Example stack trace:
Related: