Translate bytea.Any() as length > 0#3817
Conversation
There was a problem hiding this comment.
Pull request overview
Adds translation support for predicate-less Any() on byte[] columns mapped to PostgreSQL bytea, addressing EF Core 9/10’s rewrite of byte[].Length > 0 into Any() which previously failed translation.
Changes:
- Translate
Enumerable.Any()(no predicate) overbyteato a SQL length check (octet_length(...) > 0). - Expose
EnumerableMethods.AnyWithoutPredicatefor method matching. - Add a functional test asserting SQL generation for
byteanon-emptiness checks.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/EFCore.PG.FunctionalTests/Query/ArrayArrayQueryTest.cs | Adds coverage asserting SQL for querying non-empty bytea. |
| src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlByteArrayMethodTranslator.cs | Implements translation of bytea.Any() to a SQL length comparison. |
| src/EFCore.PG/Internal/EnumerableMethods.cs | Adds AnyWithoutPredicate MethodInfo for identifying predicate-less Any(). |
There was a problem hiding this comment.
Thanks @georg-jung, see some notes.
Can you also please retarget this PR against main, as I don't think we should patch 10.0.x with it?
Ah, I see this is a regression from 8, in that case we should indeed patch 10.0.x.
| [ConditionalFact] | ||
| public virtual async Task Bytea_Length_greater_than_zero() | ||
| { | ||
| await AssertQuery(ss => ss.Set<ArrayEntity>().Where(e => e.Bytea.Length > 0)); |
There was a problem hiding this comment.
Should this test for Any(), rather than for Length > 0?
Also, can you please move this test to ByteArrayTranslationsNpgsqlTest? The latter is then newer place for this kind of test (ArrayArrayQueryTest should eventually disappear).
There was a problem hiding this comment.
Actually I'm slightly confused, as #3816 talks about Length > 0 not working, but this PR implements support for Any() - are both broken, or just one of them? I'm noting that ByteArrayTranslationsNpgsqlTest does have test coverage for Length, so I assume it's working and the problem is only with Any?
There was a problem hiding this comment.
What caused the regression is that .Length > 0 was translated similar to .Length == 7 until incl. EF8, but as far as I understand it, EF started to rewrite .Length > 0 to .Any internally from EF 9. Thus, to effectively fix .Length > 0, this PR implements .Any.
Options include:
- test .Length > 0 (what we currently have); goes through the .Any path internally but the expression and the sql both use Length
- also test .Any in the expression, same translation, two tests
- only test .Any, also same translation
There was a problem hiding this comment.
There was a problem hiding this comment.
Probably dotnet/efcore#33644 introduced this
This reverts commit f9b705e.
3497195 to
05db0b8
Compare
bytea.Any() as octet_length > 0bytea.Any() as length > 0
roji
left a comment
There was a problem hiding this comment.
OK, all good - looks almost ready to merge.
| } | ||
|
|
||
| [ConditionalFact] | ||
| public virtual async Task Length_greater_than_zero() |
There was a problem hiding this comment.
Do you think this test is necessary, given that we already have coverage for Length and Any? This tests a composite tree pattern (so both ByteArray.Length and greater-than-zero), but we usually do that only when there's a special casing - here it seems like it's just the regular translation for Length, and then the regular translation for greater-than-zero.
Another way to think about it, is that AFAICT this doesn't add any more coverage than Length_greater_than_one, Length_greater_than_two would (and obviously we wouldn't do them).
(I understand that we now normalize Length > 0 to Any() - which is slightly "special" - but we already have the direct Any() test below to cover that)
So unless you think there's a specific reason for this test, we can consider removing it.
There was a problem hiding this comment.
I saw #3817 (comment) - I'm still not sure that test really adds anything, but I'm also OK with leaving it.
There was a problem hiding this comment.
Makes sense. I agree that one of them is sufficient to cover the relevant code paths, so I removed the Length > 0 test in e2ad4b0
|
(I'm not a user of EF 9, just wanted to note that we could consider cherry picking this in https://github.com/npgsql/efcore.pg/tree/hotfix/9.0.5 once merged, given the regression was introduced in EF9) |
| } | ||
|
|
||
| [ConditionalFact] | ||
| public virtual async Task Any() |
There was a problem hiding this comment.
BTW I'll "promote" this test to the EF test suite later as it's not very PG-specific.
|
Thanks @georg-jung! |
|
Thank you for the quick and insightful reviews, as always 🚀 |
Fixes #3816
I opted foroctet_lengthhere because it communicates clearly that it is about byte length, not text length.Let me know if this goes in the right direction and/or if I should change something.
Have a nice weekend!
(Note that I chose hotfix/10.0.2 as target branch; mostly because executing the test suite with an unreleased preview sdk was somewhat hard to setup)