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

TupleLoader distinct on all included loaders #324

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

Conversation

jekel
Copy link
Contributor

@jekel jekel commented Sep 3, 2018

When using TupleLoader with complex query like: (ModelOne.distinct(ModelOne.id).load(...=ModelTwo.distinct...), CustomLoader(query.column_name))
result is incorrect, becase, it assumes that all rows results from each loader are distinct which is not always true

@jekel jekel changed the title Tuple loader distinct on all included loaders TupleLoader distinct on all included loaders Sep 3, 2018
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1148

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

Totals Coverage Status
Change from base Build 1144: 0.004%
Covered Lines: 3954
Relevant Lines: 4024

💛 - Coveralls

@wwwjfy
Copy link
Member

wwwjfy commented Sep 9, 2018

Thanks for the PR!

As you said, it's not always true that all results in sub-loaders are distinct. I also understand from the added test case. But it's also not always true that all results have to be distinct to make this tuple distinct, depending on use cases. For example, I want to iterate through (company, team) pairs instead of company.teams which needs two levels of for-loops.
As a tuple loader, each tuple represents a row in the raw result, so the job for it should be to simply convert elements in the raw result to models, and users need to decide how to use them.

@jekel
Copy link
Contributor Author

jekel commented Sep 9, 2018

Hm... i have an idea, TupleLoader must also relay on group by columns inside query to correctly handle all cases

@wwwjfy
Copy link
Member

wwwjfy commented Sep 16, 2018

By group by, do you mean the group by in the SQL? If so, there won't be duplicated rows. Say group by company.id, we won't get two rows of the same company.id, right?

One way to get what you want may be to check each returned tuple, and save it to context, like ModelLoader does. I'll explore and see if it makes sense.

@jekel
Copy link
Contributor Author

jekel commented Sep 20, 2018

I mean, when you have query result like that, with group by User:

User Company
User_1 Company_1
User_1 Company_2
User2 Company_1

we can automaticaly find it in group by columns and call company attribute to load Companies list,
to not explicitly add User.Distinct in query

@wwwjfy
Copy link
Member

wwwjfy commented Sep 20, 2018

I'm not sure I get what you mean.
If it's implemented, what would the syntax be like?

Like this?

TupleLoader(
    (
        User.load(add_company=Company),
        ColumnLoader(query.c.test_column),
    ),
    group_by=User,
)

@fantix fantix added this to the v1.1.x milestone Oct 19, 2018
@fantix fantix force-pushed the master branch 10 times, most recently from 684f7b1 to 3969c27 Compare February 1, 2019 16:29
@fantix fantix force-pushed the master branch 5 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.

4 participants