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

Continue solving sheathing issues in infix subqueries #2387

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

deusaquilus
Copy link
Collaborator

After initially looking problems with the #2335 fix (for #2328) that were presented in #2340 the problem seems deeper then it originally seemed. The 1st problem was that materializeQueryMeta added an additional map clause to the query making any solution in SqlIdiom via case a: Query => more difficult. For example, in forUpdate in SqlDslSpec instead of this:

query[TestEntity].filter(t => t.s == "a")

The a parameter will be this:

query[TestEntity].filter(t => t.s == "a").map(x => (x.s, x.i, x.l, x.o))

This is problematic because if we want to use a NamingSchema e.g. io.getquill.naming.UpperCase we can introduce a Ast CaseClass:

CaseClass("S"->x.s, "I"->x.i, "L"->x.l, "O"->x.o)

Doing this after .map(x => (x.s, x.i, x.l, x.o)) makes no sense, this map clause is introduced by materializeQueryMeta.

Now materializeQueryMeta originally was introduced to expand aliases in queries for example in:

run( query[Person].map(p => p) )
// SELECT p.* FROM Person p

We need to expand that query to SELECT p.name, p.age FROM Person p in ExpandNestedQueries however if we just have Ident(p) in the SelectValue with no further information we do not know enough information to perform the expansion. This why materializeQueryMeta was introduced i.e. to add query[Person].map(p => p).map(p => (p.name, p.age)) to the query so that the correct SelectValue clauses could be introduced.
However, now since we have Quat information on identifiers, we know what fields are on Ident(p) (i.e. Ident(p, CC(name:V,age:V))) so we can just expand the fields normally. This has already been implemented in previous iterations in ExpandNestedQueries making the materializeQueryMeta component no longer necessary however it was kept in case the quat-based mechanism does not work. Now since there is a good use-case to remove it, this has been done in #2381.

Now what remains is to implement Implement the CaseClass wrapping as has been mentioned before. This has been attempted but so far is not working.

Additionally, even if this is implemented, the wrapping will not account for schemaMetas. This will likely be done in later steps.

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.

1 participant