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

Normalization of array access to ElementAt breaks broke PG JSON array traversal #30386

Open
Tracked by #2540
roji opened this issue Mar 2, 2023 · 2 comments
Open
Tracked by #2540

Comments

@roji
Copy link
Member

roji commented Mar 2, 2023

#29656 added CollectionIndexerToElementAtNormalizingExpressionVisitor to the preprocessing phase; this visitor normalizes array and list indexing (e.g. arr[2], list[2]) to ElementAt method calls.

Unfortunately, this breaks JSON array traversal on the PG side. The normalization itself isn't the issue,

Input query (test JsonPocoQueryTest.Array_of_object):

var x = ctx.JsonbEntities.Single(e => e.Customer.Orders[0].Price == 99.5m);

WIth 8.0-preview.1, we get the following tree after preprocessing:

DbSet<JsonbEntity>()
    .Where(j => j.Customer.Orders
        .Select(s => s.Price)
        .ElementAt(0) == 99.5)
    .Single()

Note how the member access for Price gets pulled up as a Select before the ElementAt; I'm guessing this is maybe the doing of nav expansion. In any case, this tree no longer faithfully represents what's going on; do we translate this construct in the EF SQL Server JSON support?

/cc @maumar

@maumar
Copy link
Contributor

maumar commented Mar 2, 2023

The rewrite is done in SubqueryMemberPushdownExpressionVisitor.
It was done to support queries like this:

ss.Set<JsonEntityBasic>()
                .Select(x => new { x.Id, x.OwnedCollectionRoot[0].OwnedCollectionBranch[prm].OwnedCollectionLeaf })
                .AsNoTracking()

i.e. collection navigation after array access. Without the pushdown, nav expansion doesn't know how to wrap MaterializeCollectionNavigationExpression around the collection call, and we need that to properly reason about the results when processing the query further.

This is the query before nav expansion (but with pushdown):

DbSet<JsonEntityBasic>()
    .Select(x => new { 
        Id = x.Id, 
        OwnedCollectionLeaf = x.OwnedCollectionRoot
            .AsQueryable()
            .ElementAt(0).OwnedCollectionBranch
            .AsQueryable()
            .Select(s => s.OwnedCollectionLeaf)
            .ElementAt(__prm_0)
     })

this is after nav expansion

DbSet<JsonEntityBasic>()
    .Select(j => new { 
        Id = j.Id, 
        OwnedCollectionLeaf = EF.Property<List<JsonOwnedBranch>>(EF.Property<List<JsonOwnedRoot>>(j, "OwnedCollectionRoot")
            .AsQueryable()
            .ElementAt(0), "OwnedCollectionBranch")
            .AsQueryable()
            .Select(o0 => MaterializeCollectionNavigation(
                Navigation: JsonEntityBasic.OwnedCollectionRoot#JsonOwnedRoot.OwnedCollectionBranch#JsonOwnedBranch.OwnedCollectionLeaf,
                subquery: EF.Property<List<JsonOwnedLeaf>>(o0, "OwnedCollectionLeaf")
                    .AsQueryable()))
            .ElementAt(__prm_0)
     })

without the pushdown, the query would look something like this:

DbSet<JsonEntityBasic>()
    .Select(x => new { 
        Id = x.Id, 
        OwnedCollectionLeaf = x.OwnedCollectionRoot
            .AsQueryable()
            .ElementAt(0).OwnedCollectionBranch
            .AsQueryable()
            .ElementAt(__prm_0).OwnedCollectionLeaf
     })

and we would need to back-track to the outer select to apply MCNE

@maumar
Copy link
Contributor

maumar commented Mar 2, 2023

but the specific reason why JSON needed this change was when it array access chain ends in collection navigation (maybe a reference nav - need to double check). If that's not something that npg need to worry about, perhaps we could make the rewrite more targeted to that scenario?

@ajcvickers ajcvickers added this to the Backlog milestone Mar 9, 2023
lauxjpn added a commit to lauxjpn/Pomelo.EntityFrameworkCore.MySql that referenced this issue Sep 22, 2023
…roperty).ElementAt(index), because it messes-up our JSON-Array handling in `MySqlSqlTranslatingExpressionVisitor`. See dotnet/efcore#30386.
lauxjpn added a commit to lauxjpn/Pomelo.EntityFrameworkCore.MySql that referenced this issue Sep 23, 2023
…roperty).ElementAt(index), because it messes-up our JSON-Array handling in `MySqlSqlTranslatingExpressionVisitor`. See dotnet/efcore#30386.
lauxjpn added a commit to lauxjpn/Pomelo.EntityFrameworkCore.MySql that referenced this issue Sep 26, 2023
lauxjpn added a commit to lauxjpn/Pomelo.EntityFrameworkCore.MySql that referenced this issue Sep 29, 2023
…roperty).ElementAt(index), because it messes-up our JSON-Array handling in `MySqlSqlTranslatingExpressionVisitor`. See dotnet/efcore#30386.
lauxjpn added a commit to lauxjpn/Pomelo.EntityFrameworkCore.MySql that referenced this issue Sep 29, 2023
lauxjpn added a commit to PomeloFoundation/Pomelo.EntityFrameworkCore.MySql that referenced this issue Sep 29, 2023
* Update dependencies to .NET 8 and EF Core 8 and make projects compile.

* Support b.Property(...).HasMaxLength(-1).

* Fix UPDATE statement translation.

* Update server version support.

* Fix casting of datetime(n), timestamp(n) and time(n).

* Track LIMIT support using anything but a constant value.

* Add ElementAt() support.

* Add DateTimeOffset.ToUnixTimeSeconds() and DateTimeOffset.ToUnixTimeMilliseconds() support.

* Add DegreesToRadians() and RadiansToDegrees() support for float/double.

* Skips normalization of array[index].Property to array.Select(e => e.Property).ElementAt(index), because it messes-up our JSON-Array handling in `MySqlSqlTranslatingExpressionVisitor`. See dotnet/efcore#30386.

* Add IMethodCallTranslator implementation with QueryCompilationContext support.

* Add improved parameter support for StartsWith, EndsWith and Contains translations.

* Throw meaningful error message when trying to use EF Core 7 JSON mappings.

* Ensure JSON value compatible type mappings.

* Fix StartsWith, EndsWith and Contains edge case translations.

* Implement primitive collection handling.

* Fix class name.

* Workaround dotnet/efcore#30386 for now.

* Implement flags to prevent MySQL 8 from crashing at some later point, when using JSON_TABLE(@JSON, ..) with @JSON being a parameter.

* Workaround MySQL 8 engine crash at some later point, when using JSON_TABLE(@JSON, ..) with @JSON being a parameter, by directly inlining the parameter values in question into the generated SQL.

* Fix JSON_VALUE support.

* Check for JSON_TABLE compatibility.

* Fix legacy VALUES generation.

* Fix restart sequence support.

* Implement database engine bug mitigations.

* Add EnablePrimitiveCollectionsSupport option.

* Update test project.

* Update test runner.

* Fix tests for MySQL 8.0.

* Fix tests for MySQL 5.7.

* Fix tests for MariaDB 11.1.

* Fix tests for MariaDB 11.0.

* Update branding to 8.0.0-beta.1.

* Update JSON_TABLE issue number.

* Update CI.
lauxjpn added a commit to lauxjpn/Pomelo.EntityFrameworkCore.MySql that referenced this issue Sep 29, 2023
…roperty).ElementAt(index), because it messes-up our JSON-Array handling in `MySqlSqlTranslatingExpressionVisitor`. See dotnet/efcore#30386.
lauxjpn added a commit to lauxjpn/Pomelo.EntityFrameworkCore.MySql that referenced this issue Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants