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.

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

Support for row value comparisons #2350

Merged
merged 1 commit into from
May 7, 2022
Merged

Conversation

roji
Copy link
Member

@roji roji commented May 4, 2022

This implements dotnet/efcore#26822 for PostgreSQL only; things are easier here because there's already full support for arrays. I don't think this blocks us if we introduce something in relational, we can always obsolete the specific methods here.

Note: the methods have the long names (e.g. RowValueGreaterThan) since PG also supports array comparison in SQL.

/cc @smitpatel @ajcvickers @mrahhal

Closes #2349

@roji roji force-pushed the RowValueComparisons branch from 024ff54 to f869f88 Compare May 4, 2022 20:23
return new PostgresRowValueExpression(constant.Select(c => _sqlExpressionFactory.Constant(c)).ToArray());

default:
throw new ArgumentException($"{method.Name} accepts only new array initialization literals");

Choose a reason for hiding this comment

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

does this mean parameters are not allowed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately it does, at the moment... Apparently a LINQ NewArrayExpression gets parameterized the moment it contains a parameter inside it (if we somehow got the NewArrayExpression with ParameterExpression inside it, that would work).

As for handling a ParameterExpression representing the entire row value... PG does have "composite types" (like UDT, which represents a row in a table). At the ADO level Npgsql supports mapping these to CLR types and reading/writing them, but there's no composite support at the EF provider level (#22). If it's valuable/requested I can always add that later.

Note that composite parameters would also be PG-specific: while SQLite and MySQL support row values "literals" (as supported via this PR), I don't think they have a true 1st-class type like that, so a parameterized row value is probably impossible.

Makes sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's true this restriction does limit the usefulness of this feature, since typically you'd want to parameterize the constants on the right-hand side...

Copy link
Member Author

@roji roji May 4, 2022

Choose a reason for hiding this comment

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

So yeah... PostgreSQL doesn't currently allow parameterizing "anonymous" row types (or records), as in npgsql/npgsql#4437 (comment). Named row types (AKA composites) can be parameterized, but that means a type must exist prior to the query, which is inappropriate here (similar to how we can't use SQL Server TVPs in various scenarios).

I can add [NotParameterizable] to the array parameters of these new functions. This stops us from trying to parameterize the entire array, but also prevents parameterization of the elements inside the array, making them into constants instead. So:

var foo = "foo";
_ = ctx.Blogs.Where(b => EF.Functions.RowValueGreaterThan(new[] { b.Name1, b.Name2 }, new[] { "A", foo })).ToList();

... yields:

SELECT b."Id", b."Name1", b."Name2"
FROM "Blogs" AS b
WHERE (b."Name1", b."Name2") > ('A', 'foo')

(where 'foo' should be a parameter instead of a constant).

Copy link
Member Author

Choose a reason for hiding this comment

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

Just excluding all NewArrayExpression in EvaluatableExpressionFilter produces the right results (('A', @__foo_1)), but seems like too much... In theory if we had something like [NotParameterizable] but with more options, e.g. to prevent evaluability on a specific parameter...

OK enough, going to bed... 😴

Choose a reason for hiding this comment

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

Sounds fine to me, just wanted to make sure you have considered parameters also. Can always add it later if there is user feedback and an easy way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, looking at this again with fresh eyes, I guess excluding NewArrayExpression via EvaluatableExpressionFilter isn't too bad (I'm assuming evaluatable NewArrayExpressions should be rare?). Otherwise, if parameters aren't supported, this feature isn't very useful...

If you think otherwise or have some other bright idea, let me know.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ended up going with NewExpression of ValueTuple instead of NewArrayExpression, allowing us to disallow specifically that in EvaluatableExpressionFilter.

See #2349 (comment)

}

public override async Task Where_compare_tuple_constructed_multi_value_equal(bool async)
{
// Anonymous type to constant comparison. Issue #14672.
await AssertTranslationFailed(() => base.Where_compare_tuple_constructed_multi_value_equal(async));
await AssertQuery(
Copy link
Member Author

@roji roji May 7, 2022

Choose a reason for hiding this comment

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

/cc @smitpatel note these translation failure tests which now pass via row values, the resulting SQL is even a bit tighter:

WHERE (c."City", c."Country") = ('Sao Paulo', 'Brazil')

... rather than the expanded:

WHERE c."City" = 'Sao Paulo' AND c."Country" = 'Brazil'`

If we decide to bring this to relational, then specifically for SQL Server which doesn't support this, we could have a post-processing step which expands RowValueExpression to the equivalent verbose form (as above).

@roji roji force-pushed the RowValueComparisons branch from 47656a0 to 253c168 Compare May 7, 2022 19:32
@roji roji enabled auto-merge (squash) May 7, 2022 19:32
@roji roji merged commit 52f68c3 into npgsql:main May 7, 2022
@roji roji deleted the RowValueComparisons branch May 7, 2022 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow expression row value comparisons via extension methods
2 participants