From 038cd0a65036c49290da584cdb2ab0864d7f8e1e Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Mon, 17 Aug 2026 21:05:33 +0000 Subject: [PATCH 01/14] Do not complement ordering comparisons on floating point stats columns The negation pushdown deliberately refuses to complement ordering comparisons, because IEEE-754 makes every ordered comparison against a NaN false, so NOT(a < b) is true exactly where a >= b is false. The stats converter still complemented them one layer down: NOT(col < lit) became col >= lit and then vmax >= lit. For a row group holding {NaN, 1.0, 2.0} and lit = 50, the NaN row satisfies NOT(col < 50), yet vmax >= 50 is false and the row group is pruned. cudf files are immune because the writer drops min/max entirely when a NaN is seen (PARQUET-1246), but Arrow writes min/max that merely exclude NaN - pyarrow 23 yields min=1.0, max=2.0 for that chunk - so the hole is live for Arrow-written files. Give the converter the column data types and skip the rewrite for floating point columns, relaxing instead. Equality is unaffected: NaN == x is false and NaN != x is true, so those stay exact complements. Costs pruning only for negated ordering comparisons on float columns, which is what the existing ParquetPredicatePushdownTestAST expectation for NOT(col0 < 100) OR IS_NULL(col0) now records. Reported by @vuule. --- .../parquet/experimental/page_index_filter.cu | 10 +- cpp/src/io/parquet/predicate_pushdown.cpp | 2 +- cpp/src/io/parquet/stats_filter_helpers.cpp | 29 +++-- cpp/src/io/parquet/stats_filter_helpers.hpp | 13 +- cpp/tests/io/parquet_reader_test.cpp | 7 +- .../cudf/tests/input_output/test_parquet.py | 117 ++++++++++++++++++ 6 files changed, 163 insertions(+), 15 deletions(-) diff --git a/cpp/src/io/parquet/experimental/page_index_filter.cu b/cpp/src/io/parquet/experimental/page_index_filter.cu index 7ec4aa859f0..7783896ae7f 100644 --- a/cpp/src/io/parquet/experimental/page_index_filter.cu +++ b/cpp/src/io/parquet/experimental/page_index_filter.cu @@ -562,9 +562,10 @@ struct page_stats_to_row_mask_converter : public page_stats_caster { auto page_stats_table = cudf::table(std::move(columns)); // Converts AST to StatsAST with reference to min, max columns in above `stats_table`. - auto constexpr num_columns = 1; + // The page stats table holds a single column, so the converter sees just this column's type + auto const single_dtype = cudf::host_span{&dtype, 1}; parquet::detail::stats_expression_converter const stats_expr{ - filter.get(), num_columns, has_is_null_operator, stream}; + filter.get(), single_dtype, has_is_null_operator, stream}; // Filter the input table using AST expression and return the (BOOL8) predicate column. auto const page_mask = cudf::detail::compute_column(page_stats_table, @@ -971,7 +972,10 @@ std::unique_ptr aggregate_reader_metadata::build_row_mask_with_pag // Converts AST to StatsAST with reference to min, max columns in above `stats_table`. parquet::detail::stats_expression_converter const stats_expr{ - filter.get(), static_cast(output_dtypes.size()), has_is_null_operator, stream}; + filter.get(), + cudf::host_span{output_dtypes.data(), output_dtypes.size()}, + has_is_null_operator, + stream}; // Filter the input table using AST expression and return the (BOOL8) predicate column. return cudf::detail::compute_column( diff --git a/cpp/src/io/parquet/predicate_pushdown.cpp b/cpp/src/io/parquet/predicate_pushdown.cpp index 44fa83badc7..05d954ee7af 100644 --- a/cpp/src/io/parquet/predicate_pushdown.cpp +++ b/cpp/src/io/parquet/predicate_pushdown.cpp @@ -150,7 +150,7 @@ std::optional>> aggregate_reader_metadata::ap // Converts AST to StatsAST with reference to min, max columns in above `stats_table`. stats_expression_converter const stats_expr{ - filter.get(), static_cast(output_dtypes.size()), has_is_null_operator, stream}; + filter.get(), output_dtypes, has_is_null_operator, stream}; // Filter stats table with StatsAST expression and collect filtered row group indices return collect_filtered_row_group_indices( diff --git a/cpp/src/io/parquet/stats_filter_helpers.cpp b/cpp/src/io/parquet/stats_filter_helpers.cpp index fb5dbd3e1e1..02c318a4d77 100644 --- a/cpp/src/io/parquet/stats_filter_helpers.cpp +++ b/cpp/src/io/parquet/stats_filter_helpers.cpp @@ -89,18 +89,27 @@ std::pair, bool> stats_columns_collector::get_stats_co return {std::move(_columns_mask), _has_is_null_operator}; } -stats_expression_converter::stats_expression_converter(ast::expression const& expr, - size_type num_columns, - bool has_is_null_operator, - cuda::stream_ref stream) - : _always_true_scalar{std::make_unique>(true, true, stream)}, +stats_expression_converter::stats_expression_converter( + ast::expression const& expr, + cudf::host_span output_dtypes, + bool has_is_null_operator, + cuda::stream_ref stream) + : _output_dtypes{output_dtypes}, + _always_true_scalar{std::make_unique>(true, true, stream)}, _always_true{std::make_unique(*_always_true_scalar)} { _stats_cols_per_column = has_is_null_operator ? 3 : 2; - _num_columns = num_columns; + _num_columns = static_cast(output_dtypes.size()); expr.accept(*this); } +bool stats_expression_converter::can_negate_ordering(ast::column_reference const& col_ref) const +{ + auto const col_idx = col_ref.get_column_index(); + if (std::cmp_greater_equal(col_idx, _output_dtypes.size())) { return false; } + return not cudf::is_floating_point(_output_dtypes[col_idx]); +} + std::reference_wrapper stats_expression_converter::visit( ast::operation const& expr) { @@ -158,8 +167,12 @@ std::reference_wrapper stats_expression_converter::visit( auto const rhs_kind = binary_operands.rhs_type; // For NOT(col op lit) negate the operator if negatable and visit the negated operation - // directly - if (lhs_kind == operand_kind::COLUMN_REF and rhs_kind == operand_kind::LITERAL) { + // directly. Equality is always exact, but an ordering comparison is only exact when the + // column cannot hold a `NaN` - see `can_negate_ordering()` + auto const is_equality = + child_op == ast_operator::EQUAL or child_op == ast_operator::NOT_EQUAL; + if (lhs_kind == operand_kind::COLUMN_REF and rhs_kind == operand_kind::LITERAL and + (is_equality or can_negate_ordering(*binary_operands.col_ref))) { auto const negated_op = transform_operator(child_op); if (negated_op.has_value()) { auto const& child_operands = child_operation->get_operands(); diff --git a/cpp/src/io/parquet/stats_filter_helpers.hpp b/cpp/src/io/parquet/stats_filter_helpers.hpp index ed7ac756bd2..acb5f4306c7 100644 --- a/cpp/src/io/parquet/stats_filter_helpers.hpp +++ b/cpp/src/io/parquet/stats_filter_helpers.hpp @@ -358,7 +358,7 @@ class stats_columns_collector : public ast::detail::expression_transformer { class stats_expression_converter : public stats_columns_collector { public: stats_expression_converter(ast::expression const& expr, - size_type num_columns, + cudf::host_span output_dtypes, bool has_is_null_operator, cuda::stream_ref stream); @@ -383,6 +383,17 @@ class stats_expression_converter : public stats_columns_collector { thrust::host_vector get_stats_columns_mask() && = delete; private: + /** + * @brief Whether complementing an *ordering* comparison on this column is exact + * + * IEEE-754 makes every ordered comparison against a `NaN` false, so `NOT(col < v)` is true + * exactly where `col >= v` is false. Rewriting one into the other would prune a row group whose + * `NaN` rows satisfy the predicate, whenever the writer emitted min/max that merely exclude + * `NaN` - which Arrow does. Equality is unaffected and stays exact. + */ + [[nodiscard]] bool can_negate_ordering(ast::column_reference const& col_ref) const; + + cudf::host_span _output_dtypes; ast::tree _stats_expr; cudf::size_type _stats_cols_per_column; std::unique_ptr> _always_true_scalar; diff --git a/cpp/tests/io/parquet_reader_test.cpp b/cpp/tests/io/parquet_reader_test.cpp index 6e55ccbd5ca..ecc9ecfb8bb 100644 --- a/cpp/tests/io/parquet_reader_test.cpp +++ b/cpp/tests/io/parquet_reader_test.cpp @@ -4414,9 +4414,12 @@ void filter_unary_operation_typed_test() filter_expression = cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_OR, not_expr1, expr2); ref_filter = cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_OR, ref_not_expr1, ref_expr2); - // For signed numeric types, RGs 1,2,3 pass. Otherwise, RGs 2,3 pass + // For signed numeric types, RGs 1,2,3 pass. Otherwise, RGs 2,3 pass. + // Floating point columns are the exception: `NOT(col0 < 100)` cannot be rewritten as + // `col0 >= 100` for them, because every ordered comparison against a `NaN` is false, so the + // negated comparison is relaxed and the disjunction keeps every row group auto constexpr expected_filtered_row_groups_with_unary_or = - (cudf::is_numeric() and cudf::is_signed()) ? 3 : 2; + cudf::is_floating_point() ? 4 : ((cudf::is_numeric() and cudf::is_signed()) ? 3 : 2); test_predicate_pushdown(filter_expression, ref_filter, expected_total_row_groups, diff --git a/python/cudf/cudf/tests/input_output/test_parquet.py b/python/cudf/cudf/tests/input_output/test_parquet.py index 6df81526d37..4606bc316cd 100644 --- a/python/cudf/cudf/tests/input_output/test_parquet.py +++ b/python/cudf/cudf/tests/input_output/test_parquet.py @@ -4721,6 +4721,123 @@ def test_parquet_reader_mismatched_nullability_structs(tmp_path): ) +def test_parquet_negated_ordering_with_nan_stats(tmp_path): + """`NOT(col < v)` must not be rewritten to `col >= v` for a float column. + + IEEE-754 makes every ordered comparison against NaN false, so a NaN row satisfies + `NOT(col < v)` while `col >= v` is false for it. Arrow writes min/max that merely + *exclude* NaN (cudf's own writer drops min/max entirely, per PARQUET-1246), so a row + group holding NaN still has usable statistics and would be wrongly pruned. + """ + import pylibcudf as plc + from pylibcudf.expressions import ( + ASTOperator, + ColumnNameReference, + Literal, + Operation, + ) + + # One row group per 3 rows. The first holds NaN alongside small values, so its + # statistics are min=1.0/max=2.0 and `vmax >= 50` is false for it. + values = [float("nan"), 1.0, 2.0] + [100.0, 200.0, 300.0] + path = tmp_path / "nan_ordering.parquet" + pq.write_table(pa.table({"x": values}), path, row_group_size=3) + + # Sanity check the fixture actually reproduces the Arrow statistics behaviour + stats = pq.ParquetFile(path).metadata.row_group(0).column(0).statistics + assert stats.has_min_max and stats.min == 1.0 and stats.max == 2.0 + + col = ColumnNameReference("x") + lit = Literal(plc.Scalar.from_arrow(pa.scalar(50.0))) + filter_expr = Operation( + ASTOperator.NOT, Operation(ASTOperator.LESS, col, lit) + ) + + source = plc.io.SourceInfo([str(path)]) + options = plc.io.parquet.ParquetReaderOptions.builder(source).build() + options.set_filter(filter_expr) + got = plc.io.parquet.read_parquet(options).tbl.to_arrow().column(0).to_pylist() + + # NOT(x < 50) is true for NaN and for 100/200/300, and false for 1.0/2.0 + assert len(got) == 4 + assert math.isnan(got[0]) + assert got[1:] == [100.0, 200.0, 300.0] + + +@pytest.mark.parametrize( + "bloom_filter_fname", + [ + "mixed_card_ndv_100_bf_fpp0.1_nostats.snappy.parquet", + "mixed_card_ndv_500_bf_fpp0.1_nostats.snappy.parquet", + ], +) +def test_parquet_bloom_filter_negated_equality(datadir, bloom_filter_fname): + """Negated equality predicates must prune identically to their rewrites""" + + import pylibcudf as plc + from pylibcudf.expressions import ( + ASTOperator, + ColumnNameReference, + Literal, + Operation, + ) + + fname = datadir / bloom_filter_fname + needle = Literal(plc.Scalar.from_arrow(pa.scalar("FINDME"))) + + def read_with(filter_expr): + source = plc.io.SourceInfo([str(fname)]) + options = plc.io.parquet.ParquetReaderOptions.builder(source).build() + options.set_filter(filter_expr) + return plc.io.parquet.read_parquet(options) + + def assert_equivalent(lhs, rhs): + lhs_result = read_with(lhs) + rhs_result = read_with(rhs) + assert_eq( + lhs_result.num_row_groups_after_bloom_filter, + rhs_result.num_row_groups_after_bloom_filter, + ) + assert_arrow_table_equal( + lhs_result.tbl.to_arrow(), rhs_result.tbl.to_arrow() + ) + return lhs_result + + str_col = ColumnNameReference("str") + str_eq = Operation(ASTOperator.EQUAL, str_col, needle) + str_ne = Operation(ASTOperator.NOT_EQUAL, str_col, needle) + + negated_equality = assert_equivalent( + Operation(ASTOperator.NOT, str_eq), + str_ne, + ) + # 998 of the 1000 rows are not "FINDME". + assert_eq(negated_equality.tbl.num_rows(), 998) + + assert_equivalent( + Operation(ASTOperator.NOT, str_ne), + str_eq, + ) + + fp64_col = ColumnNameReference("fp64") + fp64_needle = Literal(plc.Scalar.from_arrow(pa.scalar(500.0))) + assert_equivalent( + Operation( + ASTOperator.NOT, + Operation( + ASTOperator.LOGICAL_AND, + str_ne, + Operation(ASTOperator.NOT_EQUAL, fp64_col, fp64_needle), + ), + ), + Operation( + ASTOperator.LOGICAL_OR, + str_eq, + Operation(ASTOperator.EQUAL, fp64_col, fp64_needle), + ), + ) + + @pytest.mark.skipif( pa.__version__ == "19.0.0", reason="https://github.com/apache/arrow/issues/45283, https://github.com/NVIDIA/cudf/issues/17806", From 56e8bd3418a87a7cdd989e548dc7a9b430f94924 Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Thu, 20 Aug 2026 00:23:52 +0000 Subject: [PATCH 02/14] Do not prune floating point chunks on col != val either The guard added for negated ordering comparisons was not sufficient. The NOT_EQUAL leaf is unsound for the same reason and involves no negation at all: col != val --> vmin != vmax OR vmax != val A chunk of {NaN, val} reports min == max == val, because Arrow and parquet-mr both skip NaN when updating min/max, and is therefore indistinguishable from a constant-val chunk. The transform prunes exactly that shape - but NaN != val is true, so its NaN rows do satisfy the filter and are dropped. This also reaches NOT(col == val), which the normalizer complements into col != val. Reproduced on a two row group pyarrow file [NaN, 5.0 | 7.0, 8.0] filtered by x != 5.0: the first row group is pruned and only [7.0, 8.0] comes back. Relax the leaf for floating point columns. The reader cannot be more precise: the Parquet Statistics struct carries null_count, distinct_count and the min/max exactness flags, but nothing about NaN, so a NaN-free chunk is indistinguishable from one whose NaN was skipped. Costs pruning for col != val on float columns only. The other leaves stay sound because NaN never satisfies them: col < v, col > v and col == v are all false for NaN, so excluding it from min/max cannot make them prune a matching row. The unsound cases are exactly the predicates NaN satisfies. parquet-mr's DoubleStatistics.updateStats behaving like Arrow here was confirmed by Paul Mattione, widening this from Arrow-written files to Spark-written ones as well. --- cpp/src/io/parquet/stats_filter_helpers.cpp | 17 ++++++-- cpp/src/io/parquet/stats_filter_helpers.hpp | 11 +++++ .../cudf/tests/input_output/test_parquet.py | 43 +++++++++++++++++++ 3 files changed, 68 insertions(+), 3 deletions(-) diff --git a/cpp/src/io/parquet/stats_filter_helpers.cpp b/cpp/src/io/parquet/stats_filter_helpers.cpp index 02c318a4d77..9d9a7cf5871 100644 --- a/cpp/src/io/parquet/stats_filter_helpers.cpp +++ b/cpp/src/io/parquet/stats_filter_helpers.cpp @@ -103,13 +103,18 @@ stats_expression_converter::stats_expression_converter( expr.accept(*this); } +bool stats_expression_converter::is_floating_point_column(size_type col_index) const +{ + return std::cmp_less(col_index, _output_dtypes.size()) and + cudf::is_floating_point(_output_dtypes[col_index]); +} + bool stats_expression_converter::can_negate_ordering(ast::column_reference const& col_ref) const { - auto const col_idx = col_ref.get_column_index(); - if (std::cmp_greater_equal(col_idx, _output_dtypes.size())) { return false; } - return not cudf::is_floating_point(_output_dtypes[col_idx]); + return not is_floating_point_column(col_ref.get_column_index()); } + std::reference_wrapper stats_expression_converter::visit( ast::operation const& expr) { @@ -223,6 +228,12 @@ std::reference_wrapper stats_expression_converter::visit( break; } case ast_operator::NOT_EQUAL: { + // NaNs satisfy `col != val` but Arrow and parquet-mr exclude them from min/max, so + // `{NaN, val}` appears constant and must not be pruned. + if (is_floating_point_column(col_index)) { + _stats_expr.push(ast::operation{ast_operator::IDENTITY, *_always_true}); + return *_always_true; + } auto const& vmin = _stats_expr.push(ast::column_reference{col_index * _stats_cols_per_column}); auto const& vmax = diff --git a/cpp/src/io/parquet/stats_filter_helpers.hpp b/cpp/src/io/parquet/stats_filter_helpers.hpp index acb5f4306c7..339e8109074 100644 --- a/cpp/src/io/parquet/stats_filter_helpers.hpp +++ b/cpp/src/io/parquet/stats_filter_helpers.hpp @@ -383,6 +383,17 @@ class stats_expression_converter : public stats_columns_collector { thrust::host_vector get_stats_columns_mask() && = delete; private: + /** + * @brief Whether this column's statistics may hide a `NaN` + * + * Arrow and parquet-mr both skip `NaN` when updating min/max, so a floating point chunk that + * contains one still reports usable statistics computed from the remaining values. Any stats + * transform whose predicate is *satisfied* by `NaN` is therefore unsound for such a column. + * cudf's own writer drops min/max entirely instead (PARQUET-1246), but the reader cannot assume + * it produced the file. + */ + [[nodiscard]] bool is_floating_point_column(size_type col_index) const; + /** * @brief Whether complementing an *ordering* comparison on this column is exact * diff --git a/python/cudf/cudf/tests/input_output/test_parquet.py b/python/cudf/cudf/tests/input_output/test_parquet.py index 4606bc316cd..ad4743d3730 100644 --- a/python/cudf/cudf/tests/input_output/test_parquet.py +++ b/python/cudf/cudf/tests/input_output/test_parquet.py @@ -4721,6 +4721,49 @@ def test_parquet_reader_mismatched_nullability_structs(tmp_path): ) +def test_parquet_not_equal_with_nan_stats(tmp_path): + """`col != v` must not prune a row group whose NaN rows satisfy it. + + Arrow and parquet-mr both skip NaN when updating min/max, so a chunk of {NaN, v} + reports min == max == v and is indistinguishable from a constant-v chunk. The + `col != val` stats transform prunes exactly that shape, dropping the NaN rows - + which do satisfy `col != val`, since NaN != v is true. + """ + import pylibcudf as plc + from pylibcudf.expressions import ( + ASTOperator, + ColumnNameReference, + Literal, + Operation, + ) + + path = tmp_path / "nan_not_equal.parquet" + pq.write_table( + pa.table({"x": [float("nan"), 5.0, 7.0, 8.0]}), path, row_group_size=2 + ) + + # Sanity check the fixture: NaN is excluded, so row group 0 looks constant + stats = pq.ParquetFile(path).metadata.row_group(0).column(0).statistics + assert stats.min == 5.0 and stats.max == 5.0 + + scalar = plc.Scalar.from_arrow(pa.scalar(5.0)) + filter_expr = Operation( + ASTOperator.NOT_EQUAL, ColumnNameReference("x"), Literal(scalar) + ) + + source = plc.io.SourceInfo([str(path)]) + options = plc.io.parquet.ParquetReaderOptions.builder(source).build() + options.set_filter(filter_expr) + result = plc.io.parquet.read_parquet(options) + + # Neither row group may be pruned: rg0 holds a NaN, rg1 holds 7.0 and 8.0 + assert result.num_row_groups_after_stats_filter == 2 + got = result.tbl.to_arrow().column(0).to_pylist() + assert len(got) == 3 + assert math.isnan(got[0]) + assert got[1:] == [7.0, 8.0] + + def test_parquet_negated_ordering_with_nan_stats(tmp_path): """`NOT(col < v)` must not be rewritten to `col >= v` for a float column. From 340f072a2c759daa5b2a83dbb9bbf8ab524951ba Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Thu, 20 Aug 2026 01:14:29 +0000 Subject: [PATCH 03/14] Simplify --- .../parquet/experimental/page_index_filter.cu | 8 ++--- cpp/src/io/parquet/stats_filter_helpers.cpp | 30 ++++++++----------- cpp/src/io/parquet/stats_filter_helpers.hpp | 25 ++-------------- 3 files changed, 16 insertions(+), 47 deletions(-) diff --git a/cpp/src/io/parquet/experimental/page_index_filter.cu b/cpp/src/io/parquet/experimental/page_index_filter.cu index 7783896ae7f..ee0e94bcc31 100644 --- a/cpp/src/io/parquet/experimental/page_index_filter.cu +++ b/cpp/src/io/parquet/experimental/page_index_filter.cu @@ -563,9 +563,8 @@ struct page_stats_to_row_mask_converter : public page_stats_caster { auto page_stats_table = cudf::table(std::move(columns)); // Converts AST to StatsAST with reference to min, max columns in above `stats_table`. // The page stats table holds a single column, so the converter sees just this column's type - auto const single_dtype = cudf::host_span{&dtype, 1}; parquet::detail::stats_expression_converter const stats_expr{ - filter.get(), single_dtype, has_is_null_operator, stream}; + filter.get(), std::span{&dtype, 1}, has_is_null_operator, stream}; // Filter the input table using AST expression and return the (BOOL8) predicate column. auto const page_mask = cudf::detail::compute_column(page_stats_table, @@ -972,10 +971,7 @@ std::unique_ptr aggregate_reader_metadata::build_row_mask_with_pag // Converts AST to StatsAST with reference to min, max columns in above `stats_table`. parquet::detail::stats_expression_converter const stats_expr{ - filter.get(), - cudf::host_span{output_dtypes.data(), output_dtypes.size()}, - has_is_null_operator, - stream}; + filter.get(), output_dtypes, has_is_null_operator, stream}; // Filter the input table using AST expression and return the (BOOL8) predicate column. return cudf::detail::compute_column( diff --git a/cpp/src/io/parquet/stats_filter_helpers.cpp b/cpp/src/io/parquet/stats_filter_helpers.cpp index 9d9a7cf5871..dc1afb4a010 100644 --- a/cpp/src/io/parquet/stats_filter_helpers.cpp +++ b/cpp/src/io/parquet/stats_filter_helpers.cpp @@ -91,7 +91,7 @@ std::pair, bool> stats_columns_collector::get_stats_co stats_expression_converter::stats_expression_converter( ast::expression const& expr, - cudf::host_span output_dtypes, + std::span output_dtypes, bool has_is_null_operator, cuda::stream_ref stream) : _output_dtypes{output_dtypes}, @@ -103,18 +103,6 @@ stats_expression_converter::stats_expression_converter( expr.accept(*this); } -bool stats_expression_converter::is_floating_point_column(size_type col_index) const -{ - return std::cmp_less(col_index, _output_dtypes.size()) and - cudf::is_floating_point(_output_dtypes[col_index]); -} - -bool stats_expression_converter::can_negate_ordering(ast::column_reference const& col_ref) const -{ - return not is_floating_point_column(col_ref.get_column_index()); -} - - std::reference_wrapper stats_expression_converter::visit( ast::operation const& expr) { @@ -167,17 +155,23 @@ std::reference_wrapper stats_expression_converter::visit( } } // Binary operation wrapped else if (cudf::ast::detail::ast_operator_arity(child_op) == 2) { + // For NOT(col op lit) negate the operator if negatable and visit the negated operation + // directly. auto const binary_operands = extract_binary_operands(*child_operation); auto const lhs_kind = binary_operands.lhs_type; auto const rhs_kind = binary_operands.rhs_type; - // For NOT(col op lit) negate the operator if negatable and visit the negated operation - // directly. Equality is always exact, but an ordering comparison is only exact when the - // column cannot hold a `NaN` - see `can_negate_ordering()` + // Equality is always exact auto const is_equality = child_op == ast_operator::EQUAL or child_op == ast_operator::NOT_EQUAL; + + // An ordering comparison is only exact when the column cannot hold a `NaN` (aka not a + // floating point column) + auto const can_negate_ordering = not cudf::is_floating_point( + _output_dtypes[binary_operands.col_ref->get_column_index()]); + if (lhs_kind == operand_kind::COLUMN_REF and rhs_kind == operand_kind::LITERAL and - (is_equality or can_negate_ordering(*binary_operands.col_ref))) { + (is_equality or can_negate_ordering)) { auto const negated_op = transform_operator(child_op); if (negated_op.has_value()) { auto const& child_operands = child_operation->get_operands(); @@ -230,7 +224,7 @@ std::reference_wrapper stats_expression_converter::visit( case ast_operator::NOT_EQUAL: { // NaNs satisfy `col != val` but Arrow and parquet-mr exclude them from min/max, so // `{NaN, val}` appears constant and must not be pruned. - if (is_floating_point_column(col_index)) { + if (cudf::is_floating_point(_output_dtypes[col_index])) { _stats_expr.push(ast::operation{ast_operator::IDENTITY, *_always_true}); return *_always_true; } diff --git a/cpp/src/io/parquet/stats_filter_helpers.hpp b/cpp/src/io/parquet/stats_filter_helpers.hpp index 339e8109074..0d1f0ab371b 100644 --- a/cpp/src/io/parquet/stats_filter_helpers.hpp +++ b/cpp/src/io/parquet/stats_filter_helpers.hpp @@ -358,7 +358,7 @@ class stats_columns_collector : public ast::detail::expression_transformer { class stats_expression_converter : public stats_columns_collector { public: stats_expression_converter(ast::expression const& expr, - cudf::host_span output_dtypes, + std::span output_dtypes, bool has_is_null_operator, cuda::stream_ref stream); @@ -383,28 +383,7 @@ class stats_expression_converter : public stats_columns_collector { thrust::host_vector get_stats_columns_mask() && = delete; private: - /** - * @brief Whether this column's statistics may hide a `NaN` - * - * Arrow and parquet-mr both skip `NaN` when updating min/max, so a floating point chunk that - * contains one still reports usable statistics computed from the remaining values. Any stats - * transform whose predicate is *satisfied* by `NaN` is therefore unsound for such a column. - * cudf's own writer drops min/max entirely instead (PARQUET-1246), but the reader cannot assume - * it produced the file. - */ - [[nodiscard]] bool is_floating_point_column(size_type col_index) const; - - /** - * @brief Whether complementing an *ordering* comparison on this column is exact - * - * IEEE-754 makes every ordered comparison against a `NaN` false, so `NOT(col < v)` is true - * exactly where `col >= v` is false. Rewriting one into the other would prune a row group whose - * `NaN` rows satisfy the predicate, whenever the writer emitted min/max that merely exclude - * `NaN` - which Arrow does. Equality is unaffected and stays exact. - */ - [[nodiscard]] bool can_negate_ordering(ast::column_reference const& col_ref) const; - - cudf::host_span _output_dtypes; + std::span _output_dtypes; ast::tree _stats_expr; cudf::size_type _stats_cols_per_column; std::unique_ptr> _always_true_scalar; From 8d83b2f2eaff1f1bc210e3639f43671aa4a96a99 Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Thu, 20 Aug 2026 01:21:44 +0000 Subject: [PATCH 04/14] Keep the NaN check behind the col-op-lit guard Inlining can_negate_ordering() hoisted the column lookup out of the short-circuit that protected it. extract_binary_operands() only reports a column reference for the `col op lit` and `lit op col` forms; for anything else it returns nullptr, so the unconditional _output_dtypes[binary_operands.col_ref->get_column_index()] dereferences null for any NOT wrapping a comparison neither of whose operands is a bare column, such as NOT((col + 1) > 5). The read is now nested inside the `col op lit` check rather than sitting in the condition alongside it, which is what the short-circuit was doing before. Behaviour is otherwise unchanged. Reproduced with the filter (col_a < 150) AND NOT((col_a + 10) > 50); the first conjunct is what makes a column stats-usable, so the converter is built at all. Covered by ParquetReaderTest.FilterNegationPushdown, which segfaults without this. --- cpp/src/io/parquet/stats_filter_helpers.cpp | 35 +++++++++++---------- cpp/tests/io/parquet_reader_test.cpp | 13 ++++++++ 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/cpp/src/io/parquet/stats_filter_helpers.cpp b/cpp/src/io/parquet/stats_filter_helpers.cpp index dc1afb4a010..4c3b615a227 100644 --- a/cpp/src/io/parquet/stats_filter_helpers.cpp +++ b/cpp/src/io/parquet/stats_filter_helpers.cpp @@ -161,22 +161,25 @@ std::reference_wrapper stats_expression_converter::visit( auto const lhs_kind = binary_operands.lhs_type; auto const rhs_kind = binary_operands.rhs_type; - // Equality is always exact - auto const is_equality = - child_op == ast_operator::EQUAL or child_op == ast_operator::NOT_EQUAL; - - // An ordering comparison is only exact when the column cannot hold a `NaN` (aka not a - // floating point column) - auto const can_negate_ordering = not cudf::is_floating_point( - _output_dtypes[binary_operands.col_ref->get_column_index()]); - - if (lhs_kind == operand_kind::COLUMN_REF and rhs_kind == operand_kind::LITERAL and - (is_equality or can_negate_ordering)) { - auto const negated_op = transform_operator(child_op); - if (negated_op.has_value()) { - auto const& child_operands = child_operation->get_operands(); - return visit( - ast::operation{*negated_op, child_operands.front(), child_operands.back()}); + // `col_ref` is only non-null for the `col op lit` form, so both checks below must + // stay inside this branch + if (lhs_kind == operand_kind::COLUMN_REF and rhs_kind == operand_kind::LITERAL) { + // Equality is always exact + auto const is_equality = + child_op == ast_operator::EQUAL or child_op == ast_operator::NOT_EQUAL; + + // An ordering comparison is only exact when the column cannot hold a `NaN` (aka not + // a floating point column) + auto const can_negate_ordering = not cudf::is_floating_point( + _output_dtypes[binary_operands.col_ref->get_column_index()]); + + if (is_equality or can_negate_ordering) { + auto const negated_op = transform_operator(child_op); + if (negated_op.has_value()) { + auto const& child_operands = child_operation->get_operands(); + return visit( + ast::operation{*negated_op, child_operands.front(), child_operands.back()}); + } } } } diff --git a/cpp/tests/io/parquet_reader_test.cpp b/cpp/tests/io/parquet_reader_test.cpp index ecc9ecfb8bb..404c6440a68 100644 --- a/cpp/tests/io/parquet_reader_test.cpp +++ b/cpp/tests/io/parquet_reader_test.cpp @@ -2422,6 +2422,19 @@ TEST_F(ParquetReaderTest, FilterNegationPushdown) expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, not_lt), 1); } + // NOT over a comparison whose operands are neither `col op lit` nor `lit op col`. The stats + // converter inspects the wrapped comparison to decide whether it can complement the operator, + // and `extract_binary_operands()` reports a null column reference for this shape, so that + // decision has to stay behind the `col op lit` check. The `col_a < 150` conjunct makes a column + // stats-usable, so the stats converter is actually built for this filter. + { + auto sum = cudf::ast::operation(cudf::ast::ast_operator::ADD, col_ref_a, lit_10); + auto sum_gt_50 = cudf::ast::operation(cudf::ast::ast_operator::GREATER, sum, lit_50); + auto not_sum = cudf::ast::operation(cudf::ast::ast_operator::NOT, sum_gt_50); + expect_matches_unrewritten( + cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_AND, a_lt_150, not_sum)); + } + // Double negation over a non-boolean operand must NOT be eliminated. { auto not_a = cudf::ast::operation(cudf::ast::ast_operator::NOT, col_ref_a); From f108ead42aba7b103cc4225fa1b6f218792196b3 Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Fri, 21 Aug 2026 17:59:37 +0000 Subject: [PATCH 05/14] Address review nits: name the transform, make the mode switch explicit Two follow-ups from Lawrence's review of #23580. Call the rewrite by its name. "Negation normal form" is the standard term in mathematical logic for an expression whose negations appear only on atoms, reached by eliminating double negations and applying De Morgan's laws, which is exactly what the normalizer produces. Both class docs now say so. Make transform_operator's mode dispatch exhaustive, so that adding a slot to operator_transform and calling with it fails to compile rather than silently taking the NEGATE branch. The static_assert condition mentions `mode` deliberately: cudf builds as C++20, where a bare static_assert(false) in a discarded if-constexpr branch is ill-formed and fires unconditionally. Keeping the condition value-dependent defers it to instantiation, which is what makes the check fire only for an unhandled mode. --- cpp/src/io/parquet/experimental/hybrid_scan_helpers.hpp | 4 ++-- cpp/src/io/parquet/expression_transform_helpers.cpp | 8 ++++++-- cpp/src/io/parquet/expression_transform_helpers.hpp | 7 +++++-- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/cpp/src/io/parquet/experimental/hybrid_scan_helpers.hpp b/cpp/src/io/parquet/experimental/hybrid_scan_helpers.hpp index 59591d43891..d408f3086a6 100644 --- a/cpp/src/io/parquet/experimental/hybrid_scan_helpers.hpp +++ b/cpp/src/io/parquet/experimental/hybrid_scan_helpers.hpp @@ -400,8 +400,8 @@ class dictionary_literals_collector : public equality_literals_collector { }; /** - * @brief Converts named columns to index reference columns and pushes logical negations down to - * expression leaves + * @brief Converts named columns to index reference columns and rewrites the expression into + * negation normal form, pushing logical negations down to the leaves */ class parquet_filter_normalizer : public parquet::detail::parquet_filter_normalizer { public: diff --git a/cpp/src/io/parquet/expression_transform_helpers.cpp b/cpp/src/io/parquet/expression_transform_helpers.cpp index bec42f2905a..63e9910c979 100644 --- a/cpp/src/io/parquet/expression_transform_helpers.cpp +++ b/cpp/src/io/parquet/expression_transform_helpers.cpp @@ -58,8 +58,7 @@ std::optional transform_operator(ast::ast_operator op) case ast::ast_operator::GREATER_EQUAL: return ast::ast_operator::LESS_EQUAL; default: return std::make_optional(op); } - } else { - // mode == NEGATE + } else if constexpr (mode == operator_transform::NEGATE) { switch (op) { case ast::ast_operator::LESS: return ast::ast_operator::GREATER_EQUAL; case ast::ast_operator::GREATER: return ast::ast_operator::LESS_EQUAL; @@ -69,6 +68,11 @@ std::optional transform_operator(ast::ast_operator op) case ast::ast_operator::NOT_EQUAL: return ast::ast_operator::EQUAL; default: return std::nullopt; } + } else { + // Condition mentions `mode` so that it is only checked once the branch is instantiated, + // which C++20 requires - a bare `static_assert(false)` here would fire unconditionally + static_assert(mode == operator_transform::INVERT or mode == operator_transform::NEGATE, + "Unhandled operator transform"); } } diff --git a/cpp/src/io/parquet/expression_transform_helpers.hpp b/cpp/src/io/parquet/expression_transform_helpers.hpp index f509e979b32..12c072263e4 100644 --- a/cpp/src/io/parquet/expression_transform_helpers.hpp +++ b/cpp/src/io/parquet/expression_transform_helpers.hpp @@ -157,8 +157,11 @@ class names_from_expression : public ast::detail::expression_transformer { }; /** - * @brief Converts named columns to index reference columns and pushes logical negations down to the - * leaves of the expression. + * @brief Converts named columns to index reference columns and rewrites the expression into + * negation normal form, pushing logical negations down to the leaves + * + * Negation normal form is the standard term for an expression in which negation appears only on + * atoms, reached by eliminating double negations and applying De Morgan's laws. */ class parquet_filter_normalizer : public ast::detail::expression_transformer { public: From e660abbd7943ebf12c3561c7c8fb80aa65a11adb Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Fri, 21 Aug 2026 19:02:31 +0000 Subject: [PATCH 06/14] Clean up --- .../io/parquet/experimental/page_index_filter.cu | 1 - cpp/src/io/parquet/expression_transform_helpers.cpp | 11 +++++------ cpp/src/io/parquet/expression_transform_helpers.hpp | 5 +---- cpp/src/io/parquet/stats_filter_helpers.cpp | 8 ++++---- cpp/tests/io/parquet_reader_test.cpp | 13 ++++--------- 5 files changed, 14 insertions(+), 24 deletions(-) diff --git a/cpp/src/io/parquet/experimental/page_index_filter.cu b/cpp/src/io/parquet/experimental/page_index_filter.cu index ee0e94bcc31..2ef90e9a94a 100644 --- a/cpp/src/io/parquet/experimental/page_index_filter.cu +++ b/cpp/src/io/parquet/experimental/page_index_filter.cu @@ -562,7 +562,6 @@ struct page_stats_to_row_mask_converter : public page_stats_caster { auto page_stats_table = cudf::table(std::move(columns)); // Converts AST to StatsAST with reference to min, max columns in above `stats_table`. - // The page stats table holds a single column, so the converter sees just this column's type parquet::detail::stats_expression_converter const stats_expr{ filter.get(), std::span{&dtype, 1}, has_is_null_operator, stream}; diff --git a/cpp/src/io/parquet/expression_transform_helpers.cpp b/cpp/src/io/parquet/expression_transform_helpers.cpp index 63e9910c979..fe4014a4e48 100644 --- a/cpp/src/io/parquet/expression_transform_helpers.cpp +++ b/cpp/src/io/parquet/expression_transform_helpers.cpp @@ -50,6 +50,9 @@ namespace { template std::optional transform_operator(ast::ast_operator op) { + static_assert(mode == operator_transform::INVERT or mode == operator_transform::NEGATE, + "Unhandled operator transform"); + if constexpr (mode == operator_transform::INVERT) { switch (op) { case ast::ast_operator::LESS: return ast::ast_operator::GREATER; @@ -58,7 +61,8 @@ std::optional transform_operator(ast::ast_operator op) case ast::ast_operator::GREATER_EQUAL: return ast::ast_operator::LESS_EQUAL; default: return std::make_optional(op); } - } else if constexpr (mode == operator_transform::NEGATE) { + } else { + // mode == NEGATE switch (op) { case ast::ast_operator::LESS: return ast::ast_operator::GREATER_EQUAL; case ast::ast_operator::GREATER: return ast::ast_operator::LESS_EQUAL; @@ -68,11 +72,6 @@ std::optional transform_operator(ast::ast_operator op) case ast::ast_operator::NOT_EQUAL: return ast::ast_operator::EQUAL; default: return std::nullopt; } - } else { - // Condition mentions `mode` so that it is only checked once the branch is instantiated, - // which C++20 requires - a bare `static_assert(false)` here would fire unconditionally - static_assert(mode == operator_transform::INVERT or mode == operator_transform::NEGATE, - "Unhandled operator transform"); } } diff --git a/cpp/src/io/parquet/expression_transform_helpers.hpp b/cpp/src/io/parquet/expression_transform_helpers.hpp index 12c072263e4..b17734aa445 100644 --- a/cpp/src/io/parquet/expression_transform_helpers.hpp +++ b/cpp/src/io/parquet/expression_transform_helpers.hpp @@ -158,10 +158,7 @@ class names_from_expression : public ast::detail::expression_transformer { /** * @brief Converts named columns to index reference columns and rewrites the expression into - * negation normal form, pushing logical negations down to the leaves - * - * Negation normal form is the standard term for an expression in which negation appears only on - * atoms, reached by eliminating double negations and applying De Morgan's laws. + * negation normal form, pushing logical negations down to the leaves. */ class parquet_filter_normalizer : public ast::detail::expression_transformer { public: diff --git a/cpp/src/io/parquet/stats_filter_helpers.cpp b/cpp/src/io/parquet/stats_filter_helpers.cpp index 4c3b615a227..83c5f11ea8f 100644 --- a/cpp/src/io/parquet/stats_filter_helpers.cpp +++ b/cpp/src/io/parquet/stats_filter_helpers.cpp @@ -168,8 +168,8 @@ std::reference_wrapper stats_expression_converter::visit( auto const is_equality = child_op == ast_operator::EQUAL or child_op == ast_operator::NOT_EQUAL; - // An ordering comparison is only exact when the column cannot hold a `NaN` (aka not - // a floating point column) + // An ordering comparison is only exact when the column cannot hold a `NaN`. i.e., not + // a floating point type auto const can_negate_ordering = not cudf::is_floating_point( _output_dtypes[binary_operands.col_ref->get_column_index()]); @@ -225,8 +225,8 @@ std::reference_wrapper stats_expression_converter::visit( break; } case ast_operator::NOT_EQUAL: { - // NaNs satisfy `col != val` but Arrow and parquet-mr exclude them from min/max, so - // `{NaN, val}` appears constant and must not be pruned. + // Some Parquet writers exclude `NaN`s from stats so we can't reliably prune row groups for + // columns that may contain them. if (cudf::is_floating_point(_output_dtypes[col_index])) { _stats_expr.push(ast::operation{ast_operator::IDENTITY, *_always_true}); return *_always_true; diff --git a/cpp/tests/io/parquet_reader_test.cpp b/cpp/tests/io/parquet_reader_test.cpp index 404c6440a68..7f827dec56a 100644 --- a/cpp/tests/io/parquet_reader_test.cpp +++ b/cpp/tests/io/parquet_reader_test.cpp @@ -2422,11 +2422,8 @@ TEST_F(ParquetReaderTest, FilterNegationPushdown) expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, not_lt), 1); } - // NOT over a comparison whose operands are neither `col op lit` nor `lit op col`. The stats - // converter inspects the wrapped comparison to decide whether it can complement the operator, - // and `extract_binary_operands()` reports a null column reference for this shape, so that - // decision has to stay behind the `col op lit` check. The `col_a < 150` conjunct makes a column - // stats-usable, so the stats converter is actually built for this filter. + // NOT(col_a + 10 > 50) - operand is not `col op lit`, so it must NOT be complemented. The + // `col_a < 150` conjunct keeps the filter stats-usable. { auto sum = cudf::ast::operation(cudf::ast::ast_operator::ADD, col_ref_a, lit_10); auto sum_gt_50 = cudf::ast::operation(cudf::ast::ast_operator::GREATER, sum, lit_50); @@ -4427,10 +4424,8 @@ void filter_unary_operation_typed_test() filter_expression = cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_OR, not_expr1, expr2); ref_filter = cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_OR, ref_not_expr1, ref_expr2); - // For signed numeric types, RGs 1,2,3 pass. Otherwise, RGs 2,3 pass. - // Floating point columns are the exception: `NOT(col0 < 100)` cannot be rewritten as - // `col0 >= 100` for them, because every ordered comparison against a `NaN` is false, so the - // negated comparison is relaxed and the disjunction keeps every row group + // Signed numeric types pass RGs 1,2,3, others pass RGs 2,3. Floats keep all 4: NaN makes + // every ordered comparison false, so `NOT(col0 < 100)` is not `col0 >= 100` and gets relaxed. auto constexpr expected_filtered_row_groups_with_unary_or = cudf::is_floating_point() ? 4 : ((cudf::is_numeric() and cudf::is_signed()) ? 3 : 2); test_predicate_pushdown(filter_expression, From c8f2f7aa889c1cd9185b8fc14d7dd0bf987b1c44 Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Fri, 21 Aug 2026 19:36:41 +0000 Subject: [PATCH 07/14] Clean up pytests --- .../cudf/tests/input_output/test_parquet.py | 76 +------------------ 1 file changed, 1 insertion(+), 75 deletions(-) diff --git a/python/cudf/cudf/tests/input_output/test_parquet.py b/python/cudf/cudf/tests/input_output/test_parquet.py index ad4743d3730..eb6f44802fd 100644 --- a/python/cudf/cudf/tests/input_output/test_parquet.py +++ b/python/cudf/cudf/tests/input_output/test_parquet.py @@ -4782,7 +4782,7 @@ def test_parquet_negated_ordering_with_nan_stats(tmp_path): # One row group per 3 rows. The first holds NaN alongside small values, so its # statistics are min=1.0/max=2.0 and `vmax >= 50` is false for it. - values = [float("nan"), 1.0, 2.0] + [100.0, 200.0, 300.0] + values = [float("nan"), 1.0, 2.0, 100.0, 200.0, 300.0] path = tmp_path / "nan_ordering.parquet" pq.write_table(pa.table({"x": values}), path, row_group_size=3) @@ -4807,80 +4807,6 @@ def test_parquet_negated_ordering_with_nan_stats(tmp_path): assert got[1:] == [100.0, 200.0, 300.0] -@pytest.mark.parametrize( - "bloom_filter_fname", - [ - "mixed_card_ndv_100_bf_fpp0.1_nostats.snappy.parquet", - "mixed_card_ndv_500_bf_fpp0.1_nostats.snappy.parquet", - ], -) -def test_parquet_bloom_filter_negated_equality(datadir, bloom_filter_fname): - """Negated equality predicates must prune identically to their rewrites""" - - import pylibcudf as plc - from pylibcudf.expressions import ( - ASTOperator, - ColumnNameReference, - Literal, - Operation, - ) - - fname = datadir / bloom_filter_fname - needle = Literal(plc.Scalar.from_arrow(pa.scalar("FINDME"))) - - def read_with(filter_expr): - source = plc.io.SourceInfo([str(fname)]) - options = plc.io.parquet.ParquetReaderOptions.builder(source).build() - options.set_filter(filter_expr) - return plc.io.parquet.read_parquet(options) - - def assert_equivalent(lhs, rhs): - lhs_result = read_with(lhs) - rhs_result = read_with(rhs) - assert_eq( - lhs_result.num_row_groups_after_bloom_filter, - rhs_result.num_row_groups_after_bloom_filter, - ) - assert_arrow_table_equal( - lhs_result.tbl.to_arrow(), rhs_result.tbl.to_arrow() - ) - return lhs_result - - str_col = ColumnNameReference("str") - str_eq = Operation(ASTOperator.EQUAL, str_col, needle) - str_ne = Operation(ASTOperator.NOT_EQUAL, str_col, needle) - - negated_equality = assert_equivalent( - Operation(ASTOperator.NOT, str_eq), - str_ne, - ) - # 998 of the 1000 rows are not "FINDME". - assert_eq(negated_equality.tbl.num_rows(), 998) - - assert_equivalent( - Operation(ASTOperator.NOT, str_ne), - str_eq, - ) - - fp64_col = ColumnNameReference("fp64") - fp64_needle = Literal(plc.Scalar.from_arrow(pa.scalar(500.0))) - assert_equivalent( - Operation( - ASTOperator.NOT, - Operation( - ASTOperator.LOGICAL_AND, - str_ne, - Operation(ASTOperator.NOT_EQUAL, fp64_col, fp64_needle), - ), - ), - Operation( - ASTOperator.LOGICAL_OR, - str_eq, - Operation(ASTOperator.EQUAL, fp64_col, fp64_needle), - ), - ) - - @pytest.mark.skipif( pa.__version__ == "19.0.0", reason="https://github.com/apache/arrow/issues/45283, https://github.com/NVIDIA/cudf/issues/17806", From fe8d6912e8c93c33797c8aa4b56c5816fbb5ceb2 Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Fri, 21 Aug 2026 19:49:29 +0000 Subject: [PATCH 08/14] ruff format --- .../cudf/tests/input_output/test_parquet.py | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/python/cudf/cudf/tests/input_output/test_parquet.py b/python/cudf/cudf/tests/input_output/test_parquet.py index eb6f44802fd..174189793ef 100644 --- a/python/cudf/cudf/tests/input_output/test_parquet.py +++ b/python/cudf/cudf/tests/input_output/test_parquet.py @@ -4722,13 +4722,7 @@ def test_parquet_reader_mismatched_nullability_structs(tmp_path): def test_parquet_not_equal_with_nan_stats(tmp_path): - """`col != v` must not prune a row group whose NaN rows satisfy it. - - Arrow and parquet-mr both skip NaN when updating min/max, so a chunk of {NaN, v} - reports min == max == v and is indistinguishable from a constant-v chunk. The - `col != val` stats transform prunes exactly that shape, dropping the NaN rows - - which do satisfy `col != val`, since NaN != v is true. - """ + """`col != v` must not prune matching `NaN` rows.""" import pylibcudf as plc from pylibcudf.expressions import ( ASTOperator, @@ -4765,13 +4759,7 @@ def test_parquet_not_equal_with_nan_stats(tmp_path): def test_parquet_negated_ordering_with_nan_stats(tmp_path): - """`NOT(col < v)` must not be rewritten to `col >= v` for a float column. - - IEEE-754 makes every ordered comparison against NaN false, so a NaN row satisfies - `NOT(col < v)` while `col >= v` is false for it. Arrow writes min/max that merely - *exclude* NaN (cudf's own writer drops min/max entirely, per PARQUET-1246), so a row - group holding NaN still has usable statistics and would be wrongly pruned. - """ + """`NOT(col < v)` must not prune matching `NaN` rows.""" import pylibcudf as plc from pylibcudf.expressions import ( ASTOperator, @@ -4799,7 +4787,12 @@ def test_parquet_negated_ordering_with_nan_stats(tmp_path): source = plc.io.SourceInfo([str(path)]) options = plc.io.parquet.ParquetReaderOptions.builder(source).build() options.set_filter(filter_expr) - got = plc.io.parquet.read_parquet(options).tbl.to_arrow().column(0).to_pylist() + got = ( + plc.io.parquet.read_parquet(options) + .tbl.to_arrow() + .column(0) + .to_pylist() + ) # NOT(x < 50) is true for NaN and for 100/200/300, and false for 1.0/2.0 assert len(got) == 4 From 91c29f89152b8aef99d7b78259497aa85a18ab1b Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Fri, 21 Aug 2026 19:51:21 +0000 Subject: [PATCH 09/14] Clean up slop --- .../cudf/tests/input_output/test_parquet.py | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/python/cudf/cudf/tests/input_output/test_parquet.py b/python/cudf/cudf/tests/input_output/test_parquet.py index 174189793ef..cb685cf6005 100644 --- a/python/cudf/cudf/tests/input_output/test_parquet.py +++ b/python/cudf/cudf/tests/input_output/test_parquet.py @@ -4738,7 +4738,8 @@ def test_parquet_not_equal_with_nan_stats(tmp_path): # Sanity check the fixture: NaN is excluded, so row group 0 looks constant stats = pq.ParquetFile(path).metadata.row_group(0).column(0).statistics - assert stats.min == 5.0 and stats.max == 5.0 + assert_eq(stats.min, 5.0) + assert_eq(stats.max, 5.0) scalar = plc.Scalar.from_arrow(pa.scalar(5.0)) filter_expr = Operation( @@ -4751,11 +4752,10 @@ def test_parquet_not_equal_with_nan_stats(tmp_path): result = plc.io.parquet.read_parquet(options) # Neither row group may be pruned: rg0 holds a NaN, rg1 holds 7.0 and 8.0 - assert result.num_row_groups_after_stats_filter == 2 - got = result.tbl.to_arrow().column(0).to_pylist() - assert len(got) == 3 - assert math.isnan(got[0]) - assert got[1:] == [7.0, 8.0] + assert_eq(result.num_row_groups_after_stats_filter, 2) + assert_arrow_table_equal( + pa.table({"x": [float("nan"), 7.0, 8.0]}), result.tbl.to_arrow() + ) def test_parquet_negated_ordering_with_nan_stats(tmp_path): @@ -4776,7 +4776,9 @@ def test_parquet_negated_ordering_with_nan_stats(tmp_path): # Sanity check the fixture actually reproduces the Arrow statistics behaviour stats = pq.ParquetFile(path).metadata.row_group(0).column(0).statistics - assert stats.has_min_max and stats.min == 1.0 and stats.max == 2.0 + assert_eq(stats.has_min_max, True) + assert_eq(stats.min, 1.0) + assert_eq(stats.max, 2.0) col = ColumnNameReference("x") lit = Literal(plc.Scalar.from_arrow(pa.scalar(50.0))) @@ -4787,17 +4789,13 @@ def test_parquet_negated_ordering_with_nan_stats(tmp_path): source = plc.io.SourceInfo([str(path)]) options = plc.io.parquet.ParquetReaderOptions.builder(source).build() options.set_filter(filter_expr) - got = ( - plc.io.parquet.read_parquet(options) - .tbl.to_arrow() - .column(0) - .to_pylist() - ) + got = plc.io.parquet.read_parquet(options).tbl.to_arrow() # NOT(x < 50) is true for NaN and for 100/200/300, and false for 1.0/2.0 - assert len(got) == 4 - assert math.isnan(got[0]) - assert got[1:] == [100.0, 200.0, 300.0] + assert_arrow_table_equal( + pa.table({"x": [float("nan"), 100.0, 200.0, 300.0]}), + got, + ) @pytest.mark.skipif( From cc246eafb93046e8af76cec4dafe6e4aa5da140d Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Fri, 21 Aug 2026 20:05:08 +0000 Subject: [PATCH 10/14] Address comments --- cpp/src/io/parquet/stats_filter_helpers.cpp | 7 ++++--- cpp/tests/io/parquet_reader_test.cpp | 9 +++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/cpp/src/io/parquet/stats_filter_helpers.cpp b/cpp/src/io/parquet/stats_filter_helpers.cpp index 83c5f11ea8f..970703cef1e 100644 --- a/cpp/src/io/parquet/stats_filter_helpers.cpp +++ b/cpp/src/io/parquet/stats_filter_helpers.cpp @@ -155,8 +155,8 @@ std::reference_wrapper stats_expression_converter::visit( } } // Binary operation wrapped else if (cudf::ast::detail::ast_operator_arity(child_op) == 2) { - // For NOT(col op lit) negate the operator if negatable and visit the negated operation - // directly. + // For NOT(col op lit) or NOT(lit op col), negate the operator if negatable and visit + // the negated operation directly. auto const binary_operands = extract_binary_operands(*child_operation); auto const lhs_kind = binary_operands.lhs_type; auto const rhs_kind = binary_operands.rhs_type; @@ -174,7 +174,8 @@ std::reference_wrapper stats_expression_converter::visit( _output_dtypes[binary_operands.col_ref->get_column_index()]); if (is_equality or can_negate_ordering) { - auto const negated_op = transform_operator(child_op); + auto const negated_op = + transform_operator(child_operation->get_operator()); if (negated_op.has_value()) { auto const& child_operands = child_operation->get_operands(); return visit( diff --git a/cpp/tests/io/parquet_reader_test.cpp b/cpp/tests/io/parquet_reader_test.cpp index 7f827dec56a..abe1ad296e8 100644 --- a/cpp/tests/io/parquet_reader_test.cpp +++ b/cpp/tests/io/parquet_reader_test.cpp @@ -2422,6 +2422,15 @@ TEST_F(ParquetReaderTest, FilterNegationPushdown) expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, not_lt), 1); } + // NOT(50 op col_a) - literal-left ordering comparisons preserve operand order when complemented. + for (auto const op : {cudf::ast::ast_operator::LESS, + cudf::ast::ast_operator::LESS_EQUAL, + cudf::ast::ast_operator::GREATER, + cudf::ast::ast_operator::GREATER_EQUAL}) { + auto literal_left = cudf::ast::operation(op, lit_50, col_ref_a); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, literal_left)); + } + // NOT(col_a + 10 > 50) - operand is not `col op lit`, so it must NOT be complemented. The // `col_a < 150` conjunct keeps the filter stats-usable. { From a0bdb649c28e86897cfe4dff9e590b8c14db33c7 Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Fri, 21 Aug 2026 20:10:13 +0000 Subject: [PATCH 11/14] minor fix --- .../cudf/tests/input_output/test_parquet.py | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/python/cudf/cudf/tests/input_output/test_parquet.py b/python/cudf/cudf/tests/input_output/test_parquet.py index cb685cf6005..d7254533430 100644 --- a/python/cudf/cudf/tests/input_output/test_parquet.py +++ b/python/cudf/cudf/tests/input_output/test_parquet.py @@ -4753,9 +4753,10 @@ def test_parquet_not_equal_with_nan_stats(tmp_path): # Neither row group may be pruned: rg0 holds a NaN, rg1 holds 7.0 and 8.0 assert_eq(result.num_row_groups_after_stats_filter, 2) - assert_arrow_table_equal( - pa.table({"x": [float("nan"), 7.0, 8.0]}), result.tbl.to_arrow() - ) + got = result.tbl.to_arrow().column(0).to_pylist() + assert_eq(len(got), 3) + assert_eq(math.isnan(got[0]), True) + assert_eq(got[1:], [7.0, 8.0]) def test_parquet_negated_ordering_with_nan_stats(tmp_path): @@ -4789,13 +4790,17 @@ def test_parquet_negated_ordering_with_nan_stats(tmp_path): source = plc.io.SourceInfo([str(path)]) options = plc.io.parquet.ParquetReaderOptions.builder(source).build() options.set_filter(filter_expr) - got = plc.io.parquet.read_parquet(options).tbl.to_arrow() + got = ( + plc.io.parquet.read_parquet(options) + .tbl.to_arrow() + .column(0) + .to_pylist() + ) # NOT(x < 50) is true for NaN and for 100/200/300, and false for 1.0/2.0 - assert_arrow_table_equal( - pa.table({"x": [float("nan"), 100.0, 200.0, 300.0]}), - got, - ) + assert_eq(len(got), 4) + assert_eq(math.isnan(got[0]), True) + assert_eq(got[1:], [100.0, 200.0, 300.0]) @pytest.mark.skipif( From 8d18b6e28b65da938b0d16d4ef343a06aed45b92 Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Fri, 21 Aug 2026 20:18:03 +0000 Subject: [PATCH 12/14] Address comments from @vuule --- .../parquet/experimental/page_index_filter.cu | 4 +--- cpp/src/io/parquet/predicate_pushdown.cpp | 3 +-- cpp/src/io/parquet/stats_filter_helpers.cpp | 21 +++++++++++++------ cpp/src/io/parquet/stats_filter_helpers.hpp | 6 +++++- cpp/tests/io/parquet_reader_test.cpp | 7 +++++-- 5 files changed, 27 insertions(+), 14 deletions(-) diff --git a/cpp/src/io/parquet/experimental/page_index_filter.cu b/cpp/src/io/parquet/experimental/page_index_filter.cu index 2ef90e9a94a..a9b2c83639d 100644 --- a/cpp/src/io/parquet/experimental/page_index_filter.cu +++ b/cpp/src/io/parquet/experimental/page_index_filter.cu @@ -865,9 +865,7 @@ std::unique_ptr aggregate_reader_metadata::build_row_mask_with_pag // Get a boolean mask indicating which columns will participate in stats based filtering auto const [stats_columns_mask, has_is_null_operator] = - parquet::detail::stats_columns_collector{filter.get(), - static_cast(output_dtypes.size())} - .get_stats_columns_mask(); + parquet::detail::stats_columns_collector{filter.get(), output_dtypes}.get_stats_columns_mask(); // Return early if no columns will participate in stats based page filtering if (stats_columns_mask.empty()) { return build_all_true_row_mask(row_group_indices, stream, mr); } diff --git a/cpp/src/io/parquet/predicate_pushdown.cpp b/cpp/src/io/parquet/predicate_pushdown.cpp index 05d954ee7af..60e07024b2b 100644 --- a/cpp/src/io/parquet/predicate_pushdown.cpp +++ b/cpp/src/io/parquet/predicate_pushdown.cpp @@ -69,8 +69,7 @@ std::optional>> aggregate_reader_metadata::ap // Get a boolean mask indicating which columns can participate in stats based filtering auto const [stats_columns_mask, has_is_null_operator] = - stats_columns_collector{filter.get(), static_cast(output_dtypes.size())} - .get_stats_columns_mask(); + stats_columns_collector{filter.get(), output_dtypes}.get_stats_columns_mask(); // Return early if no columns will participate in stats based filtering if (stats_columns_mask.empty()) { return std::nullopt; } diff --git a/cpp/src/io/parquet/stats_filter_helpers.cpp b/cpp/src/io/parquet/stats_filter_helpers.cpp index 970703cef1e..fc9f6e680fc 100644 --- a/cpp/src/io/parquet/stats_filter_helpers.cpp +++ b/cpp/src/io/parquet/stats_filter_helpers.cpp @@ -14,11 +14,16 @@ namespace cudf::io::parquet::detail { +stats_columns_collector::stats_columns_collector(std::span output_dtypes) + : _num_columns(static_cast(output_dtypes.size())), _output_dtypes(output_dtypes) +{ +} + stats_columns_collector::stats_columns_collector(ast::expression const& expr, - cudf::size_type num_columns) - : _num_columns(num_columns) + std::span output_dtypes) + : stats_columns_collector(output_dtypes) { - _columns_mask.resize(num_columns, false); + _columns_mask.resize(_num_columns, false); expr.accept(*this); } @@ -75,7 +80,12 @@ std::reference_wrapper stats_columns_collector::visit( if (op == ast_operator::EQUAL or op == ast_operator::NOT_EQUAL or op == ast_operator::LESS or op == ast_operator::LESS_EQUAL or op == ast_operator::GREATER or op == ast_operator::GREATER_EQUAL) { - _columns_mask[col_ref->get_column_index()] = true; + // NOT_EQUAL leaf for floating points relaxes to always true as Parquet statistics do not + // record NaNs. + if (op != ast_operator::NOT_EQUAL or + not cudf::is_floating_point(_output_dtypes[col_ref->get_column_index()])) { + _columns_mask[col_ref->get_column_index()] = true; + } } } else { // Visit the operands and ignore any output as we only want to build the column mask @@ -94,12 +104,11 @@ stats_expression_converter::stats_expression_converter( std::span output_dtypes, bool has_is_null_operator, cuda::stream_ref stream) - : _output_dtypes{output_dtypes}, + : stats_columns_collector{output_dtypes}, _always_true_scalar{std::make_unique>(true, true, stream)}, _always_true{std::make_unique(*_always_true_scalar)} { _stats_cols_per_column = has_is_null_operator ? 3 : 2; - _num_columns = static_cast(output_dtypes.size()); expr.accept(*this); } diff --git a/cpp/src/io/parquet/stats_filter_helpers.hpp b/cpp/src/io/parquet/stats_filter_helpers.hpp index 0d1f0ab371b..30d1f3beadc 100644 --- a/cpp/src/io/parquet/stats_filter_helpers.hpp +++ b/cpp/src/io/parquet/stats_filter_helpers.hpp @@ -308,7 +308,8 @@ class stats_columns_collector : public ast::detail::expression_transformer { public: stats_columns_collector() = default; - stats_columns_collector(ast::expression const& expr, cudf::size_type num_columns); + stats_columns_collector(ast::expression const& expr, + std::span output_dtypes); /** * @copydoc ast::detail::expression_transformer::visit(ast::literal const& ) @@ -340,7 +341,10 @@ class stats_columns_collector : public ast::detail::expression_transformer { std::pair, bool> get_stats_columns_mask() &&; protected: + stats_columns_collector(std::span output_dtypes); + size_type _num_columns; + std::span _output_dtypes; private: thrust::host_vector _columns_mask; diff --git a/cpp/tests/io/parquet_reader_test.cpp b/cpp/tests/io/parquet_reader_test.cpp index abe1ad296e8..d9a3a1d8f64 100644 --- a/cpp/tests/io/parquet_reader_test.cpp +++ b/cpp/tests/io/parquet_reader_test.cpp @@ -2428,7 +2428,10 @@ TEST_F(ParquetReaderTest, FilterNegationPushdown) cudf::ast::ast_operator::GREATER, cudf::ast::ast_operator::GREATER_EQUAL}) { auto literal_left = cudf::ast::operation(op, lit_50, col_ref_a); - expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, literal_left)); + auto const expected_row_groups = + op == cudf::ast::ast_operator::LESS or op == cudf::ast::ast_operator::LESS_EQUAL ? 1 : 4; + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, literal_left), + expected_row_groups); } // NOT(col_a + 10 > 50) - operand is not `col op lit`, so it must NOT be complemented. The @@ -2438,7 +2441,7 @@ TEST_F(ParquetReaderTest, FilterNegationPushdown) auto sum_gt_50 = cudf::ast::operation(cudf::ast::ast_operator::GREATER, sum, lit_50); auto not_sum = cudf::ast::operation(cudf::ast::ast_operator::NOT, sum_gt_50); expect_matches_unrewritten( - cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_AND, a_lt_150, not_sum)); + cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_AND, a_lt_150, not_sum), 1); } // Double negation over a non-boolean operand must NOT be eliminated. From 76f3e8c3fc5d2fe00a973843e73ac9297259dcd6 Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Fri, 21 Aug 2026 20:24:06 +0000 Subject: [PATCH 13/14] minor --- cpp/src/io/parquet/stats_filter_helpers.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/cpp/src/io/parquet/stats_filter_helpers.hpp b/cpp/src/io/parquet/stats_filter_helpers.hpp index 30d1f3beadc..0ef941c8ed3 100644 --- a/cpp/src/io/parquet/stats_filter_helpers.hpp +++ b/cpp/src/io/parquet/stats_filter_helpers.hpp @@ -387,7 +387,6 @@ class stats_expression_converter : public stats_columns_collector { thrust::host_vector get_stats_columns_mask() && = delete; private: - std::span _output_dtypes; ast::tree _stats_expr; cudf::size_type _stats_cols_per_column; std::unique_ptr> _always_true_scalar; From bdb0f7497862aacd1024776b103e26627db3e59f Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Fri, 21 Aug 2026 20:26:56 +0000 Subject: [PATCH 14/14] minor --- cpp/src/io/parquet/stats_filter_helpers.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/io/parquet/stats_filter_helpers.cpp b/cpp/src/io/parquet/stats_filter_helpers.cpp index fc9f6e680fc..17830ca0f6b 100644 --- a/cpp/src/io/parquet/stats_filter_helpers.cpp +++ b/cpp/src/io/parquet/stats_filter_helpers.cpp @@ -17,13 +17,13 @@ namespace cudf::io::parquet::detail { stats_columns_collector::stats_columns_collector(std::span output_dtypes) : _num_columns(static_cast(output_dtypes.size())), _output_dtypes(output_dtypes) { + _columns_mask.resize(_num_columns, false); } stats_columns_collector::stats_columns_collector(ast::expression const& expr, std::span output_dtypes) : stats_columns_collector(output_dtypes) { - _columns_mask.resize(_num_columns, false); expr.accept(*this); }