Skip to content

Fix aggregate function on litteral #543

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

Merged
merged 2 commits into from
Feb 12, 2024

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Feb 11, 2024

Closes #460

Since you introduced this in #232, I'll be happy to have your review @arnaud-lb

When doing a SUM, I think the type must be "ungeneralize" (cf #460 )

When doing a AVG,

  • if the value is not constant, I think the type must be "ungeneralize" too
  • also, I got a failure without $type = TypeCombinator::union($type, $type->toFloat(), $type->toFloat()->toString()); because the AVG value computed in my tests was 1.0 or '1.0' (even for AVG(1)) and not 1or'1'` so I think we need to add the float result (and the stringified float result).

@VincentLanglet VincentLanglet force-pushed the fix/460-sum-aggregation branch from a024209 to 705981b Compare February 11, 2024 20:59
@VincentLanglet VincentLanglet marked this pull request as draft February 11, 2024 21:01
@VincentLanglet VincentLanglet force-pushed the fix/460-sum-aggregation branch from 705981b to da099d4 Compare February 11, 2024 21:26
@VincentLanglet VincentLanglet changed the base branch from 1.4.x to 1.3.x February 11, 2024 21:27
@VincentLanglet VincentLanglet force-pushed the fix/460-sum-aggregation branch from da099d4 to 42c4b04 Compare February 11, 2024 21:38
@VincentLanglet
Copy link
Contributor Author

Seems like

$type->toFloat()->toString()

doesn't do what I was looking for @ondrejmirtes, I may require your help to end this PR.

If I have a ConstantInteger(1), I can get a ConstantFloat(1.0) with the toFloat method.
What would be the recommended way to get a ConstantString('1.0') from this float now ?
The toString method is giving me a ConstantString('1')...

@janedbal
Copy link
Contributor

janedbal commented Feb 12, 2024

I'm using generalize for those cases (expand QueryResultTypeWalker)

@VincentLanglet VincentLanglet force-pushed the fix/460-sum-aggregation branch from bf700d5 to e8900d4 Compare February 12, 2024 08:48
@VincentLanglet VincentLanglet force-pushed the fix/460-sum-aggregation branch from e8900d4 to 9c3f7ae Compare February 12, 2024 08:48
@VincentLanglet
Copy link
Contributor Author

I'm using generalize for those cases (expand QueryResultTypeWalker)

I used generalize for the SUM method.

But if we're doing AVG(1) if thought this was better to return 1|'1'|... rather than float|int|numeric-string|null but after a good night, I think it's maybe a useless edge case to handle.

@VincentLanglet VincentLanglet marked this pull request as ready for review February 12, 2024 08:58
@janedbal
Copy link
Contributor

For AVG, it is edgecase, but the same issue for SUM is not.

Copy link
Contributor

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ondrejmirtes ondrejmirtes merged commit 4040cf0 into phpstan:1.3.x Feb 12, 2024
@ondrejmirtes
Copy link
Member

Thank you.

@VincentLanglet
Copy link
Contributor Author

Your welcome

Since 1.3.x is not the base branch it didn't closed automatically the issue #460 @ondrejmirtes

@VincentLanglet VincentLanglet deleted the fix/460-sum-aggregation branch February 12, 2024 19:20
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.

SUM() aggregation returns wrong type
4 participants