-
Notifications
You must be signed in to change notification settings - Fork 7
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
remove null assignments and check that the map is not empty check #479
Conversation
elliVM
commented
Jan 23, 2025
- Check that map is not empty before iterating it
- Remove the use of null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the logic has changed here somewhat. The map might be empty if there is no data found with the given query. The old version with the null might have let the query pass with an empty dataset, but the refactored version throws an exception. Letting the query through is probably the better option! Then the user would know that there was no data in the given time frame, because an empty dataset would be returned in the UI.
There is no test for this right now. There is one test for mode() in statsTransformationTest.java, but a test with an empty dataset could be added to see how it behaves currently as well as making sure that the logic doesn't change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see change proposal by @51-code
made some changes that on an empty dataset result will be a empty column. Added test for empty dataset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the solution for the mode aggregation looks really good, nice job. Left a comment about an import, maybe it was left behind after rewriting the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running spotless fixed the problem I commented on earlier. Approved!