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

Handling null updates in sum test cases #2341

Closed
wants to merge 1 commit into from

Conversation

adetsi
Copy link
Contributor

@adetsi adetsi commented Aug 15, 2023

Upon investigation, this issue (#1256) is not really an issue, it is as a result of the way perspective view is interacted with, creating new instances of perspective view upon table update flips the aggregate value back and forth, however, not creating new instances of perspective view upon table update increments the sum aggregate (issue complained about by @nmichaud ). @texodus @timkpaine @sc1f could you please review if all is good?

Sum value flips back and forth when a new view is created upon table update

flip.mp4

Sum value increments when no new view is created upon table update

increment.mp4

Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

#1256 is definitely a bug. Your second test's assertion is the wrong behavior, both of these tests should behave identically regardless of when View the is created. Perspective does not have a sum() function which behaves like this (though high and low aggregates do accumulate perpetually in View state).

Please do lint your fixes before opening a PR in the future, our CI will immediately fail it anyway so no point committing with --no-verify.

@texodus texodus closed this Aug 31, 2023
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.

2 participants