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

Subqueryloader #330

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Subqueryloader #330

wants to merge 1 commit into from

Conversation

jekel
Copy link
Contributor

@jekel jekel commented Sep 6, 2018

SubqueryLoader is used to load Models from sqlalchemy aliased queries, opposite of #326 which works only on aliased gino Models
Common usecase for this loader will be - when you will have complex query with subquery, aggregated columns, etc... and you need to get result as Model object defined inside subquery

Previus discussion can be found here #323

@coveralls
Copy link

coveralls commented Sep 6, 2018

Pull Request Test Coverage Report for Build 1214

  • 19 of 19 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.008%) to 98.275%

Totals Coverage Status
Change from base Build 1209: 0.008%
Covered Lines: 3987
Relevant Lines: 4057

💛 - Coveralls

@wwwjfy
Copy link
Member

wwwjfy commented Sep 9, 2018

As discussed in #323, I'm not convinced that this is the complete solution. The use case in the unit test can be achieved in the latest version of Gino already. We do not need model and corresponding_column to get the column in subquery, as columns property of the subquery already has everything, as in https://github.com/fantix/gino/pull/326/files#diff-f6247715790dd30f031c9c3d67d5814eR148.
We do need better support for SQLAlchemy subqueries, or a wrapper. I'll come up with a good use case first.

@jekel
Copy link
Contributor Author

jekel commented Sep 9, 2018

We do need better support for SQLAlchemy subqueries, or a wrapper

Why not?
New users may need this functionality when porting their application on asyncio.

I'll come up with a good use case first.

Can you show me please also how can i achive the same without using corresponding_column in new use cases?

@wwwjfy
Copy link
Member

wwwjfy commented Sep 9, 2018

We "do" need, not we "don't" need. 😂

alias.corresponding_column(User.id) can be replaced by alias.columns.id, so we don't need User to be passed to the loader. I'm not saying it can cover all cases, but at least part of them.

@jekel
Copy link
Contributor Author

jekel commented Sep 9, 2018

@wwwjfy Sorry, i have read it wrong :)

alias.corresponding_column(User.id) can be replaced by alias.columns.id

what happens when there will be several models inside subquery? please take it mind when creating new use case

@wwwjfy
Copy link
Member

wwwjfy commented Sep 9, 2018

Exactly. That's the cases I'd like to explore, so as to give users straightforward loader and least surprise. Thanks 😊

@jekel jekel force-pushed the subqueryloader branch 2 times, most recently from a8cfb1d to 3f2356b Compare September 19, 2018 16:21
@jekel
Copy link
Contributor Author

jekel commented Oct 17, 2018

Hi @wwwjfy , how is going your research?

@wwwjfy
Copy link
Member

wwwjfy commented Oct 17, 2018

Sorry for the silence!

I just picked up a few issues again and will work out something this weekend.

@fantix fantix added this to the v1.1.x milestone Oct 19, 2018
@wwwjfy
Copy link
Member

wwwjfy commented Oct 21, 2018

I created #365, trying to show what can be done in the description of this issue, for subqueries and aggregate functions. Basically, it won't be hard as long as we keep the reference of columns and/or models in a subquery and pass them to loaders.

@wwwjfy
Copy link
Member

wwwjfy commented Oct 21, 2018

I don't know if this can meet your needs. If not, an example will be very helpful.

@jekel
Copy link
Contributor Author

jekel commented Oct 23, 2018

@wwwjfy i have found the case your code does not cover - complete model loading from subquery without specifying all columns explicitly

@wwwjfy
Copy link
Member

wwwjfy commented Oct 24, 2018

@jekel I could achieve this:

async def test(user):
    ua = User.alias()
    query = select([ua]).alias()
    outer_query = select([text('1'), query])
    result = await outer_query.gino.load(ua).all()
    assert len(result) == 1
    assert result[0].id == user.id

Is it what you said?

@fantix fantix force-pushed the master branch 4 times, most recently from d3bba04 to 79e7d4c Compare December 27, 2019 07:42
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.

None yet

4 participants