Skip to content
This repository has been archived by the owner on Nov 19, 2020. It is now read-only.

reduce: do not panic on null #77

Merged
merged 2 commits into from
Aug 3, 2020
Merged

reduce: do not panic on null #77

merged 2 commits into from
Aug 3, 2020

Conversation

kylebrandt
Copy link
Contributor

also switch to require over assert in testing.

@kylebrandt kylebrandt added the bug Something isn't working label Aug 3, 2020
Copy link
Member

@ryantxu ryantxu left a comment

Choose a reason for hiding this comment

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

for sure better than panic! however ignoring nils for min/max/etc may also be reasonable.

Obviously depends on where the data came from though -- with group by queries it is easy to end up with a bunch of meaningless nils. We could also consider using nullValueMode this was from the UI settings

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM

@kylebrandt
Copy link
Contributor Author

for sure better than panic! however ignoring nils for min/max/etc may also be reasonable.

Obviously depends on where the data came from though -- with group by queries it is easy to end up with a bunch of meaningless nils. We could also consider using nullValueMode this was from the UI settings

I agree. Just going with this for now, as dropping/ignoring nils is a bit more complicated (e.g., what do you do when everything is nil).

What I eventually want to have, is dropdowns with some of these options that control various things. So for reduction, stuff like this. For math, eg $A + $B there may be drop downs how to handle various join scenerios between the items in A and the items in B.

So this is just a quick fix, because that thing I made for fun with Azure and showed on Friday panicked, and I had to fix that ;-)

@kylebrandt kylebrandt merged commit 2e64f31 into master Aug 3, 2020
@kylebrandt kylebrandt deleted the reduceEmptyNil branch August 3, 2020 15:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants