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

Misleading exception message in "sum" aggregator (when aggregating no columns) #352

Open
koperagen opened this issue Apr 12, 2023 · 3 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@koperagen
Copy link
Collaborator

we have an example in readme with
pivot { destination }.sum { recentDelays.colsOf<Int?>() } into "total delays to"
but changing the type to colsOf<Int> leads to

Sum is not supported for kotlin.Number
java.lang.IllegalArgumentException: Sum is not supported for kotlin.Number

When in fact, all direct Number inheritors are supported, it's just nullable types are not. Maybe we can support them, maybe just change the error message

@Jolanrensen Jolanrensen added bug Something isn't working invalid This issue/PR doesn't seem right labels Apr 13, 2023
@Jolanrensen Jolanrensen added this to the 0.11.0 milestone Apr 13, 2023
@koperagen
Copy link
Collaborator Author

koperagen commented Apr 13, 2023

Actually, my explanation is wrong. Int? is supported by sum aggregation. Something is just wrong with the exception

@zaleslaw zaleslaw removed the invalid This issue/PR doesn't seem right label Apr 25, 2023
@koperagen koperagen modified the milestones: 0.11.0, 0.12.0 Jun 21, 2023
@koperagen koperagen modified the milestones: 0.12.0, 0.13.0 Oct 9, 2023
@Jolanrensen Jolanrensen modified the milestones: 0.13.+, Backlog Mar 22, 2024
@Jolanrensen
Copy link
Collaborator

Jolanrensen commented Nov 19, 2024

I suspect this is because recentDelays is a column group with only Int? columns, so you try to sum no columns. Sum can only accept Numbers, so it assumes you gave it 0 Number columns to sum, then the exception occurs... So yes, sum should support Number columns, but it should also give a proper error when no columns are selected to sum.

Can be reproduced with
emptyDataFrame<Any>().sum { none().cast() } and even emptyDataFrame<Any>().sum { none().cast<Int>() }.

I would probably solve this by adding a case to aggregateAll for when the user has given an empty columnSet, but maybe we can make the exception a bit more "personalized" to the individual aggregation.

@Jolanrensen
Copy link
Collaborator

Jolanrensen commented Nov 21, 2024

#937 will remove the exception, but not solve the issue. The emptyDataFrame<Any>().sum { none().cast<Int>() } will now be 0.0 (and a classCastException).
There's still the odd switch to Number for providing an empty columnset to sum and #937 makes Double the default for Number columns.

@Jolanrensen Jolanrensen changed the title Misleading exception message in "sum" aggregator Misleading exception message in "sum" aggregator (when aggregating no columns) Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants