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

fix: big number to handle NULL as it did in the past #9314

Merged
merged 1 commit into from Mar 17, 2020

Conversation

mistercrunch
Copy link
Member

context: #9313

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

can we add a unit test to this PR as this was a regression that was recently introduced?

@mistercrunch
Copy link
Member Author

I can write a unit test covering get_data later tonight

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

ok. I think i'd feel comfortable accepting then once you add the manual test plan and some screenshots

however, a unit test should fast follow

@willbarrett
Copy link
Member

I'm working on a unit test for this PR now.

@willbarrett
Copy link
Member

@etr2460 unfortunately I was unable to come up with a comprehensive unit test by the end of my day. This will have to wait for @mistercrunch to free up again this evening. Apologies.

@mistercrunch
Copy link
Member Author

Definitely was worth taking the time to write the unit test. Pandas is doing some funky things here in pivot_table. While clearly np.isnan(np.sum(np.nan)) == True df.pivot_table({...}, aggfunc=np.sum) of a column that has only None returns 0.

Anyhow. This here works, I picked np.min as no aggregates apply (already at the right grain) and with np.min a None leads to nan, which works well in the end (same as before the original PR). I tested this same unit test on before this PR and it fails (as expected, with the 0 value in place of the None/nan/NULL), and tested on before the a commit prior to the PR that created the problem 753aeb482991f0dd3f36373f6d248ce277c67949 and this unit test passes. This gives pretty high confidence that None / NULL will be treated the same as before.

@codecov-io
Copy link

Codecov Report

Merging #9314 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9314   +/-   ##
=======================================
  Coverage   59.08%   59.08%           
=======================================
  Files         374      374           
  Lines       12205    12205           
  Branches     2989     2989           
=======================================
  Hits         7211     7211           
  Misses       4815     4815           
  Partials      179      179           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1370c5...06eb8ea. Read the comment docs.

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

makes sense to me, thanks for adding the unit test!

I appreciate the quick response to the regression called out in the revert PR

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Makes sense, LGTM. It seems one really has to be careful when applying Pandas transformations.

@etr2460
Copy link
Member

etr2460 commented Mar 17, 2020

@mistercrunch is this good to merge now?

@mistercrunch mistercrunch merged commit 6cf36c9 into apache:master Mar 17, 2020
@mistercrunch mistercrunch deleted the fix_revert_big_number_2 branch March 17, 2020 17:47
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants