[rahulsmahadev]

Rationale for this change

Scans drop NaN rows for predicates that NaN satisfies: IsNaN, NotEqualTo and NotIn. Parquet statistics exclude NaN, so a row group whose non-NaN values are all equal reports min == max. pyarrow prunes that row group for is_nan(x), which is false for the value 7.0, and the scan never returns the NaN row:

# tbl: table with id (long) and x (double)
tbl.append(pa.table({"id": pa.array([0, 1], pa.int64()), "x": pa.array([float("nan"), 7.0], pa.float64())}))
tbl.scan(row_filter=IsNaN("x")).to_arrow()  # 0 rows; expected the row (0, NaN)

The trigger is a float or double row group with no nulls whose non-NaN values are all equal. Nulls in the same row group avoid it.

This change keeps NaN-sensitive predicates away from Parquet pruning. _task_to_record_batches pushes down only the top-level AND conjuncts that cannot hide NaN rows. A conjunct is NaN-sensitive if it is IsNaN, NotEqualTo, or NotIn on a float or double column. When one is present, each batch is filtered with the full expression after the read, as already happens for scans with positional deletes. NOT is left to a follow-up that applies rewrite_not before the Arrow conversion, as the metrics evaluators do. Upstream issue: apache/arrow#51491.

Trade-off: NaN-sensitive conjuncts lose Parquet row-group pruning, and other conjuncts keep it. Java's row-group filter takes the same conservative view: for notEq and notIn it never drops a row group, and for isNaN it drops one only when the column is all null (ParquetMetricsRowGroupFilter).

Are these changes tested?

Yes. test_task_to_record_batches_keeps_nan_rows_for_nan_sensitive_filters in tests/io/test_pyarrow.py covers IsNaN, NotEqualTo, NotIn, AND, OR and NotNaN, each on a float and a double column. test_scan_nan_row_with_equal_finite_statistics in tests/catalog/test_catalog_behaviors.py runs the public scan path on the memory, sql, and sql_without_rowcount catalogs. Both fail without the fix and pass with it.

Are there any user-facing changes?

Bug fix only: scans return the NaN rows they previously dropped. No API changes.

This pull request and its description were written by Isaac.

[rahulsmahadev]

@kevinjqliu @GayathriSrividya could you take a look when you have a chance? It changes which filters _task_to_record_batches pushes down to the Arrow scanner (the path #3448 reworked) so row-group pruning no longer drops NaN rows.

[GayathriSrividya]

@kevinjqliu @GayathriSrividya could you take a look when you have a chance? It changes which filters _task_to_record_batches pushes down to the Arrow scanner (the path #3448 reworked) so row-group pruning no longer drops NaN rows.

Sure, will do check and update

[Fokko]

I'm not sure NOT belongs here. The metrics evaluator applies rewrite_not, so not x < 6.0 is evaluated as x >= 6.0 at the manifest level and a file with upper bound 5.0 is skipped before we get here. With this change, the NaN row comes back for not x < 6.0 only when the file has another row group that survives, and x >= 6.0 never returns it. Could we instead apply rewrite_not to the bound filter before expression_to_pyarrow, in a separate PR? Then NOT is never NaN-sensitive and the negated flag can go.

[rahulsmahadev]

Agreed, removed the NOT handling and the negated flag in 191387d, so NOT is not treated as NaN-sensitive here. I'll put the rewrite_not change before expression_to_pyarrow in a separate PR.

[rahulsmahadev]

Opened #4350 for the rewrite_not change.

[GayathriSrividya]

I agree with Fokko's feedback on the logic and pulling the NOT handling out.

• One additional thing to include when you update the tests for the next iteration: we should add coverage for FloatType (float32).
• The NaN-sensitive predicate detection explicitly handles both FloatType and DoubleType, but the current regression tests in tests/io/test_pyarrow.py (around line 4229 in the test schema) only exercise DoubleType.
• Once Fokko's structural comments are addressed and the float32 tests are in, this looks good to me!

[rahulsmahadev]

@GayathriSrividya thanks, the regression test now runs every case on both a float32 (FloatType) and a float64 (DoubleType) column in 191387d.

[GayathriSrividya]

@GayathriSrividya thanks, the regression test now runs every case on both a float32 (FloatType) and a float64 (DoubleType) column in 191387d.

Thanks, Rahul! The float32 coverage looks good.