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

Joining dj.U with aggregation result leads to incorrect result #449

Closed
eywalker opened this issue Feb 20, 2018 · 4 comments · Fixed by #839
Closed

Joining dj.U with aggregation result leads to incorrect result #449

eywalker opened this issue Feb 20, 2018 · 4 comments · Fixed by #839
Assignees
Labels

Comments

@eywalker
Copy link
Contributor

eywalker commented Feb 20, 2018

I'll follow up with a detail, but currently, query like the following seems to lead to incorrect query result:

dj.U('max_val') * rel.aggr(rel2, max_val='max(vals)')

This is caused by the fact that joining with dj.U('max_val') simply appends the attribute max_val into the heading's primary keys, and when the other operand is an instance of GroupBy, then adding this attribute into the primary key attributes of GroupBy results in change in the aggregation target. To deal with this case, the GroupBy must be wrapped in subquery first.

@eywalker
Copy link
Contributor Author

The short term work around for this issue is to wrap the result of aggregation with dj.relational_operand.Subquery as in:

dj.U('max_val') * dj.relational_operand.Subquery.create(rel.aggr(rel2, max_val='max(vals)'))

which is obviously very cumbersome and undesirable.

@eywalker eywalker self-assigned this Feb 20, 2018
@dimitri-yatsenko dimitri-yatsenko added this to the Release 0.11 milestone Feb 20, 2018
@dimitri-yatsenko
Copy link
Member

Possibly related to #386

@eywalker
Copy link
Contributor Author

Actually since it would be best to avoid sub query when possible to avoid speed issues, we might want to allow GroupBy to have aggregation target attribute that is different from the primary key in the heading.

@dimitri-yatsenko
Copy link
Member

dimitri-yatsenko commented Feb 20, 2018

Yes, as long as it operates correctly from the user's perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants