This repository was archived by the owner on Feb 21, 2024. It is now read-only.
handle count(*) in having correctly
#2274
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
seebs
force-pushed
the
fb-1997
branch
4 times, most recently
from
2276ed4 to
1e3880d
Compare
Contributor
Author
|
@tgruben, tagged you on review because Pat looked at the original second commit, figured out it was wrong, and fixed it, so now the second commit in particular needs someone else to look at it and verify that this makes sense. |
It turns out that `having count(*) ...` was always treating the count(*) as exactly 1. After studying this a lot, I noticed that in fact, we correctly handle other counts. The reason is that there's already code to recognize aggregates in `having` clauses as matching aggregates that are being computed -- but it only covers the other aggregate clause types, not the newly added `countStarPlanExpression` from making `count(*)` work even if there's no `_id` field. We add several corresponding test cases.
Added a test case for this, and also added a fix for it. Underlying issue: qualifiedRefPlanExpression could end up producing an int64 instead of a pql.Decimal, even though it had expected type Decimal. Originally this worked by politely converting an int64 to a pql.Decimal in the Evaluate phase, but this was not ideal; the real question is why it was coming out as an int64 at that step. Showed this to Pat, who spent a while studying it and produced a better fix.
|
Kudos, SonarCloud Quality Gate passed!
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After tracking through a whole bunch of unrelated code and coming up with a 40-line patch that mostly fixed this, I realized that the fact that it worked for count(field) meant something was making it work, added
countStarPlanExpressionto that case, and magically everything got better.Writing one line of code: 30 seconds
Figuring out which one line of code to write: two days