Repository navigation
fix: keep NaN distinct from NULL in float columns when pandas is enabled - #979
Open
maharanay22 wants to merge 1 commit into
Open
maharanay22 wants to merge 1 commit into
maharanay22 wants to merge 1 commit into
Conversation
With pandas enabled (the default), _convert_arrow_table maps float32 and
float64 columns to pandas' nullable Float32Dtype/Float64Dtype. Converting
an Arrow float column to those dtypes turns IEEE NaN into pd.NA, and
to_numpy(na_value=None) then returns None for it, so a NaN value could
not be told apart from SQL NULL. With _disable_pandas=True the same data
already came back as float('nan').
After building the object array, set NaN back on the cells that are NaN
in the Arrow column, using a vectorized pyarrow.compute.is_nan mask.
NULL stays None, other values are unchanged, and tables without float
columns take no extra work.
Signed-off-by: Maha Rana Yadavalli <271375718+maharanay22@users.noreply.github.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Description
Fixes #978.
With pandas enabled (the default),
ResultSet._convert_arrow_tablemaps float32/float64 columns to pandas' nullableFloat32Dtype/Float64Dtype. That conversion turns IEEE NaN intopd.NA, andto_numpy(na_value=None)then returnsNone, so a NaN in a FLOAT or DOUBLE column could not be told apart from SQL NULL. With_disable_pandas=Truethe same data already came back asfloat('nan'). For example,[NaN, NULL, inf, 1.5]read back as[None, None, inf, 1.5]by default and[nan, None, inf, 1.5]with_disable_pandas=True, on both pandas 2.3.3 and 3.0.6.After the object array is built, NaN is set back on the cells that are NaN in the Arrow column, using a vectorized
pyarrow.compute.is_nanmask. NULL staysNoneand other values are unchanged. Tables without float columns do no extra work, and for float columns the cost is one boolean mask per column (about 5 ms per million rows locally, next to roughly 80 ms for the existing pandas conversion).This is a user-visible change: code that treated a NaN result as
Nonewill now seefloat('nan'). Changelog entry added under# Unreleased.How is this tested?
New tests in
tests/unit/test_pandas_compatibility.pycheck exact row values for float32 and float64 columns containing NaN, NULL, inf, -inf and a normal value (split over two chunks, after a non-float column), and check that the default and_disable_pandas=Truepaths return the same rows. Both fail onmainand pass with this change, under pandas 2.3.3 and 3.0.6. Full unit suite: 1022 passed, 5 skipped.Related Tickets & Documents
Fixes #978. Related: #960 edits the same function for nested types. This change sits right after
to_numpyand only uses the table that went through pandas, so either PR rebases onto the other with a one-name change (table_renamedtoscalar_table). Both PRs also add a# Unreleasedchangelog section, so the second one to merge will need a trivial changelog rebase.