Omit Parquet min/max statistics for float/double columns containing NaN

[wjxiz1992]

Description

Closes #22817.

A float/double column containing a NaN mixed with non-NaN values was getting column-chunk min/max statistics computed from the non-NaN values only. The min/max reduce in cpp/src/io/statistics/typed_statistics_chunk.cuh uses cuda::std::min/max over extrema_type::convert(val) (which returns the value unchanged), and since NaN < x / NaN > x are both false a NaN never wins the reduce and is silently dropped — so the chunk ends up with a defined min/max that ignores the NaN. Per the Parquet convention adopted in parquet-mr (PARQUET-1246), a floating-point column that contains any NaN must omit min/max; otherwise a reader doing predicate pushdown (e.g. col = NaN, or a range predicate) uses the bogus bounds to skip the row group and silently drops valid rows. An all-NaN column was safe only incidentally — the merge's has_minmax = (minimum_value <= maximum_value) check flips it off because the reduce leaves the accumulator at inverted identity — but the mixed NaN + non-NaN case passes that check.

Found via NVIDIA/cudf-spark#15004 (a GPU-written file read back by CPU Spark with a = NaN predicate-pushdown filter returned 0 rows instead of the matching NaN row). Affects any float/double leaf, top-level or nested in list/struct/map at any depth.

Fix

Track whether a NaN was seen during the float/double chunk reduce (has_nan), propagate it through block_reduce and the chunk merge, and force has_minmax = false for Parquet when it is set. ORC is unchanged: the guard is if constexpr (IO == PARQUET), and the flag is only carried (never acted on) for ORC.

Added ParquetWriterTest.FloatingPointWithNaNStatsOmitted (float / double / all-NaN must omit min/max; a no-NaN control still writes them). ParquetWriterTest (54 tests) and the *Stats* / *ColumnIndex* suites (45 tests) pass locally.

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR adds has_nan tracking through statistics reduction for floating-point types, switches device-side NaN checks to cuda::std::isnan, uses has_nan in Parquet merge logic to suppress min/max when any NaN is observed, and adds tests covering single-fragment, multi-fragment, and nested-list cases.

Changes

NaN Detection in Parquet Statistics

Layer / File(s)	Summary
Statistics chunk data contract cpp/src/io/statistics/statistics.cuh	statistics_chunk gains has_nan (uint8_t) to record NaN detection; SPDX year updated to 2026.
Device-safe isnan usage cpp/src/io/statistics/statistics_type_identification.cuh	Adds cuda/std headers and changes floating-point isnan checks to cuda::std::isnan for device contexts.
NaN detection in typed statistics reduction cpp/src/io/statistics/typed_statistics_chunk.cuh	Both typed_statistics_chunk specializations add has_nan, set it when cuda::std::isnan(elem) for floating types, merge with |= and __syncthreads_or, and propagate to untyped statistics_chunk in get_untyped_chunk; SPDX year updated to 2026.
Parquet merge handling for NaN-containing stats cpp/src/io/statistics/column_statistics.cuh	merge_group_statistics_functor clears chunk.has_minmax when chunk.has_nan is true to prevent writing min/max for NaN-containing columns.
End-to-end tests for NaN statistics omission cpp/tests/io/parquet_writer_test.cpp	Adds tests FloatingPointWithNaNStatsOmitted, FloatingPointWithNaNStatsOmittedAcrossFragments, and FloatingPointWithNaNStatsOmittedNested validating min/max are omitted for NaN-containing float/double columns.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

bug, 4 - Needs Review, cuIO

Suggested reviewers

• vuule
• mhaseeb123
• abellina

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The only out-of-scope change is an include addition in host_memory.cpp (<rmm/detail/aligned.hpp>), which appears to be an unrelated infrastructure update.	The include in host_memory.cpp is unrelated to NaN statistics handling and should be removed or placed in a separate PR for clarity.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title accurately and concisely describes the main change: omitting Parquet min/max statistics for float/double columns containing NaN, which is the core bug fix.
Description check	✅ Passed	The PR description clearly explains the bug, root cause, fix approach, and includes references to the linked issue and test coverage.
Linked Issues check	✅ Passed	The PR comprehensively addresses issue #22817 by tracking NaN via has_nan flag, propagating it through block_reduce and chunk merge, and forcing has_minmax=false for Parquet when NaN is detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cpp/src/io/statistics/typed_statistics_chunk.cuh`:
- Around line 171-173: The device code in the typed_statistics_chunk
specialization incorrectly calls isnan(elem); update the check to use
cuda::std::isnan(elem) so the device-safe overload is used (replace isnan with
cuda::std::isnan in the block that sets has_nan when elem is NaN), mirroring the
fix applied in the typed_statistics_chunk<T, true> specialization and ensuring
the device compilation uses the CUDA std isnan implementation.

In `@cpp/tests/io/parquet_writer_test.cpp`:
- Around line 856-895: The test FloatingPointWithNaNStatsOmitted currently
checks basic NaN handling but misses nulls, multi-row-group, and
nested-list/struct edge cases; extend the ParquetWriterTest by adding sub-tests
that (1) create a column_wrapper<float> with mixed NaN and null validity (e.g.,
col_with_nulls) and assert get_statistics(...) on its column omits min/max, (2)
write the same data using
parquet_writer_options::builder(...).row_group_size_rows(2) to force multiple
row groups and verify for each fmd.row_groups[i].columns[...] that
get_statistics(...) reports no min/max, and (3) construct a LIST and a STRUCT
column whose leaf float/double child contains NaN values (use make_lists_column
/ struct construction used elsewhere in tests) and assert get_statistics(...) on
the parent columns also omits min/max; reuse read_footer, write_parquet,
get_statistics, and the existing expected/table_view setup to locate where to
add these cases.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 16cec354-bd55-4911-8a64-c27294bff6ed

📥 Commits

Reviewing files that changed from the base of the PR and between 4ae9d07 and 5f7c798.

📒 Files selected for processing (4)

• cpp/src/io/statistics/column_statistics.cuh
• cpp/src/io/statistics/statistics.cuh
• cpp/src/io/statistics/typed_statistics_chunk.cuh
• cpp/tests/io/parquet_writer_test.cpp

[Copilot]

Pull request overview

This PR fixes Parquet writer behavior for floating-point columns that contain NaN values by ensuring min/max statistics are omitted when any NaN is present, matching the Parquet ecosystem convention (PARQUET-1246) and preventing incorrect predicate pushdown row-group skipping.

Changes:

• Track has_nan during per-chunk statistics reduction for floating-point types and propagate it through block reduction and chunk merges.
• For Parquet only, force has_minmax = false when has_nan is set so min/max are not written.
• Add a Parquet writer regression test covering mixed-NaN and all-NaN float/double columns.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
cpp/src/io/statistics/typed_statistics_chunk.cuh	Adds has_nan tracking during element reduction, propagates it through block reduction, and carries it into the untyped statistics_chunk.
cpp/src/io/statistics/statistics.cuh	Extends statistics_chunk with a has_nan flag to persist NaN presence across kernels/merges.
cpp/src/io/statistics/column_statistics.cuh	For Parquet merges, disables min/max emission when any contributing chunk observed a NaN.
cpp/tests/io/parquet_writer_test.cpp	Adds a regression test asserting min/max are omitted for float/double columns containing NaN, while unaffected for no-NaN control.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[wjxiz1992]

/ok to test 35230a3

[wjxiz1992]

/ok to test 37df470

[wjxiz1992]

/ok to merge

[wjxiz1992]

/ok to test

[copy-pr-bot]

/ok to test

@wjxiz1992, there was an error processing your request: E1

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/

[wjxiz1992]

/ok to test 0f21ea3

[wjxiz1992]

/merge