From 023cae7b57fbd90afd70afe2bdafbcb0b2d18862 Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Thu, 6 Aug 2026 15:37:29 -0700 Subject: [PATCH 1/9] Push logical negations down to the leaves of parquet filter expressions The parquet pruning transformers convert a filter into an expression over per-row-group summary columns that answers "might some row here match?". That is an existential, and existentials are not closed under negation: "no row is 5" is not "some row is not 5". The bloom filter and dictionary page converters wrapped the transformed child in the input unary operator, so `NOT(col == 5)` became `NOT(may_contain(5))` and pruned every row group holding a single 5, dropping rows that satisfy `col != 5`. This affected the hybrid scan dictionary filter and the bloom filter path in both the hybrid scan and the regular reader. Rewrite the filter once in `named_to_reference_converter` so that no transformer ever sees `NOT` over an expression it has already rewritten: eliminate double negations, apply De Morgan's laws over both the null-propagating and the Kleene logical operators, and complement the equality operators. This converted expression also filters the decoded rows, so only exact rewrites are applied - ordering comparisons are left alone because every ordered comparison against a NaN is false, making `NOT(a < b)` true exactly where `a >= b` is false. `apply_unary_membership_transform` now always relaxes to `always_true` rather than negating a membership result. After the pushdown no `NOT` should reach it with a non-relaxed child, but that is a by-construction argument and the guard keeps a future gap from returning wrong rows. De Morgan also lets the stats converter evaluate negated compound predicates it previously gave up on, so `NOT(a AND b)` now prunes. Co-Authored-By: Claude Opus 5 --- cpp/src/io/parquet/bloom_filter_reader.cu | 2 +- .../experimental/dictionary_page_filter.cu | 2 +- .../parquet/expression_transform_helpers.cpp | 70 ++++++++++ .../parquet/expression_transform_helpers.hpp | 78 +++++++---- .../experimental/hybrid_scan_filters_test.cpp | 32 ++++- cpp/tests/io/parquet_reader_test.cpp | 130 ++++++++++++++++++ 6 files changed, 279 insertions(+), 35 deletions(-) diff --git a/cpp/src/io/parquet/bloom_filter_reader.cu b/cpp/src/io/parquet/bloom_filter_reader.cu index 3418c8ca9ada..b0a651390376 100644 --- a/cpp/src/io/parquet/bloom_filter_reader.cu +++ b/cpp/src/io/parquet/bloom_filter_reader.cu @@ -231,7 +231,7 @@ class bloom_filter_expression_converter : public equality_literals_collector { return this->visit_operands(operands); }; return parquet::detail::apply_unary_membership_transform( - expr, _bloom_filter_expr, *_always_true, *this, visit_operands_fn); + expr, _bloom_filter_expr, *_always_true, visit_operands_fn); } // Binary operation diff --git a/cpp/src/io/parquet/experimental/dictionary_page_filter.cu b/cpp/src/io/parquet/experimental/dictionary_page_filter.cu index 501d2cbab355..36665cd1a7b1 100644 --- a/cpp/src/io/parquet/experimental/dictionary_page_filter.cu +++ b/cpp/src/io/parquet/experimental/dictionary_page_filter.cu @@ -1366,7 +1366,7 @@ class dictionary_expression_converter : public equality_literals_collector { return this->visit_operands(operands); }; return parquet::detail::apply_unary_membership_transform( - expr, _dictionary_expr, *_always_true, *this, visit_operands_fn); + expr, _dictionary_expr, *_always_true, visit_operands_fn); } // Binary operation diff --git a/cpp/src/io/parquet/expression_transform_helpers.cpp b/cpp/src/io/parquet/expression_transform_helpers.cpp index 8c9351cba92a..c2478728257e 100644 --- a/cpp/src/io/parquet/expression_transform_helpers.cpp +++ b/cpp/src/io/parquet/expression_transform_helpers.cpp @@ -137,9 +137,79 @@ std::reference_wrapper named_to_reference_converter::visi return std::reference_wrapper(_col_ref.back()); } +std::optional> +named_to_reference_converter::push_down_negation(ast::expression const& operand) +{ + using cudf::ast::ast_operator; + + auto const* child_operation = dynamic_cast(&operand); + if (child_operation == nullptr) { return std::nullopt; } + + auto const op = child_operation->get_operator(); + auto const operands = child_operation->get_operands(); + + // `NOT(NOT(x))` is `x`, including when `x` is null + if (op == ast_operator::NOT) { return operands.front().get().accept(*this); } + + // De Morgan's laws. Exact for the null-propagating operators, where a null operand makes both + // sides null, and for the Kleene operators, where they hold by definition + auto const de_morgan_op = [op]() -> std::optional { + switch (op) { + case ast_operator::LOGICAL_AND: return ast_operator::LOGICAL_OR; + case ast_operator::LOGICAL_OR: return ast_operator::LOGICAL_AND; + case ast_operator::NULL_LOGICAL_AND: return ast_operator::NULL_LOGICAL_OR; + case ast_operator::NULL_LOGICAL_OR: return ast_operator::NULL_LOGICAL_AND; + default: return std::nullopt; + } + }(); + if (de_morgan_op.has_value()) { + // Negate both operands before emplacing the parent, as negating them appends to `_operators` + auto const lhs = negate(operands.front().get()); + auto const rhs = negate(operands.back().get()); + _operators.emplace_back(de_morgan_op.value(), lhs, rhs); + return std::reference_wrapper(_operators.back()); + } + + // Complement the equality operators. Exact for floating point values as well: IEEE-754 makes + // `NaN == x` false and `NaN != x` true, so the two stay exact complements. Ordering comparisons + // are excluded - every ordered comparison against a `NaN` is false, so `NOT(a < b)` is true + // exactly where `a >= b` is false + if (op == ast_operator::EQUAL or op == ast_operator::NOT_EQUAL) { + auto const complement = + (op == ast_operator::EQUAL) ? ast_operator::NOT_EQUAL : ast_operator::EQUAL; + auto new_operands = visit_operands(operands); + _operators.emplace_back(complement, new_operands.front(), new_operands.back()); + return std::reference_wrapper(_operators.back()); + } + + return std::nullopt; +} + +std::reference_wrapper named_to_reference_converter::negate( + ast::expression const& operand) +{ + if (auto const negated = push_down_negation(operand); negated.has_value()) { + return negated.value(); + } + // No exact rewrite, so convert the operand and negate it in place + auto const new_operand = operand.accept(*this); + _operators.emplace_back(ast::ast_operator::NOT, new_operand); + return std::reference_wrapper(_operators.back()); +} + std::reference_wrapper named_to_reference_converter::visit( ast::operation const& expr) { + // Push the negation down to the leaves so that no downstream transformer has to reason about + // `NOT` over an expression it has already rewritten + if (expr.get_operator() == ast::ast_operator::NOT) { + if (auto const negated = push_down_negation(expr.get_operands().front().get()); + negated.has_value()) { + _converted_expr = negated.value(); + return negated.value(); + } + } + auto const operands = expr.get_operands(); auto op = expr.get_operator(); auto new_operands = visit_operands(operands); diff --git a/cpp/src/io/parquet/expression_transform_helpers.hpp b/cpp/src/io/parquet/expression_transform_helpers.hpp index 3762c174c941..2f7bb5123886 100644 --- a/cpp/src/io/parquet/expression_transform_helpers.hpp +++ b/cpp/src/io/parquet/expression_transform_helpers.hpp @@ -116,46 +116,36 @@ template * @brief Handle unary operation transform for membership-based row group filters. i.e., bloom * filter and dictionary page filter. * - * @tparam VisitorType Type of the AST visitor that implements accept() + * A transformed operand of a membership filter is an existential summary of a row group - it + * answers "could some row here match?" rather than "is the operand true?". No unary operator is + * meaning-preserving over such a summary, so a unary operation is always relaxed to `always_true`. + * + * `NOT` is the case that matters: transforming `NOT(col == v)` into `NOT(dict_contains(v))` would + * prune every row group holding a single `v`, whereas `col != v` only permits pruning a row group + * whose values are all `v`. See `negation_pushdown`, which rewrites `NOT(col == v)` into + * `col != v` before it ever reaches a converter. + * * @tparam VisitOperandsFn Callable matching `(host_span>) -> * vector>` * * @param expr Unary operation to transform * @param expr_tree The AST tree to push transformed expressions into * @param always_true Reference to the always_true sentinel literal - * @param visitor The visitor used to accept column references * @param visit_operands_fn Callable to visit operands and return the transformed operands - * @return Transformed expression or _always_true if the operation cannot be evaluated + * @return The `always_true` expression */ -template +template [[nodiscard]] inline std::reference_wrapper apply_unary_membership_transform( ast::operation const& expr, ast::tree& expr_tree, std::reference_wrapper const always_true, - VisitorType& visitor, VisitOperandsFn&& visit_operands_fn) { - auto const [kind, col_ref] = extract_unary_operand(expr); - - // For `op col` form, push the `_always_true` expression - if (kind == operand_kind::COLUMN_REF) { - col_ref->accept(visitor); - expr_tree.push(ast::operation{ast::ast_operator::IDENTITY, always_true}); - return always_true; - } - // For `op expr` form, visit operands and push expression - else { - auto new_operands = visit_operands_fn(expr.get_operands()); - if (&new_operands.front().get() == &always_true.get()) { - // Pass through the _always_true child operand as is - expr_tree.push(ast::operation{ast::ast_operator::IDENTITY, expr_tree.back()}); - return always_true; - } else { - auto const input_op = expr.get_operator(); - expr_tree.push(ast::operation{input_op, new_operands.front()}); - return expr_tree.back(); - } - } + // Visit the operands to validate column references and collect any nested literals, then discard + // the transformed operands and relax this operation to `always_true` + std::ignore = visit_operands_fn(expr.get_operands()); + expr_tree.push(ast::operation{ast::ast_operator::IDENTITY, always_true}); + return always_true; } /** @@ -208,7 +198,12 @@ class names_from_expression : public ast::detail::expression_transformer { }; /** - * @brief Converts named columns to index reference columns + * @brief Converts named columns to index reference columns and pushes logical negations down to + * the leaves of the expression + * + * The converted expression is the single expression the reader uses both to prune row groups and + * pages, and to filter the decoded rows. Every negation rewrite must therefore be an exact + * equivalence rather than a relaxation - see `push_down_negation()`. */ class named_to_reference_converter : public ast::detail::expression_transformer { public: @@ -254,6 +249,35 @@ class named_to_reference_converter : public ast::detail::expression_transformer std::vector> visit_operands( cudf::host_span const> operands); + /** + * @brief Rewrites `NOT(operand)` into an equivalent expression with the negation pushed into + * `operand`'s own operands + * + * Only rewrites that are exact in every case cudf's AST evaluates are applied, as the converted + * expression also filters the decoded rows: + * + * - `NOT(NOT(x))` becomes `x` + * - `NOT(a AND b)` becomes `NOT(a) OR NOT(b)` and the three other De Morgan forms, for both the + * null-propagating (`LOGICAL_*`) and the Kleene (`NULL_LOGICAL_*`) operators + * - `NOT(a == b)` becomes `a != b` and vice versa + * + * Ordering comparisons are deliberately **not** complemented. IEEE-754 makes every ordered + * comparison against a `NaN` false, so `NOT(a < b)` is true exactly where `a >= b` is false. + * `NOT(IS_NULL(x))` and `NOT(NULL_EQUAL(a, b))` have no complement operator and are left alone. + * + * @param operand The operand of the `NOT` operation to rewrite + * @return The rewritten expression, or std::nullopt if no exact rewrite exists + */ + [[nodiscard]] std::optional> push_down_negation( + ast::expression const& operand); + + /** + * @brief Returns the converted negation of `operand`, pushing the negation down if possible and + * otherwise wrapping the converted operand in a `NOT` + */ + [[nodiscard]] std::reference_wrapper negate( + ast::expression const& operand); + column_path_map _column_name_to_index; std::optional> _converted_expr; // Using std::list or std::deque to avoid reference invalidation diff --git a/cpp/tests/io/experimental/hybrid_scan_filters_test.cpp b/cpp/tests/io/experimental/hybrid_scan_filters_test.cpp index d0d4f570adbc..065239085b9b 100644 --- a/cpp/tests/io/experimental/hybrid_scan_filters_test.cpp +++ b/cpp/tests/io/experimental/hybrid_scan_filters_test.cpp @@ -489,7 +489,9 @@ TEST_F(HybridScanFiltersTest, FilterRowGroupsWithComplexExpressions) } // Filter: NOT(col0 > 50 AND col0 < 100) - // NOT over a compound expression (LOGICAL_AND) cannot be negated, degrades to always_true. + // De Morgan rewrites this to NOT(col0 > 50) OR NOT(col0 < 100), i.e. col0 <= 50 OR col0 >= 100, + // stats transform: vmin <= 50 OR vmax >= 100. Every row group satisfies one of the two disjuncts, + // so nothing is pruned - but now for an evaluated reason rather than a relaxation. { auto literal_50_value = cudf::numeric_scalar(50, true, cudf::get_default_stream()); auto literal_50 = cudf::ast::literal(literal_50_value); @@ -509,7 +511,8 @@ TEST_F(HybridScanFiltersTest, FilterRowGroupsWithComplexExpressions) } // Filter: NOT(NOT(col0 < 100) OR col0 > 150) - // Outer NOT wraps a compound expression (LOGICAL_OR), degrades to always_true. + // De Morgan plus double-negation elimination rewrites this to col0 < 100 AND NOT(col0 > 150), + // stats transform: vmin < 100 AND vmin <= 150. Prunes RG2 (vmin=100) and RG3 (vmin=150). { auto literal_100_value = cudf::numeric_scalar(100, true, cudf::get_default_stream()); auto literal_100 = cudf::ast::literal(literal_100_value); @@ -526,7 +529,8 @@ TEST_F(HybridScanFiltersTest, FilterRowGroupsWithComplexExpressions) auto input_row_group_indices = reader->all_row_groups(options); auto stats_filtered = reader->filter_row_groups_with_stats( input_row_group_indices, options, cudf::get_default_stream()); - EXPECT_EQ(stats_filtered.size(), 4); + EXPECT_EQ(stats_filtered.size(), 2); + EXPECT_EQ(reader->total_rows_in_row_groups(stats_filtered), 2 * rows_per_row_group); } } @@ -1319,6 +1323,9 @@ TEST_F(HybridScanFiltersTest, FilterRowGroupsWithDictionary) } // Filtering - NOT(table[0] == 50) + // Rewritten to table[0] != 50, which prunes a row group only when 50 is the *only* value in its + // dictionary. The row group holding 50 holds other values too, so nothing is pruned. Negating the + // membership result instead would prune that row group and drop its non-50 rows. { auto uint_literal_value = cudf::numeric_scalar(50, true, stream); auto uint_literal = cudf::ast::literal(uint_literal_value); @@ -1328,11 +1335,22 @@ TEST_F(HybridScanFiltersTest, FilterRowGroupsWithDictionary) cudf::io::parquet_reader_options::builder().filter(filter_expression).build(); auto const result = filter_row_groups_with_dictionaries(datasource_ref, reader_ref, options, stream, mr); - auto const expected = std::vector{0, 2, 3}; + auto const expected = std::vector{0, 1, 2, 3}; EXPECT_EQ(result, expected); + + // `NOT(col == v)` and `col != v` are the same predicate and must prune identically + auto const not_equal = + cudf::ast::operation(cudf::ast::ast_operator::NOT_EQUAL, col0_ref, uint_literal); + auto const not_equal_options = + cudf::io::parquet_reader_options::builder().filter(not_equal).build(); + EXPECT_EQ(result, + filter_row_groups_with_dictionaries( + datasource_ref, reader_ref, not_equal_options, stream, mr)); } // Filtering - NOT(table[0] == 50) AND (table[0] NULL_EQUAL 100) + // Rewritten to (table[0] != 50) AND NULL_EQUAL(...). NULL_EQUAL has no dictionary transform and + // relaxes, and table[0] != 50 prunes nothing, so all row groups survive. { auto literal_50_value = cudf::numeric_scalar(50, true, stream); auto literal_50 = cudf::ast::literal(literal_50_value); @@ -1348,11 +1366,13 @@ TEST_F(HybridScanFiltersTest, FilterRowGroupsWithDictionary) cudf::io::parquet_reader_options::builder().filter(filter_expression).build(); auto const result = filter_row_groups_with_dictionaries(datasource_ref, reader_ref, options, stream, mr); - auto const expected = std::vector{0, 2, 3}; + auto const expected = std::vector{0, 1, 2, 3}; EXPECT_EQ(result, expected); } // Filtering - NOT(table[0] == 50) OR NOT(table[2] == "0100") + // Rewritten to (table[0] != 50) OR (table[2] != "0100"), matching the non-negated spelling of the + // same predicate tested above. { auto literal_50_value = cudf::numeric_scalar(50, true, stream); auto literal_50 = cudf::ast::literal(literal_50_value); @@ -1368,7 +1388,7 @@ TEST_F(HybridScanFiltersTest, FilterRowGroupsWithDictionary) cudf::io::parquet_reader_options::builder().filter(filter_expression).build(); auto const result = filter_row_groups_with_dictionaries(datasource_ref, reader_ref, options, stream, mr); - auto const expected = std::vector{0, 2, 3}; + auto const expected = std::vector{0, 1, 2, 3}; EXPECT_EQ(result, expected); } } diff --git a/cpp/tests/io/parquet_reader_test.cpp b/cpp/tests/io/parquet_reader_test.cpp index 6ac067e74d3f..a63eacb58c16 100644 --- a/cpp/tests/io/parquet_reader_test.cpp +++ b/cpp/tests/io/parquet_reader_test.cpp @@ -1932,6 +1932,136 @@ TEST_F(ParquetReaderTest, ExtendedFilterExpressions) } } +TEST_F(ParquetReaderTest, FilterNegationPushdown) +{ + // The reader rewrites the filter into an equivalent form with logical negations pushed down to + // the leaves, and uses that single rewritten expression both to prune row groups and to filter + // the decoded rows. Every rewrite must therefore be exact, including for nulls and NaNs, so each + // case below compares the reader's output against the unrewritten filter evaluated over the + // whole table. + auto constexpr num_rows = 20'000; + + // Nulls every 7th row so that nulls straddle row group boundaries + auto const valids = + cudf::detail::make_counting_transform_iterator(0, [](auto i) { return i % 7 != 0; }); + auto col_a = cudf::test::fixed_width_column_wrapper( + cuda::counting_iterator{0}, cuda::counting_iterator{num_rows}, valids); + auto col_b = cudf::test::fixed_width_column_wrapper( + cuda::counting_iterator{num_rows}, cuda::counting_iterator{2 * num_rows}, valids); + // Half the rows are NaN. Ordered comparisons against NaN are all false, so complementing them + // would not be an equivalence + auto const floats = cudf::detail::make_counting_transform_iterator( + 0, [](auto i) { return i % 2 == 0 ? NAN : static_cast(i); }); + auto col_c = cudf::test::fixed_width_column_wrapper(floats, floats + num_rows); + + auto const written_table = cudf::table_view{{col_a, col_b, col_c}}; + auto const filepath = temp_env->get_temp_filepath("FilterNegationPushdown.parquet"); + cudf::io::parquet_writer_options const out_opts = + cudf::io::parquet_writer_options::builder(cudf::io::sink_info{filepath}, written_table) + .row_group_size_rows(5'000) + .stats_level(cudf::io::statistics_freq::STATISTICS_ROWGROUP); + cudf::io::write_parquet(out_opts); + + auto const expect_matches_unrewritten = [&](cudf::ast::expression const& filter) { + auto predicate = cudf::compute_column(written_table, filter); + auto expected = cudf::apply_boolean_mask(written_table, *predicate); + + cudf::io::parquet_reader_options const read_opts = + cudf::io::parquet_reader_options::builder(cudf::io::source_info{filepath}).filter(filter); + auto result = cudf::io::read_parquet(read_opts); + CUDF_TEST_EXPECT_TABLES_EQUAL(*result.tbl, *expected); + }; + + auto col_ref_a = cudf::ast::column_reference(0); + auto col_ref_b = cudf::ast::column_reference(1); + auto col_ref_c = cudf::ast::column_reference(2); + + auto lit_10_value = cudf::numeric_scalar(10); + auto lit_10 = cudf::ast::literal(lit_10_value); + auto lit_50_value = cudf::numeric_scalar(50); + auto lit_50 = cudf::ast::literal(lit_50_value); + auto lit_150_value = cudf::numeric_scalar(150); + auto lit_150 = cudf::ast::literal(lit_150_value); + auto lit_nan_value = cudf::numeric_scalar(NAN, true); + auto lit_nan = cudf::ast::literal(lit_nan_value); + auto lit_f50_value = cudf::numeric_scalar(50.0f, true); + auto lit_f50 = cudf::ast::literal(lit_f50_value); + + auto a_eq_10 = cudf::ast::operation(cudf::ast::ast_operator::EQUAL, col_ref_a, lit_10); + auto a_neq_10 = cudf::ast::operation(cudf::ast::ast_operator::NOT_EQUAL, col_ref_a, lit_10); + auto a_lt_50 = cudf::ast::operation(cudf::ast::ast_operator::LESS, col_ref_a, lit_50); + auto a_gt_50 = cudf::ast::operation(cudf::ast::ast_operator::GREATER, col_ref_a, lit_50); + auto a_lt_150 = cudf::ast::operation(cudf::ast::ast_operator::LESS, col_ref_a, lit_150); + auto b_eq_10 = cudf::ast::operation(cudf::ast::ast_operator::EQUAL, col_ref_b, lit_10); + + // NOT(NOT(col_a < 50)) - double negation elimination + { + auto not_lt = cudf::ast::operation(cudf::ast::ast_operator::NOT, a_lt_50); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, not_lt)); + } + + // NOT(col_a == 10) and NOT(col_a != 10) - complemented equality + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, a_eq_10)); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, a_neq_10)); + + // De Morgan over the null-propagating operators + { + auto conjunction = + cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_AND, a_gt_50, a_lt_150); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, conjunction)); + + auto disjunction = cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_OR, a_gt_50, a_eq_10); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, disjunction)); + } + + // De Morgan over the Kleene operators, where a null operand does not always produce a null result + { + auto conjunction = + cudf::ast::operation(cudf::ast::ast_operator::NULL_LOGICAL_AND, a_eq_10, b_eq_10); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, conjunction)); + + auto disjunction = + cudf::ast::operation(cudf::ast::ast_operator::NULL_LOGICAL_OR, a_eq_10, b_eq_10); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, disjunction)); + } + + // NOT(col_c == NaN) - equality stays an exact complement under IEEE-754 + { + auto c_eq_nan = cudf::ast::operation(cudf::ast::ast_operator::EQUAL, col_ref_c, lit_nan); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, c_eq_nan)); + } + + // NOT(col_c < 50.0) - ordered comparisons against NaN are all false, so this must NOT become + // col_c >= 50.0 + { + auto c_lt_50 = cudf::ast::operation(cudf::ast::ast_operator::LESS, col_ref_c, lit_f50); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, c_lt_50)); + + // ... including underneath a De Morgan rewrite + auto conjunction = cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_AND, c_lt_50, a_eq_10); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, conjunction)); + } + + // Operators with no complement are left intact + { + auto a_is_null = cudf::ast::operation(cudf::ast::ast_operator::IS_NULL, col_ref_a); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, a_is_null)); + + auto a_null_eq_10 = + cudf::ast::operation(cudf::ast::ast_operator::NULL_EQUAL, col_ref_a, lit_10); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, a_null_eq_10)); + } + + // Nested negations mixing all of the above + { + auto inner_or = cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_OR, a_eq_10, a_gt_50); + auto not_or = cudf::ast::operation(cudf::ast::ast_operator::NOT, inner_or); + auto outer_and = + cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_AND, not_or, a_lt_150); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, outer_and)); + } +} + TEST_F(ParquetReaderTest, FilterNamedExpression) { auto [src, filepath] = create_parquet_with_stats("NamedExpression.parquet"); From b769625a2879af4caae8b087c6268f4b9627e727 Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Thu, 6 Aug 2026 16:30:49 -0700 Subject: [PATCH 2/9] Add a bloom filter regression test for negated equality The bloom filter half of the negation bug had no coverage: cudf's writer cannot produce bloom filters, so it can only be tested against checked-in files. The mixed_card_ndv_*_bf_fpp0.1_nostats fixtures already carry bloom filters and no column chunk statistics, which makes the bloom filter the only thing that can prune them. Read those files with NOT(str == "FINDME") and with str != "FINDME" and assert the two spellings prune identically. Before the fix the negated form prunes the two row groups that hold a "FINDME" and returns 600 of the 998 matching rows, while the direct form prunes nothing. Co-Authored-By: Claude Opus 5 --- .../cudf/tests/input_output/test_parquet.py | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/python/cudf/cudf/tests/input_output/test_parquet.py b/python/cudf/cudf/tests/input_output/test_parquet.py index bc7af5360010..67984214bc54 100644 --- a/python/cudf/cudf/tests/input_output/test_parquet.py +++ b/python/cudf/cudf/tests/input_output/test_parquet.py @@ -4685,6 +4685,65 @@ def test_parquet_reader_mismatched_nullability_structs(tmp_path): ) +@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): + """`NOT(col == v)` must prune exactly what `col != v` prunes. + + A bloom filter answers "might this value be present", which is an existential over the + row group. Negating that answer asks "is the value definitely absent", which is a + different question: it prunes every row group holding a single `v`, dropping the rows + that do satisfy `col != v`. These files carry bloom filters and no column chunk + statistics, so the bloom filter is the only thing that can prune here. + """ + 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) + + negated_equality = read_with( + Operation( + ASTOperator.NOT, + Operation( + ASTOperator.EQUAL, ColumnNameReference("str"), needle + ), + ) + ) + not_equal = read_with( + Operation( + ASTOperator.NOT_EQUAL, ColumnNameReference("str"), needle + ) + ) + + # The two spellings are the same predicate and must prune identically + assert ( + negated_equality.num_row_groups_after_bloom_filter + == not_equal.num_row_groups_after_bloom_filter + ) + assert negated_equality.tbl.to_arrow().equals(not_equal.tbl.to_arrow()) + + # Both must keep every row that is not "FINDME". Negating the bloom filter result + # instead prunes the two row groups that hold a "FINDME" and returns only 600. + assert negated_equality.tbl.num_rows() == 998 + + @pytest.mark.skipif( pa.__version__ == "19.0.0", reason="https://github.com/apache/arrow/issues/45283, https://github.com/rapidsai/cudf/issues/17806", From 07595b63b85384a038abf8ad9c5e15d5ac4661f0 Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Fri, 7 Aug 2026 20:21:00 +0000 Subject: [PATCH 3/9] Cleanup --- .../parquet/expression_transform_helpers.cpp | 49 ++++------ .../parquet/expression_transform_helpers.hpp | 48 ++++++---- .../experimental/hybrid_scan_filters_test.cpp | 90 ++++++++++++------- cpp/tests/io/parquet_reader_test.cpp | 31 ++++--- .../cudf/tests/input_output/test_parquet.py | 19 +--- 5 files changed, 129 insertions(+), 108 deletions(-) diff --git a/cpp/src/io/parquet/expression_transform_helpers.cpp b/cpp/src/io/parquet/expression_transform_helpers.cpp index c2478728257e..803982876851 100644 --- a/cpp/src/io/parquet/expression_transform_helpers.cpp +++ b/cpp/src/io/parquet/expression_transform_helpers.cpp @@ -151,18 +151,9 @@ named_to_reference_converter::push_down_negation(ast::expression const& operand) // `NOT(NOT(x))` is `x`, including when `x` is null if (op == ast_operator::NOT) { return operands.front().get().accept(*this); } - // De Morgan's laws. Exact for the null-propagating operators, where a null operand makes both - // sides null, and for the Kleene operators, where they hold by definition - auto const de_morgan_op = [op]() -> std::optional { - switch (op) { - case ast_operator::LOGICAL_AND: return ast_operator::LOGICAL_OR; - case ast_operator::LOGICAL_OR: return ast_operator::LOGICAL_AND; - case ast_operator::NULL_LOGICAL_AND: return ast_operator::NULL_LOGICAL_OR; - case ast_operator::NULL_LOGICAL_OR: return ast_operator::NULL_LOGICAL_AND; - default: return std::nullopt; - } - }(); - if (de_morgan_op.has_value()) { + // De Morgan's laws compute exact equivalences for both Kleene (`LOGICAL_*`) and `NULL_LOGICAL_*` + // operators, including when an operand is null + if (auto const de_morgan_op = de_morgan_operator(op); de_morgan_op.has_value()) { // Negate both operands before emplacing the parent, as negating them appends to `_operators` auto const lhs = negate(operands.front().get()); auto const rhs = negate(operands.back().get()); @@ -170,15 +161,12 @@ named_to_reference_converter::push_down_negation(ast::expression const& operand) return std::reference_wrapper(_operators.back()); } - // Complement the equality operators. Exact for floating point values as well: IEEE-754 makes - // `NaN == x` false and `NaN != x` true, so the two stay exact complements. Ordering comparisons - // are excluded - every ordered comparison against a `NaN` is false, so `NOT(a < b)` is true - // exactly where `a >= b` is false + // Complement equality operators (exact for floats too) as `NaN == x` is false and `NaN != x` is + // true. Ordering comparisons (every comparison with `NaN` is false) are excluded. if (op == ast_operator::EQUAL or op == ast_operator::NOT_EQUAL) { - auto const complement = - (op == ast_operator::EQUAL) ? ast_operator::NOT_EQUAL : ast_operator::EQUAL; - auto new_operands = visit_operands(operands); - _operators.emplace_back(complement, new_operands.front(), new_operands.back()); + auto const negation = transform_operator(op).value(); + auto new_operands = visit_operands(operands); + _operators.emplace_back(negation, new_operands.front(), new_operands.back()); return std::reference_wrapper(_operators.back()); } @@ -188,10 +176,11 @@ named_to_reference_converter::push_down_negation(ast::expression const& operand) std::reference_wrapper named_to_reference_converter::negate( ast::expression const& operand) { + // Push negation down to operands if (auto const negated = push_down_negation(operand); negated.has_value()) { return negated.value(); } - // No exact rewrite, so convert the operand and negate it in place + // No exact rewrite available, so convert the operand and wrap it back in a `NOT` auto const new_operand = operand.accept(*this); _operators.emplace_back(ast::ast_operator::NOT, new_operand); return std::reference_wrapper(_operators.back()); @@ -200,20 +189,20 @@ std::reference_wrapper named_to_reference_converter::nega std::reference_wrapper named_to_reference_converter::visit( ast::operation const& expr) { - // Push the negation down to the leaves so that no downstream transformer has to reason about - // `NOT` over an expression it has already rewritten - if (expr.get_operator() == ast::ast_operator::NOT) { - if (auto const negated = push_down_negation(expr.get_operands().front().get()); - negated.has_value()) { + auto const operands = expr.get_operands(); + auto op = expr.get_operator(); + auto const operator_arity = cudf::ast::detail::ast_operator_arity(op); + + // Push down negation to leaves so that downstream transformers don't have to handle `NOT` over + // rewritten operands + if (op == ast::ast_operator::NOT) { + if (auto const negated = push_down_negation(operands.front().get()); negated.has_value()) { _converted_expr = negated.value(); return negated.value(); } } - auto const operands = expr.get_operands(); - auto op = expr.get_operator(); - auto new_operands = visit_operands(operands); - auto const operator_arity = cudf::ast::detail::ast_operator_arity(op); + auto new_operands = visit_operands(operands); if (operator_arity == 2) { _operators.emplace_back(op, new_operands.front(), new_operands.back()); } else if (operator_arity == 1) { diff --git a/cpp/src/io/parquet/expression_transform_helpers.hpp b/cpp/src/io/parquet/expression_transform_helpers.hpp index 2f7bb5123886..65dee332051f 100644 --- a/cpp/src/io/parquet/expression_transform_helpers.hpp +++ b/cpp/src/io/parquet/expression_transform_helpers.hpp @@ -88,7 +88,7 @@ enum class operator_transform : uint8_t { * untransformable operators are returned as is (no std::nullopt) */ template -[[nodiscard]] inline std::optional transform_operator(ast::ast_operator op) +[[nodiscard]] std::optional transform_operator(ast::ast_operator op) { if constexpr (mode == operator_transform::INVERT) { switch (op) { @@ -112,20 +112,33 @@ template } } +/** + * @brief Returns the De Morgan operator for the given operator + * + * @param op Operator to transform + * @return De Morgan operator or std::nullopt + */ +[[nodiscard]] std::optional de_morgan_operator(ast::ast_operator op) +{ + switch (op) { + case ast::ast_operator::LOGICAL_AND: return ast::ast_operator::LOGICAL_OR; + case ast::ast_operator::LOGICAL_OR: return ast::ast_operator::LOGICAL_AND; + case ast::ast_operator::NULL_LOGICAL_AND: return ast::ast_operator::NULL_LOGICAL_OR; + case ast::ast_operator::NULL_LOGICAL_OR: return ast::ast_operator::NULL_LOGICAL_AND; + default: return std::nullopt; + } +} + /** * @brief Handle unary operation transform for membership-based row group filters. i.e., bloom * filter and dictionary page filter. * - * A transformed operand of a membership filter is an existential summary of a row group - it - * answers "could some row here match?" rather than "is the operand true?". No unary operator is - * meaning-preserving over such a summary, so a unary operation is always relaxed to `always_true`. + * A membership test answers "might this value be present", an existential over the row group that + * is not closed under negation, so a `NOT` is relaxed to `always_true` rather than negated. + * `named_to_reference_converter::push_down_negation` rewrites `NOT(col == v)` into `col != v` + * before any converter sees it, so no negation that could be pruned should reach here. * - * `NOT` is the case that matters: transforming `NOT(col == v)` into `NOT(dict_contains(v))` would - * prune every row group holding a single `v`, whereas `col != v` only permits pruning a row group - * whose values are all `v`. See `negation_pushdown`, which rewrites `NOT(col == v)` into - * `col != v` before it ever reaches a converter. - * - * @tparam VisitOperandsFn Callable matching `(host_span>) -> + * @tparam VisitOperandsFn Callable matching `(std::span>) -> * vector>` * * @param expr Unary operation to transform @@ -256,14 +269,13 @@ class named_to_reference_converter : public ast::detail::expression_transformer * Only rewrites that are exact in every case cudf's AST evaluates are applied, as the converted * expression also filters the decoded rows: * - * - `NOT(NOT(x))` becomes `x` - * - `NOT(a AND b)` becomes `NOT(a) OR NOT(b)` and the three other De Morgan forms, for both the - * null-propagating (`LOGICAL_*`) and the Kleene (`NULL_LOGICAL_*`) operators - * - `NOT(a == b)` becomes `a != b` and vice versa - * - * Ordering comparisons are deliberately **not** complemented. IEEE-754 makes every ordered - * comparison against a `NaN` false, so `NOT(a < b)` is true exactly where `a >= b` is false. - * `NOT(IS_NULL(x))` and `NOT(NULL_EQUAL(a, b))` have no complement operator and are left alone. + * - `NOT(NOT(x))` => `x` + * - De Morgan forms: `NOT(a AND b)` => `NOT(a) OR NOT(b)` for both the null-propagating + * (`LOGICAL_*`) and the Kleene (`NULL_LOGICAL_*`) operators + * - `NOT(a == b)` => `a != b` and vice versa + * - `NOT(IS_NULL(x))` and `NOT(NULL_EQUAL(a, b))` => left alone as they have no complement + * - Ordering comparisons (`<`, `>`, `<=`, `>=`) are *not* complemented as IEEE-754 makes every + * comparison with a `NaN` false, so `NOT(a < b)` is true while `a >= b` is not * * @param operand The operand of the `NOT` operation to rewrite * @return The rewritten expression, or std::nullopt if no exact rewrite exists diff --git a/cpp/tests/io/experimental/hybrid_scan_filters_test.cpp b/cpp/tests/io/experimental/hybrid_scan_filters_test.cpp index 065239085b9b..82fdf2987719 100644 --- a/cpp/tests/io/experimental/hybrid_scan_filters_test.cpp +++ b/cpp/tests/io/experimental/hybrid_scan_filters_test.cpp @@ -489,9 +489,8 @@ TEST_F(HybridScanFiltersTest, FilterRowGroupsWithComplexExpressions) } // Filter: NOT(col0 > 50 AND col0 < 100) - // De Morgan rewrites this to NOT(col0 > 50) OR NOT(col0 < 100), i.e. col0 <= 50 OR col0 >= 100, - // stats transform: vmin <= 50 OR vmax >= 100. Every row group satisfies one of the two disjuncts, - // so nothing is pruned - but now for an evaluated reason rather than a relaxation. + // De Morgan returns col0 <= 50 OR col0 >= 100, stats transform: vmin <= 50 OR vmax >= 100. Every + // row group satisfies a disjunct, so nothing is pruned { auto literal_50_value = cudf::numeric_scalar(50, true, cudf::get_default_stream()); auto literal_50 = cudf::ast::literal(literal_50_value); @@ -511,8 +510,8 @@ TEST_F(HybridScanFiltersTest, FilterRowGroupsWithComplexExpressions) } // Filter: NOT(NOT(col0 < 100) OR col0 > 150) - // De Morgan plus double-negation elimination rewrites this to col0 < 100 AND NOT(col0 > 150), - // stats transform: vmin < 100 AND vmin <= 150. Prunes RG2 (vmin=100) and RG3 (vmin=150). + // De Morgan plus double-negation returns col0 < 100 AND NOT(col0 > 150), stats transform: + // vmin < 100 AND vmin <= 150. Prunes RG2 (vmin=100) and RG3 (vmin=150). { auto literal_100_value = cudf::numeric_scalar(100, true, cudf::get_default_stream()); auto literal_100 = cudf::ast::literal(literal_100_value); @@ -1322,25 +1321,24 @@ TEST_F(HybridScanFiltersTest, FilterRowGroupsWithDictionary) EXPECT_EQ(result, expected); } - // Filtering - NOT(table[0] == 50) - // Rewritten to table[0] != 50, which prunes a row group only when 50 is the *only* value in its - // dictionary. The row group holding 50 holds other values too, so nothing is pruned. Negating the - // membership result instead would prune that row group and drop its non-50 rows. + // Filtering - NOT(table[2] == "0100") + // Rewritten to table[2] != "0100". Every dictionary holds "0100" and nothing else, so all four + // row groups are pruned { - auto uint_literal_value = cudf::numeric_scalar(50, true, stream); - auto uint_literal = cudf::ast::literal(uint_literal_value); - auto inner = cudf::ast::operation(cudf::ast::ast_operator::EQUAL, col0_ref, uint_literal); + auto str_literal_value = cudf::string_scalar("0100", true, stream); + auto str_literal = cudf::ast::literal(str_literal_value); + auto inner = cudf::ast::operation(cudf::ast::ast_operator::EQUAL, col2_ref, str_literal); auto const filter_expression = cudf::ast::operation(cudf::ast::ast_operator::NOT, inner); auto const options = cudf::io::parquet_reader_options::builder().filter(filter_expression).build(); auto const result = filter_row_groups_with_dictionaries(datasource_ref, reader_ref, options, stream, mr); - auto const expected = std::vector{0, 1, 2, 3}; + auto const expected = std::vector{}; EXPECT_EQ(result, expected); // `NOT(col == v)` and `col != v` are the same predicate and must prune identically auto const not_equal = - cudf::ast::operation(cudf::ast::ast_operator::NOT_EQUAL, col0_ref, uint_literal); + cudf::ast::operation(cudf::ast::ast_operator::NOT_EQUAL, col2_ref, str_literal); auto const not_equal_options = cudf::io::parquet_reader_options::builder().filter(not_equal).build(); EXPECT_EQ(result, @@ -1348,48 +1346,74 @@ TEST_F(HybridScanFiltersTest, FilterRowGroupsWithDictionary) datasource_ref, reader_ref, not_equal_options, stream, mr)); } - // Filtering - NOT(table[0] == 50) AND (table[0] NULL_EQUAL 100) - // Rewritten to (table[0] != 50) AND NULL_EQUAL(...). NULL_EQUAL has no dictionary transform and - // relaxes, and table[0] != 50 prunes nothing, so all row groups survive. + // Filtering - NOT(table[0] == 50) + // Rewritten to table[0] != 50, which prunes only when 50 is the *only* dictionary value. Row + // group 1 holds 50..99, so nothing is pruned - negating the membership result instead would prune + // it and drop its non-50 rows + { + auto uint_literal_value = cudf::numeric_scalar(50, true, stream); + auto uint_literal = cudf::ast::literal(uint_literal_value); + auto inner = cudf::ast::operation(cudf::ast::ast_operator::EQUAL, col0_ref, uint_literal); + auto const filter_expression = cudf::ast::operation(cudf::ast::ast_operator::NOT, inner); + auto const options = + cudf::io::parquet_reader_options::builder().filter(filter_expression).build(); + auto const result = + filter_row_groups_with_dictionaries(datasource_ref, reader_ref, options, stream, mr); + auto const expected = std::vector{0, 1, 2, 3}; + EXPECT_EQ(result, expected); + } + + // Filtering - NOT(table[0] != 50) AND (table[0] NULL_EQUAL 100) + // Rewritten to (table[0] == 50) AND NULL_EQUAL(...). NULL_EQUAL has no dictionary transform and + // relaxes, so only the equality prunes, keeping the row group whose dictionary holds 50 { auto literal_50_value = cudf::numeric_scalar(50, true, stream); auto literal_50 = cudf::ast::literal(literal_50_value); auto literal_100_value = cudf::numeric_scalar(100, true, stream); auto literal_100 = cudf::ast::literal(literal_100_value); - auto eq_50 = cudf::ast::operation(cudf::ast::ast_operator::EQUAL, col0_ref, literal_50); - auto not_eq_50 = cudf::ast::operation(cudf::ast::ast_operator::NOT, eq_50); + auto ne_50 = cudf::ast::operation(cudf::ast::ast_operator::NOT_EQUAL, col0_ref, literal_50); + auto not_ne_50 = cudf::ast::operation(cudf::ast::ast_operator::NOT, ne_50); auto null_eq_100 = cudf::ast::operation(cudf::ast::ast_operator::NULL_EQUAL, col0_ref, literal_100); auto const filter_expression = - cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_AND, not_eq_50, null_eq_100); + cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_AND, not_ne_50, null_eq_100); auto const options = cudf::io::parquet_reader_options::builder().filter(filter_expression).build(); auto const result = filter_row_groups_with_dictionaries(datasource_ref, reader_ref, options, stream, mr); - auto const expected = std::vector{0, 1, 2, 3}; + auto const expected = std::vector{1}; EXPECT_EQ(result, expected); } - // Filtering - NOT(table[0] == 50) OR NOT(table[2] == "0100") - // Rewritten to (table[0] != 50) OR (table[2] != "0100"), matching the non-negated spelling of the - // same predicate tested above. + // Filtering - NOT((table[0] != 50) AND (table[0] != 150)) + // De Morgan and the equality complement give (table[0] == 50) OR (table[0] == 150), keeping only + // the row groups whose dictionaries hold 50 and 150 { auto literal_50_value = cudf::numeric_scalar(50, true, stream); auto literal_50 = cudf::ast::literal(literal_50_value); - auto str_literal_value = cudf::string_scalar("0100", true, stream); - auto str_literal = cudf::ast::literal(str_literal_value); - auto eq_50 = cudf::ast::operation(cudf::ast::ast_operator::EQUAL, col0_ref, literal_50); - auto not_eq_50 = cudf::ast::operation(cudf::ast::ast_operator::NOT, eq_50); - auto eq_str = cudf::ast::operation(cudf::ast::ast_operator::EQUAL, col2_ref, str_literal); - auto not_eq_str = cudf::ast::operation(cudf::ast::ast_operator::NOT, eq_str); - auto const filter_expression = - cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_OR, not_eq_50, not_eq_str); + auto literal_150_value = cudf::numeric_scalar(150, true, stream); + auto literal_150 = cudf::ast::literal(literal_150_value); + auto ne_50 = cudf::ast::operation(cudf::ast::ast_operator::NOT_EQUAL, col0_ref, literal_50); + auto ne_150 = cudf::ast::operation(cudf::ast::ast_operator::NOT_EQUAL, col0_ref, literal_150); + auto conjunction = cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_AND, ne_50, ne_150); + auto const filter_expression = cudf::ast::operation(cudf::ast::ast_operator::NOT, conjunction); auto const options = cudf::io::parquet_reader_options::builder().filter(filter_expression).build(); auto const result = filter_row_groups_with_dictionaries(datasource_ref, reader_ref, options, stream, mr); - auto const expected = std::vector{0, 1, 2, 3}; + auto const expected = std::vector{1, 3}; EXPECT_EQ(result, expected); + + // The De Morgan rewrite must prune exactly like the directly spelled disjunction + auto eq_50 = cudf::ast::operation(cudf::ast::ast_operator::EQUAL, col0_ref, literal_50); + auto eq_150 = cudf::ast::operation(cudf::ast::ast_operator::EQUAL, col0_ref, literal_150); + auto const disjunction = + cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_OR, eq_50, eq_150); + auto const disjunction_options = + cudf::io::parquet_reader_options::builder().filter(disjunction).build(); + EXPECT_EQ(result, + filter_row_groups_with_dictionaries( + datasource_ref, reader_ref, disjunction_options, stream, mr)); } } diff --git a/cpp/tests/io/parquet_reader_test.cpp b/cpp/tests/io/parquet_reader_test.cpp index a63eacb58c16..c55dc9f9aa67 100644 --- a/cpp/tests/io/parquet_reader_test.cpp +++ b/cpp/tests/io/parquet_reader_test.cpp @@ -1962,7 +1962,9 @@ TEST_F(ParquetReaderTest, FilterNegationPushdown) .stats_level(cudf::io::statistics_freq::STATISTICS_ROWGROUP); cudf::io::write_parquet(out_opts); - auto const expect_matches_unrewritten = [&](cudf::ast::expression const& filter) { + auto const expect_matches_unrewritten = [&](cudf::ast::expression const& filter, + std::optional expected_row_groups = + std::nullopt) { auto predicate = cudf::compute_column(written_table, filter); auto expected = cudf::apply_boolean_mask(written_table, *predicate); @@ -1970,6 +1972,9 @@ TEST_F(ParquetReaderTest, FilterNegationPushdown) cudf::io::parquet_reader_options::builder(cudf::io::source_info{filepath}).filter(filter); auto result = cudf::io::read_parquet(read_opts); CUDF_TEST_EXPECT_TABLES_EQUAL(*result.tbl, *expected); + if (expected_row_groups.has_value()) { + EXPECT_EQ(result.metadata.num_row_groups_after_stats_filter, expected_row_groups); + } }; auto col_ref_a = cudf::ast::column_reference(0); @@ -1994,24 +1999,27 @@ TEST_F(ParquetReaderTest, FilterNegationPushdown) auto a_lt_150 = cudf::ast::operation(cudf::ast::ast_operator::LESS, col_ref_a, lit_150); auto b_eq_10 = cudf::ast::operation(cudf::ast::ast_operator::EQUAL, col_ref_b, lit_10); - // NOT(NOT(col_a < 50)) - double negation elimination + // NOT(NOT(col_a < 50)) - double negation elimination. Becomes col_a < 50 { auto not_lt = cudf::ast::operation(cudf::ast::ast_operator::NOT, a_lt_50); - expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, not_lt)); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, not_lt), 1); } - // NOT(col_a == 10) and NOT(col_a != 10) - complemented equality - expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, a_eq_10)); - expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, a_neq_10)); + // NOT(col_a == 10) and NOT(col_a != 10) - complemented equality. col_a != 10 prunes nothing and + // col_a == 10 keeps only the first row group + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, a_eq_10), 4); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, a_neq_10), 1); // De Morgan over the null-propagating operators { + // Becomes col_a <= 50 OR col_a >= 150, all row groups kept auto conjunction = cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_AND, a_gt_50, a_lt_150); - expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, conjunction)); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, conjunction), 4); + // Becomes col_a <= 50 AND col_a != 10, only first row group kept auto disjunction = cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_OR, a_gt_50, a_eq_10); - expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, disjunction)); + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, disjunction), 1); } // De Morgan over the Kleene operators, where a null operand does not always produce a null result @@ -2054,10 +2062,9 @@ TEST_F(ParquetReaderTest, FilterNegationPushdown) // Nested negations mixing all of the above { - auto inner_or = cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_OR, a_eq_10, a_gt_50); - auto not_or = cudf::ast::operation(cudf::ast::ast_operator::NOT, inner_or); - auto outer_and = - cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_AND, not_or, a_lt_150); + auto inner_or = cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_OR, a_eq_10, a_gt_50); + auto not_or = cudf::ast::operation(cudf::ast::ast_operator::NOT, inner_or); + auto outer_and = cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_AND, not_or, a_lt_150); expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, outer_and)); } } diff --git a/python/cudf/cudf/tests/input_output/test_parquet.py b/python/cudf/cudf/tests/input_output/test_parquet.py index 67984214bc54..a6202d329ff9 100644 --- a/python/cudf/cudf/tests/input_output/test_parquet.py +++ b/python/cudf/cudf/tests/input_output/test_parquet.py @@ -4693,14 +4693,8 @@ def test_parquet_reader_mismatched_nullability_structs(tmp_path): ], ) def test_parquet_bloom_filter_negated_equality(datadir, bloom_filter_fname): - """`NOT(col == v)` must prune exactly what `col != v` prunes. + """`NOT(col == v)` must prune identically to `col != v`""" - A bloom filter answers "might this value be present", which is an existential over the - row group. Negating that answer asks "is the value definitely absent", which is a - different question: it prunes every row group holding a single `v`, dropping the rows - that do satisfy `col != v`. These files carry bloom filters and no column chunk - statistics, so the bloom filter is the only thing that can prune here. - """ import pylibcudf as plc from pylibcudf.expressions import ( ASTOperator, @@ -4721,15 +4715,11 @@ def read_with(filter_expr): negated_equality = read_with( Operation( ASTOperator.NOT, - Operation( - ASTOperator.EQUAL, ColumnNameReference("str"), needle - ), + Operation(ASTOperator.EQUAL, ColumnNameReference("str"), needle), ) ) not_equal = read_with( - Operation( - ASTOperator.NOT_EQUAL, ColumnNameReference("str"), needle - ) + Operation(ASTOperator.NOT_EQUAL, ColumnNameReference("str"), needle) ) # The two spellings are the same predicate and must prune identically @@ -4739,8 +4729,7 @@ def read_with(filter_expr): ) assert negated_equality.tbl.to_arrow().equals(not_equal.tbl.to_arrow()) - # Both must keep every row that is not "FINDME". Negating the bloom filter result - # instead prunes the two row groups that hold a "FINDME" and returns only 600. + # 998 of the 1000 rows are not "FINDME". assert negated_equality.tbl.num_rows() == 998 From 6c3e55231c452210ddfa76589ef6e97873e5b510 Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Fri, 7 Aug 2026 20:46:16 +0000 Subject: [PATCH 4/9] Add more tests --- .../tests/io/test_experimental_hybrid_scan.py | 178 ++++++++++++++++++ 1 file changed, 178 insertions(+) diff --git a/python/pylibcudf/tests/io/test_experimental_hybrid_scan.py b/python/pylibcudf/tests/io/test_experimental_hybrid_scan.py index 7bf3a19e1d13..9ba1473a8da9 100644 --- a/python/pylibcudf/tests/io/test_experimental_hybrid_scan.py +++ b/python/pylibcudf/tests/io/test_experimental_hybrid_scan.py @@ -708,6 +708,184 @@ def test_hybrid_scan_construct_row_group_passes( ) +def _col0_stats_negation_cases() -> list[ + tuple[str, Operation, Operation, list[int]] +]: + """Cases of (id, negated, equivalent unnegated, surviving row groups).""" + + def _literal(value: int) -> Literal: + return Literal( + plc.Scalar.from_arrow(pa.scalar(value, type=pa.uint32())) + ) + + lt_250 = Operation( + ASTOperator.LESS, ColumnNameReference("col0"), _literal(250) + ) + ge_250 = Operation( + ASTOperator.GREATER_EQUAL, + ColumnNameReference("col0"), + _literal(250), + ) + lt_500 = Operation( + ASTOperator.LESS, ColumnNameReference("col0"), _literal(500) + ) + ge_500 = Operation( + ASTOperator.GREATER_EQUAL, + ColumnNameReference("col0"), + _literal(500), + ) + lt_750 = Operation( + ASTOperator.LESS, ColumnNameReference("col0"), _literal(750) + ) + ge_750 = Operation( + ASTOperator.GREATER_EQUAL, + ColumnNameReference("col0"), + _literal(750), + ) + eq_100 = Operation( + ASTOperator.EQUAL, ColumnNameReference("col0"), _literal(100) + ) + ne_100 = Operation( + ASTOperator.NOT_EQUAL, + ColumnNameReference("col0"), + _literal(100), + ) + + return [ + # Collapses to col0 < 250 + ( + "double_negation", + Operation(ASTOperator.NOT, Operation(ASTOperator.NOT, lt_250)), + lt_250, + [0], + ), + # Becomes col0 == 100 + ( + "not_equal_to_equal", + Operation(ASTOperator.NOT, ne_100), + eq_100, + [0], + ), + # Becomes col0 != 100, which stats cannot prune with + ( + "equal_to_not_equal", + Operation(ASTOperator.NOT, eq_100), + ne_100, + [0, 1, 2, 3], + ), + # De Morgan over AND, giving col0 < 250 OR col0 >= 500 + ( + "de_morgan_and", + Operation( + ASTOperator.NOT, + Operation(ASTOperator.LOGICAL_AND, ge_250, lt_500), + ), + Operation(ASTOperator.LOGICAL_OR, lt_250, ge_500), + [0, 2, 3], + ), + # De Morgan over OR, giving col0 >= 250 AND col0 < 750 + ( + "de_morgan_or", + Operation( + ASTOperator.NOT, + Operation(ASTOperator.LOGICAL_OR, lt_250, ge_750), + ), + Operation(ASTOperator.LOGICAL_AND, ge_250, lt_750), + [1, 2], + ), + ] + + +@pytest.mark.parametrize( + "negated,unnegated,expected", + [ + pytest.param(negated, unnegated, expected, id=name) + for name, negated, unnegated, expected in _col0_stats_negation_cases() + ], +) +def test_hybrid_scan_filter_row_groups_with_stats_negation( + simple_hybrid_scan_reader: HybridScanReader, + simple_parquet_options: plc.io.parquet.ParquetReaderOptions, + negated: Operation, + unnegated: Operation, + expected: list[int], +) -> None: + """A negated filter must prune exactly like its unnegated equivalent.""" + + reader = simple_hybrid_scan_reader + + def prune(filter_expression: Operation) -> list[int]: + reader.reset_column_selection() + simple_parquet_options.set_filter(filter_expression) + all_row_groups = reader.all_row_groups(simple_parquet_options) + return reader.filter_row_groups_with_stats( + all_row_groups, simple_parquet_options + ) + + assert prune(negated) == expected + assert prune(unnegated) == expected + + +@pytest.mark.parametrize( + "negated,unnegated,expected", + [ + # col1 holds 250 distinct strings per row group, so `col1 != v` never + # sees a dictionary made up only of `v` and cannot prune + pytest.param( + ASTOperator.EQUAL, + ASTOperator.NOT_EQUAL, + [0, 1, 2, 3], + id="equal_to_not_equal", + ), + # `col1 == "str_0"` keeps only the row group whose dictionary holds it + pytest.param( + ASTOperator.NOT_EQUAL, + ASTOperator.EQUAL, + [0], + id="not_equal_to_equal", + ), + ], +) +def test_hybrid_scan_filter_row_groups_with_dictionary_pages_negation( + simple_parquet_bytes: bytes, + simple_hybrid_scan_reader: HybridScanReader, + simple_parquet_options: plc.io.parquet.ParquetReaderOptions, + negated: ASTOperator, + unnegated: ASTOperator, + expected: list[int], +) -> None: + """`NOT(col == v)` must prune what `col != v` prunes, and converse.""" + + col1 = ColumnNameReference("col1") + needle = Literal(plc.Scalar.from_arrow(pa.scalar("str_0"))) + reader = simple_hybrid_scan_reader + + def prune(filter_expression: Operation) -> list[int]: + reader.reset_column_selection() + simple_parquet_options.set_filter(filter_expression) + all_row_groups = reader.all_row_groups(simple_parquet_options) + _, dictionary_ranges = reader.secondary_filters_byte_ranges( + all_row_groups, simple_parquet_options + ) + dictionary_data = [ + plc.gpumemoryview( + rmm.DeviceBuffer.to_device( + simple_parquet_bytes[r.offset : r.offset + r.size], + plc.utils._get_stream(), + ) + ) + for r in dictionary_ranges + ] + synchronize_stream() + return reader.filter_row_groups_with_dictionary_pages( + dictionary_data, all_row_groups, simple_parquet_options + ) + + inner = Operation(negated, col1, needle) + assert prune(Operation(ASTOperator.NOT, inner)) == expected + assert prune(Operation(unnegated, col1, needle)) == expected + + def test_hybrid_scan_metadata_with_page_index( simple_parquet_bytes: bytes, simple_hybrid_scan_reader: HybridScanReader, From 224b87ee0cc0e66212f3332dcbc88dad424b630d Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Fri, 7 Aug 2026 20:52:56 +0000 Subject: [PATCH 5/9] Minor fix --- .../parquet/expression_transform_helpers.cpp | 41 +++++++++++++++++++ .../parquet/expression_transform_helpers.hpp | 35 +--------------- 2 files changed, 43 insertions(+), 33 deletions(-) diff --git a/cpp/src/io/parquet/expression_transform_helpers.cpp b/cpp/src/io/parquet/expression_transform_helpers.cpp index 803982876851..3d5da45c9fcb 100644 --- a/cpp/src/io/parquet/expression_transform_helpers.cpp +++ b/cpp/src/io/parquet/expression_transform_helpers.cpp @@ -47,6 +47,47 @@ namespace { } // namespace +template +std::optional transform_operator(ast::ast_operator op) +{ + if constexpr (mode == operator_transform::INVERT) { + switch (op) { + case ast::ast_operator::LESS: return ast::ast_operator::GREATER; + case ast::ast_operator::GREATER: return ast::ast_operator::LESS; + case ast::ast_operator::LESS_EQUAL: return ast::ast_operator::GREATER_EQUAL; + case ast::ast_operator::GREATER_EQUAL: return ast::ast_operator::LESS_EQUAL; + default: return std::make_optional(op); + } + } 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; + case ast::ast_operator::LESS_EQUAL: return ast::ast_operator::GREATER; + case ast::ast_operator::GREATER_EQUAL: return ast::ast_operator::LESS; + case ast::ast_operator::EQUAL: return ast::ast_operator::NOT_EQUAL; + case ast::ast_operator::NOT_EQUAL: return ast::ast_operator::EQUAL; + default: return std::nullopt; + } + } +} + +template std::optional transform_operator( + ast::ast_operator op); +template std::optional transform_operator( + ast::ast_operator op); + +std::optional de_morgan_operator(ast::ast_operator op) +{ + switch (op) { + case ast::ast_operator::LOGICAL_AND: return ast::ast_operator::LOGICAL_OR; + case ast::ast_operator::LOGICAL_OR: return ast::ast_operator::LOGICAL_AND; + case ast::ast_operator::NULL_LOGICAL_AND: return ast::ast_operator::NULL_LOGICAL_OR; + case ast::ast_operator::NULL_LOGICAL_OR: return ast::ast_operator::NULL_LOGICAL_AND; + default: return std::nullopt; + } +} + unary_operand extract_unary_operand(ast::operation const& expr) { auto const& operands = expr.get_operands(); diff --git a/cpp/src/io/parquet/expression_transform_helpers.hpp b/cpp/src/io/parquet/expression_transform_helpers.hpp index 65dee332051f..49796317450f 100644 --- a/cpp/src/io/parquet/expression_transform_helpers.hpp +++ b/cpp/src/io/parquet/expression_transform_helpers.hpp @@ -88,29 +88,7 @@ enum class operator_transform : uint8_t { * untransformable operators are returned as is (no std::nullopt) */ template -[[nodiscard]] std::optional transform_operator(ast::ast_operator op) -{ - if constexpr (mode == operator_transform::INVERT) { - switch (op) { - case ast::ast_operator::LESS: return ast::ast_operator::GREATER; - case ast::ast_operator::GREATER: return ast::ast_operator::LESS; - case ast::ast_operator::LESS_EQUAL: return ast::ast_operator::GREATER_EQUAL; - case ast::ast_operator::GREATER_EQUAL: return ast::ast_operator::LESS_EQUAL; - default: return std::make_optional(op); - } - } 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; - case ast::ast_operator::LESS_EQUAL: return ast::ast_operator::GREATER; - case ast::ast_operator::GREATER_EQUAL: return ast::ast_operator::LESS; - case ast::ast_operator::EQUAL: return ast::ast_operator::NOT_EQUAL; - case ast::ast_operator::NOT_EQUAL: return ast::ast_operator::EQUAL; - default: return std::nullopt; - } - } -} +[[nodiscard]] std::optional transform_operator(ast::ast_operator op); /** * @brief Returns the De Morgan operator for the given operator @@ -118,16 +96,7 @@ template * @param op Operator to transform * @return De Morgan operator or std::nullopt */ -[[nodiscard]] std::optional de_morgan_operator(ast::ast_operator op) -{ - switch (op) { - case ast::ast_operator::LOGICAL_AND: return ast::ast_operator::LOGICAL_OR; - case ast::ast_operator::LOGICAL_OR: return ast::ast_operator::LOGICAL_AND; - case ast::ast_operator::NULL_LOGICAL_AND: return ast::ast_operator::NULL_LOGICAL_OR; - case ast::ast_operator::NULL_LOGICAL_OR: return ast::ast_operator::NULL_LOGICAL_AND; - default: return std::nullopt; - } -} +[[nodiscard]] std::optional de_morgan_operator(ast::ast_operator op); /** * @brief Handle unary operation transform for membership-based row group filters. i.e., bloom From 1b56bad2c87f7eeecd452b08d1d295d840f77f9f Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Fri, 7 Aug 2026 21:36:07 +0000 Subject: [PATCH 6/9] Refactor to fix class names --- .../io/parquet/parquet_reader_metadata.cpp | 6 +- .../ast/detail/expression_transformer.hpp | 12 ++- cpp/src/ast/expressions.cpp | 12 +++ cpp/src/io/parquet/bloom_filter_reader.cu | 12 --- .../experimental/hybrid_scan_helpers.cpp | 7 +- .../experimental/hybrid_scan_helpers.hpp | 19 ++-- .../parquet/experimental/hybrid_scan_impl.cpp | 38 +++---- .../parquet/experimental/hybrid_scan_impl.hpp | 14 +-- .../parquet/expression_transform_helpers.cpp | 100 +++++++++--------- .../parquet/expression_transform_helpers.hpp | 62 +++++------ cpp/src/io/parquet/reader_impl.cpp | 5 +- cpp/src/io/parquet/reader_impl.hpp | 4 +- cpp/src/io/parquet/stats_filter_helpers.cpp | 14 +-- cpp/src/io/parquet/stats_filter_helpers.hpp | 3 - 14 files changed, 150 insertions(+), 158 deletions(-) diff --git a/cpp/benchmarks/io/parquet/parquet_reader_metadata.cpp b/cpp/benchmarks/io/parquet/parquet_reader_metadata.cpp index 28f5ada2adf8..a183ac65d5d1 100644 --- a/cpp/benchmarks/io/parquet/parquet_reader_metadata.cpp +++ b/cpp/benchmarks/io/parquet/parquet_reader_metadata.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION. + * SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 */ @@ -309,7 +309,7 @@ void BM_parquet_filter_name_resolution(nvbench::state& state) auto constexpr pass_read_limit = 0; // No column projection is requested, so the reader reads all columns; this isolates filter name - // resolution (named_to_reference_converter) from select_columns name scanning. + // resolution (`parquet_filter_normalizer`) from select_columns name scanning. auto read_opts = cudf::io::parquet_reader_options::builder(source_sink.make_source_info()) .use_arrow_schema(false) .build(); @@ -326,7 +326,7 @@ void BM_parquet_filter_name_resolution(nvbench::state& state) auto metadatas = cudf::io::read_parquet_footers(sources); // Reader construction resolves all referenced filter column names - // (named_to_reference_converter) and runs column selection. Construction throws if a + // (`parquet_filter_normalizer`) and runs column selection. Construction throws if a // referenced name is missing, so successful construction is the validation; has_next() is // intentionally not called so per-sample timing is not perturbed by row-group filter // evaluation over the wide predicate. diff --git a/cpp/include/cudf/ast/detail/expression_transformer.hpp b/cpp/include/cudf/ast/detail/expression_transformer.hpp index 012fb3958871..55167039f2f7 100644 --- a/cpp/include/cudf/ast/detail/expression_transformer.hpp +++ b/cpp/include/cudf/ast/detail/expression_transformer.hpp @@ -1,6 +1,6 @@ /* - * SPDX-FileCopyrightText: Copyright (c) 2023-2024, NVIDIA CORPORATION. + * SPDX-FileCopyrightText: Copyright (c) 2023-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 */ #pragma once @@ -50,6 +50,16 @@ class expression_transformer { virtual std::reference_wrapper visit(column_name_reference const& expr) = 0; virtual ~expression_transformer() {} + + protected: + /** + * @brief Visits each expression in `operands`. + * + * @param operands Expressions to visit + * @return References to transformed expressions + */ + [[nodiscard]] std::vector> visit_operands( + std::vector> const& operands); }; } // namespace ast::detail diff --git a/cpp/src/ast/expressions.cpp b/cpp/src/ast/expressions.cpp index f3126fa49cbd..f1ca81cd15b0 100644 --- a/cpp/src/ast/expressions.cpp +++ b/cpp/src/ast/expressions.cpp @@ -31,6 +31,18 @@ operation::operation(ast_operator op, expression const& left, expression const& std::invalid_argument); } +std::vector> +detail::expression_transformer::visit_operands( + std::vector> const& operands) +{ + std::vector> transformed_operands; + transformed_operands.reserve(operands.size()); + for (auto const& operand : operands) { + transformed_operands.push_back(operand.get().accept(*this)); + } + return transformed_operands; +} + cudf::size_type literal::accept(detail::expression_parser& visitor) const { return visitor.visit(*this); diff --git a/cpp/src/io/parquet/bloom_filter_reader.cu b/cpp/src/io/parquet/bloom_filter_reader.cu index b0a651390376..07a8afb0eff9 100644 --- a/cpp/src/io/parquet/bloom_filter_reader.cu +++ b/cpp/src/io/parquet/bloom_filter_reader.cu @@ -556,16 +556,4 @@ std::vector> equality_literals_collector::get_literal return std::move(_literals); } -std::vector> -equality_literals_collector::visit_operands( - cudf::host_span const> operands) -{ - std::vector> transformed_operands; - for (auto const& operand : operands) { - auto const new_operand = operand.get().accept(*this); - transformed_operands.push_back(new_operand); - } - return transformed_operands; -} - } // namespace cudf::io::parquet::detail diff --git a/cpp/src/io/parquet/experimental/hybrid_scan_helpers.cpp b/cpp/src/io/parquet/experimental/hybrid_scan_helpers.cpp index b3ba5c5aaeee..9edc67436f68 100644 --- a/cpp/src/io/parquet/experimental/hybrid_scan_helpers.cpp +++ b/cpp/src/io/parquet/experimental/hybrid_scan_helpers.cpp @@ -725,9 +725,10 @@ aggregate_reader_metadata::filter_row_groups_with_bloom_filters( } /** - * @brief Converts column named expression to column index reference expression + * @brief Converts named columns to index reference columns and pushes logical negations down to + * expression leaves */ -named_to_reference_converter::named_to_reference_converter( +parquet_filter_normalizer::parquet_filter_normalizer( std::optional> expr, table_metadata const& metadata, std::vector const& schema_tree, @@ -749,7 +750,7 @@ named_to_reference_converter::named_to_reference_converter( expr.value().get().accept(*this); } -std::reference_wrapper named_to_reference_converter::visit( +std::reference_wrapper parquet_filter_normalizer::visit( ast::column_reference const& expr) { // Map the column index to its name diff --git a/cpp/src/io/parquet/experimental/hybrid_scan_helpers.hpp b/cpp/src/io/parquet/experimental/hybrid_scan_helpers.hpp index 92287b55bb7a..eed14211bd0b 100644 --- a/cpp/src/io/parquet/experimental/hybrid_scan_helpers.hpp +++ b/cpp/src/io/parquet/experimental/hybrid_scan_helpers.hpp @@ -399,19 +399,20 @@ class dictionary_literals_collector : public equality_literals_collector { }; /** - * @brief Converts named columns to index reference columns + * @brief Converts named columns to index reference columns and pushes logical negations down to + * expression leaves */ -class named_to_reference_converter : public parquet::detail::named_to_reference_converter { +class parquet_filter_normalizer : public parquet::detail::parquet_filter_normalizer { public: - named_to_reference_converter() = default; + parquet_filter_normalizer() = default; - named_to_reference_converter(std::optional> expr, - table_metadata const& metadata, - std::vector const& schema_tree, - cudf::io::parquet_reader_options const& options, - bool case_sensitive_names); + parquet_filter_normalizer(std::optional> expr, + table_metadata const& metadata, + std::vector const& schema_tree, + cudf::io::parquet_reader_options const& options, + bool case_sensitive_names); - using parquet::detail::named_to_reference_converter::visit; + using parquet::detail::parquet_filter_normalizer::visit; /** * @copydoc ast::detail::expression_transformer::visit(ast::column_reference const& ) diff --git a/cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp b/cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp index aab25c0e648b..738e267b06d7 100644 --- a/cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp +++ b/cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp @@ -221,15 +221,15 @@ void hybrid_scan_reader_impl::reset_column_selection() _is_payload_columns_selected = false; } -std::pair> +std::pair> hybrid_scan_reader_impl::prepare_filter_and_output_types(parquet_reader_options const& options) { CUDF_EXPECTS(options.get_filter().has_value(), "Empty input filter expression encountered"); select_columns(read_columns_mode::FILTER_COLUMNS, options); - // Convert the input expression (must be done after column selection) - auto expr_conv = build_converted_expression(options); + // Normalize the input expression (must be done after column selection) + auto expr_conv = build_normalized_expression(options); auto output_dtypes = get_output_types(_output_buffers_template); return {std::move(expr_conv), std::move(output_dtypes)}; @@ -514,8 +514,8 @@ table_with_metadata hybrid_scan_reader_impl::materialize_filter_columns( prepare_materialization( read_columns_mode::FILTER_COLUMNS, row_group_indices.size(), options, stream, mr); - // Convert the input expression (must be done after prepare_materialization) - _expr_conv = build_converted_expression(options); + // Normalize the input expression (must be done after prepare_materialization) + _expr_conv = build_normalized_expression(options); // Return early if all rows are pruned if (are_all_rows_pruned(row_mask, stream)) { @@ -588,8 +588,8 @@ table_with_metadata hybrid_scan_reader_impl::materialize_all_columns( prepare_materialization( read_columns_mode::ALL_COLUMNS, row_group_indices.size(), options, stream, mr); - // Convert the input expression (must be done after prepare_materialization) - _expr_conv = build_converted_expression(options); + // Normalize the input expression after materialization preparation. + _expr_conv = build_normalized_expression(options); prepare_data(read_mode::READ_ALL, row_group_indices, column_chunk_data, {}); @@ -623,8 +623,8 @@ void hybrid_scan_reader_impl::setup_chunking_for_filter_columns( _input_pass_read_limit = pass_read_limit; _output_chunk_read_limit = chunk_read_limit; - // Convert the input expression (must be done after prepare_materialization) - _expr_conv = build_converted_expression(options); + // Normalize the input expression (must be done after prepare_materialization) + _expr_conv = build_normalized_expression(options); // Return early if all rows are pruned if (are_all_rows_pruned(row_mask, stream)) { @@ -740,8 +740,8 @@ void hybrid_scan_reader_impl::setup_chunking_for_all_columns( _input_pass_read_limit = pass_read_limit; _output_chunk_read_limit = chunk_read_limit; - // Convert the input expression (must be done after column selection) - _expr_conv = build_converted_expression(options); + // Normalize the input expression (must be done after column selection) + _expr_conv = build_normalized_expression(options); prepare_data(read_mode::CHUNKED_READ, row_group_indices, column_chunk_data, {}); } @@ -880,7 +880,7 @@ void hybrid_scan_reader_impl::reset_internal_state() _output_chunk_read_limit = 0; _strings_to_categorical = false; _reader_column_schema.reset(); - _expr_conv = named_to_reference_converter{}; + _expr_conv = parquet_filter_normalizer{}; _mr = cudf::get_current_device_resource_ref(); } @@ -914,18 +914,18 @@ void hybrid_scan_reader_impl::initialize_options(parquet_reader_options const& o _mr = mr; } -named_to_reference_converter hybrid_scan_reader_impl::build_converted_expression( +parquet_filter_normalizer hybrid_scan_reader_impl::build_normalized_expression( parquet_reader_options const& options) { - if (not options.get_filter().has_value()) { return named_to_reference_converter{}; } + if (not options.get_filter().has_value()) { return parquet_filter_normalizer{}; } table_metadata metadata; populate_metadata(metadata); - auto expr_conv = named_to_reference_converter(options.get_filter(), - metadata, - _extended_metadata->get_schema_tree(), - options, - options.is_enabled_case_sensitive_names()); + auto expr_conv = parquet_filter_normalizer(options.get_filter(), + metadata, + _extended_metadata->get_schema_tree(), + options, + options.is_enabled_case_sensitive_names()); CUDF_EXPECTS(expr_conv.get_converted_expr().has_value(), "Columns names in filter expression must be convertible to index references"); return expr_conv; diff --git a/cpp/src/io/parquet/experimental/hybrid_scan_impl.hpp b/cpp/src/io/parquet/experimental/hybrid_scan_impl.hpp index 3dd241af35ff..f67f4786902e 100644 --- a/cpp/src/io/parquet/experimental/hybrid_scan_impl.hpp +++ b/cpp/src/io/parquet/experimental/hybrid_scan_impl.hpp @@ -335,13 +335,13 @@ class hybrid_scan_reader_impl : public parquet::detail::reader_impl { rmm::device_async_resource_ref mr); /** - * @brief Convert the input filter expression such that all column name references are replaced - * with corresponding column references + * @brief Normalize input filter such that all column names are converted to index references and + * logical negations are pushed down to the leaves. * * @param options Reader options - * @return Converted expression + * @return Filter expression normalizer */ - [[nodiscard]] named_to_reference_converter build_converted_expression( + [[nodiscard]] parquet_filter_normalizer build_normalized_expression( parquet_reader_options const& options); /** @@ -370,12 +370,12 @@ class hybrid_scan_reader_impl : public parquet::detail::reader_impl { std::span const> row_group_indices) const; /** - * @brief Helper to prepare converted filter expression and output column data types + * @brief Helper to prepare a normalized filter expression and output column data types * * @param options Parquet reader options - * @return A pair of a converted filter expression and a vector of output column data types + * @return A pair of filter expression normalizer and output column data types */ - std::pair> + std::pair> prepare_filter_and_output_types(parquet_reader_options const& options); /** diff --git a/cpp/src/io/parquet/expression_transform_helpers.cpp b/cpp/src/io/parquet/expression_transform_helpers.cpp index 3d5da45c9fcb..ecd4613aedef 100644 --- a/cpp/src/io/parquet/expression_transform_helpers.cpp +++ b/cpp/src/io/parquet/expression_transform_helpers.cpp @@ -130,7 +130,7 @@ binary_operands extract_binary_operands(ast::operation const& expr) } } -named_to_reference_converter::named_to_reference_converter( +parquet_filter_normalizer::parquet_filter_normalizer( std::optional> expr, table_metadata const& metadata, bool case_sensitive_names) @@ -147,21 +147,21 @@ named_to_reference_converter::named_to_reference_converter( expr.value().get().accept(*this); } -std::reference_wrapper named_to_reference_converter::visit( +std::reference_wrapper parquet_filter_normalizer::visit( ast::literal const& expr) { _converted_expr = std::reference_wrapper(expr); return expr; } -std::reference_wrapper named_to_reference_converter::visit( +std::reference_wrapper parquet_filter_normalizer::visit( ast::column_reference const& expr) { _converted_expr = std::reference_wrapper(expr); return expr; } -std::reference_wrapper named_to_reference_converter::visit( +std::reference_wrapper parquet_filter_normalizer::visit( ast::column_name_reference const& expr) { // check if column name is in metadata @@ -179,7 +179,7 @@ std::reference_wrapper named_to_reference_converter::visi } std::optional> -named_to_reference_converter::push_down_negation(ast::expression const& operand) +parquet_filter_normalizer::push_down_negation(ast::expression const& operand) { using cudf::ast::ast_operator; @@ -214,7 +214,7 @@ named_to_reference_converter::push_down_negation(ast::expression const& operand) return std::nullopt; } -std::reference_wrapper named_to_reference_converter::negate( +std::reference_wrapper parquet_filter_normalizer::negate( ast::expression const& operand) { // Push negation down to operands @@ -227,7 +227,7 @@ std::reference_wrapper named_to_reference_converter::nega return std::reference_wrapper(_operators.back()); } -std::reference_wrapper named_to_reference_converter::visit( +std::reference_wrapper parquet_filter_normalizer::visit( ast::operation const& expr) { auto const operands = expr.get_operands(); @@ -253,18 +253,6 @@ std::reference_wrapper named_to_reference_converter::visi return std::reference_wrapper(_operators.back()); } -std::vector> -named_to_reference_converter::visit_operands( - cudf::host_span const> operands) -{ - std::vector> transformed_operands; - for (auto const& operand : operands) { - auto const new_operand = operand.get().accept(*this); - transformed_operands.push_back(new_operand); - } - return transformed_operands; -} - names_from_expression::names_from_expression( std::optional> expr, std::vector const& skip_names, @@ -319,7 +307,7 @@ std::reference_wrapper names_from_expression::visit( std::reference_wrapper names_from_expression::visit( ast::operation const& expr) { - visit_operands(expr.get_operands()); + std::ignore = visit_operands(expr.get_operands()); return expr; } @@ -329,12 +317,50 @@ std::vector names_from_expression::to_vector() && std::make_move_iterator(_column_names.end())}; } -void names_from_expression::visit_operands( - cudf::host_span const> operands) +offset_column_references::offset_column_references( + std::optional> expr, size_type offset) + : _offset{offset} { - for (auto const& operand : operands) { - operand.get().accept(*this); + if (not expr.has_value()) { return; } + if (offset == 0) { + _converted_expr = expr; + return; } + _converted_expr = expr.value().get().accept(*this); +} + +std::optional> +offset_column_references::get_converted_expr() const +{ + return _converted_expr; +} + +std::reference_wrapper offset_column_references::visit( + ast::literal const& expr) +{ + return expr; +} + +std::reference_wrapper offset_column_references::visit( + ast::column_reference const& expr) +{ + return _tree.push( + ast::column_reference{expr.get_column_index() + _offset, expr.get_table_source()}); +} + +std::reference_wrapper offset_column_references::visit( + ast::operation const& expr) +{ + auto const new_operands = visit_operands(expr.get_operands()); + auto const arity = cudf::ast::detail::ast_operator_arity(expr.get_operator()); + if (arity == 1) { return _tree.push(ast::operation{expr.get_operator(), new_operands.front()}); } + return _tree.push(ast::operation{expr.get_operator(), new_operands.front(), new_operands.back()}); +} + +std::reference_wrapper offset_column_references::visit( + ast::column_name_reference const&) +{ + CUDF_FAIL("Column name references are not supported in column reference offsetter"); } [[nodiscard]] std::unordered_map map_column_indices_to_names( @@ -484,30 +510,4 @@ std::optional>> collect_filtered_row_group_in return {filtered_row_group_indices}; } -offset_column_references::offset_column_references( - std::optional> expr, size_type offset) - : _offset{offset} -{ - if (!expr.has_value()) { return; } - - if (offset == 0) { - _converted_expr = expr; - return; - } - _converted_expr = expr.value().get().accept(*this); -} - -std::reference_wrapper offset_column_references::visit( - ast::column_reference const& expr) -{ - _col_ref.emplace_back(expr.get_column_index() + _offset, expr.get_table_source()); - return _col_ref.back(); -} - -std::reference_wrapper offset_column_references::visit( - ast::column_name_reference const&) -{ - CUDF_FAIL("Column name references are not supported in offset_column_references"); -} - } // namespace cudf::io::parquet::detail diff --git a/cpp/src/io/parquet/expression_transform_helpers.hpp b/cpp/src/io/parquet/expression_transform_helpers.hpp index 49796317450f..20a437f90380 100644 --- a/cpp/src/io/parquet/expression_transform_helpers.hpp +++ b/cpp/src/io/parquet/expression_transform_helpers.hpp @@ -102,11 +102,6 @@ template * @brief Handle unary operation transform for membership-based row group filters. i.e., bloom * filter and dictionary page filter. * - * A membership test answers "might this value be present", an existential over the row group that - * is not closed under negation, so a `NOT` is relaxed to `always_true` rather than negated. - * `named_to_reference_converter::push_down_negation` rewrites `NOT(col == v)` into `col != v` - * before any converter sees it, so no negation that could be pruned should reach here. - * * @tparam VisitOperandsFn Callable matching `(std::span>) -> * vector>` * @@ -171,29 +166,22 @@ class names_from_expression : public ast::detail::expression_transformer { [[nodiscard]] std::vector to_vector() &&; private: - void visit_operands( - cudf::host_span const> operands); - std::unordered_map _column_indices_to_names; std::unordered_set _column_names; column_path_set _skip_names; }; /** - * @brief Converts named columns to index reference columns and pushes logical negations down to - * the leaves of the expression - * - * The converted expression is the single expression the reader uses both to prune row groups and - * pages, and to filter the decoded rows. Every negation rewrite must therefore be an exact - * equivalence rather than a relaxation - see `push_down_negation()`. + * @brief Converts named columns to index reference columns and pushes logical negations down to the + * leaves of the expression. */ -class named_to_reference_converter : public ast::detail::expression_transformer { +class parquet_filter_normalizer : public ast::detail::expression_transformer { public: - named_to_reference_converter() = default; + parquet_filter_normalizer() = default; - named_to_reference_converter(std::optional> expr, - table_metadata const& metadata, - bool case_sensitive_names); + parquet_filter_normalizer(std::optional> expr, + table_metadata const& metadata, + bool case_sensitive_names); /** * @copydoc ast::detail::expression_transformer::visit(ast::literal const& ) @@ -219,18 +207,12 @@ class named_to_reference_converter : public ast::detail::expression_transformer /** * @brief Returns the converted AST expression * - * @return AST operation expression + * @return Converted expression, if an input expression was provided */ [[nodiscard]] std::optional> get_converted_expr() - const - { - return _converted_expr; - } + const; protected: - std::vector> visit_operands( - cudf::host_span const> operands); - /** * @brief Rewrites `NOT(operand)` into an equivalent expression with the negation pushed into * `operand`'s own operands @@ -308,9 +290,6 @@ class equality_literals_collector : public ast::detail::expression_transformer { [[nodiscard]] std::vector> get_literals() &&; protected: - std::vector> visit_operands( - cudf::host_span const> operands); - cudf::host_span _output_dtypes; std::vector> _literals; @@ -323,26 +302,43 @@ class equality_literals_collector : public ast::detail::expression_transformer { * @brief Offsets every column referencein an expression by the specified value * */ -class offset_column_references : public named_to_reference_converter { +class offset_column_references : public ast::detail::expression_transformer { public: offset_column_references(std::optional> expr, size_type offset); - // Use `visit` overloads from named_to_reference_converter - using named_to_reference_converter::visit; + /** + * @copydoc ast::detail::expression_transformer::visit(ast::literal const& ) + */ + std::reference_wrapper visit(ast::literal const& expr) override; /** * @copydoc ast::detail::expression_transformer::visit(ast::column_reference const& ) */ std::reference_wrapper visit(ast::column_reference const& expr) override; + /** + * @copydoc ast::detail::expression_transformer::visit(ast::operation const& ) + */ + std::reference_wrapper visit(ast::operation const& expr) override; + /** * @copydoc ast::detail::expression_transformer::visit(ast::column_name_reference const& ) */ std::reference_wrapper visit( ast::column_name_reference const& expr) override; + /** + * @brief Returns the converted AST expression + * + * @return Converted expression, if an input expression was provided + */ + [[nodiscard]] std::optional> get_converted_expr() + const; + private: + ast::tree _tree; + std::optional> _converted_expr; size_type _offset{0}; }; diff --git a/cpp/src/io/parquet/reader_impl.cpp b/cpp/src/io/parquet/reader_impl.cpp index 307015ec2c3f..76558d81158b 100644 --- a/cpp/src/io/parquet/reader_impl.cpp +++ b/cpp/src/io/parquet/reader_impl.cpp @@ -575,12 +575,11 @@ reader_impl::reader_impl(std::size_t chunk_read_limit, std::back_inserter(_output_buffers_template), [](auto const& buff) { return cudf::io::detail::inline_column_buffer::empty_like(buff); }); - // Save the name to reference converter to extract output filter AST in - // `preprocess_file()` and `finalize_output()` + // Save the normalized output filter for `preprocess_file()` and `finalize_output()`. table_metadata metadata; populate_metadata(metadata); _expr_conv = - named_to_reference_converter(options.get_filter(), metadata, _options.case_sensitive_names); + parquet_filter_normalizer(options.get_filter(), metadata, _options.case_sensitive_names); } void reader_impl::prepare_data(read_mode mode) diff --git a/cpp/src/io/parquet/reader_impl.hpp b/cpp/src/io/parquet/reader_impl.hpp index 76d88f52e310..94421670fd55 100644 --- a/cpp/src/io/parquet/reader_impl.hpp +++ b/cpp/src/io/parquet/reader_impl.hpp @@ -544,8 +544,8 @@ class reader_impl { bool prepend_row_index_column = false; } _options; - // name to reference converter to extract AST output filter - named_to_reference_converter _expr_conv{std::nullopt, table_metadata{}, true}; + // Converts the input filter to AST output filter. + parquet_filter_normalizer _expr_conv{std::nullopt, table_metadata{}, true}; std::vector> _sources; std::unique_ptr _metadata; diff --git a/cpp/src/io/parquet/stats_filter_helpers.cpp b/cpp/src/io/parquet/stats_filter_helpers.cpp index d8826ef95748..a7d513234c08 100644 --- a/cpp/src/io/parquet/stats_filter_helpers.cpp +++ b/cpp/src/io/parquet/stats_filter_helpers.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION. + * SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. * SPDX-License-Identifier: Apache-2.0 */ @@ -89,18 +89,6 @@ std::pair, bool> stats_columns_collector::get_stats_co return {std::move(_columns_mask), _has_is_null_operator}; } -std::vector> stats_columns_collector::visit_operands( - cudf::host_span const> operands) -{ - std::vector> transformed_operands; - std::transform(operands.begin(), - operands.end(), - std::back_inserter(transformed_operands), - [t = this](auto& operand) { return operand.get().accept(*t); }); - - return transformed_operands; -} - stats_expression_converter::stats_expression_converter(ast::expression const& expr, size_type num_columns, bool has_is_null_operator, diff --git a/cpp/src/io/parquet/stats_filter_helpers.hpp b/cpp/src/io/parquet/stats_filter_helpers.hpp index 60af924ea259..f0dd69563a01 100644 --- a/cpp/src/io/parquet/stats_filter_helpers.hpp +++ b/cpp/src/io/parquet/stats_filter_helpers.hpp @@ -340,9 +340,6 @@ class stats_columns_collector : public ast::detail::expression_transformer { std::pair, bool> get_stats_columns_mask() &&; protected: - std::vector> visit_operands( - cudf::host_span const> operands); - size_type _num_columns; private: From 11bd87ae2d3623700f3cdd192c42818d29a9b77e Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Fri, 7 Aug 2026 21:50:12 +0000 Subject: [PATCH 7/9] Simplify --- cpp/src/io/parquet/bloom_filter_reader.cu | 10 +++---- .../experimental/dictionary_page_filter.cu | 10 +++---- .../parquet/expression_transform_helpers.hpp | 27 ------------------- 3 files changed, 8 insertions(+), 39 deletions(-) diff --git a/cpp/src/io/parquet/bloom_filter_reader.cu b/cpp/src/io/parquet/bloom_filter_reader.cu index 07a8afb0eff9..a2f464fcd15a 100644 --- a/cpp/src/io/parquet/bloom_filter_reader.cu +++ b/cpp/src/io/parquet/bloom_filter_reader.cu @@ -225,13 +225,11 @@ class bloom_filter_expression_converter : public equality_literals_collector { auto const input_op = expr.get_operator(); auto const operator_arity = cudf::ast::detail::ast_operator_arity(input_op); - // Unary operation + // Membership filters cannot evaluate unary operations. Visit operands and push always true if (operator_arity == 1) { - auto visit_operands_fn = [this](auto const& operands) { - return this->visit_operands(operands); - }; - return parquet::detail::apply_unary_membership_transform( - expr, _bloom_filter_expr, *_always_true, visit_operands_fn); + std::ignore = this->visit_operands(expr.get_operands()); + _bloom_filter_expr.push(ast::operation{ast_operator::IDENTITY, *_always_true}); + return *_always_true; } // Binary operation diff --git a/cpp/src/io/parquet/experimental/dictionary_page_filter.cu b/cpp/src/io/parquet/experimental/dictionary_page_filter.cu index 36665cd1a7b1..c1a6e85f5fb7 100644 --- a/cpp/src/io/parquet/experimental/dictionary_page_filter.cu +++ b/cpp/src/io/parquet/experimental/dictionary_page_filter.cu @@ -1360,13 +1360,11 @@ class dictionary_expression_converter : public equality_literals_collector { auto const input_op = expr.get_operator(); auto const operator_arity = cudf::ast::detail::ast_operator_arity(input_op); - // Unary operation + // Membership filters cannot evaluate unary operations. Visit operands and push always true if (operator_arity == 1) { - auto visit_operands_fn = [this](auto const& operands) { - return this->visit_operands(operands); - }; - return parquet::detail::apply_unary_membership_transform( - expr, _dictionary_expr, *_always_true, visit_operands_fn); + std::ignore = this->visit_operands(expr.get_operands()); + _dictionary_expr.push(ast::operation{ast_operator::IDENTITY, *_always_true}); + return *_always_true; } // Binary operation diff --git a/cpp/src/io/parquet/expression_transform_helpers.hpp b/cpp/src/io/parquet/expression_transform_helpers.hpp index 20a437f90380..c31fa9590c59 100644 --- a/cpp/src/io/parquet/expression_transform_helpers.hpp +++ b/cpp/src/io/parquet/expression_transform_helpers.hpp @@ -98,33 +98,6 @@ template */ [[nodiscard]] std::optional de_morgan_operator(ast::ast_operator op); -/** - * @brief Handle unary operation transform for membership-based row group filters. i.e., bloom - * filter and dictionary page filter. - * - * @tparam VisitOperandsFn Callable matching `(std::span>) -> - * vector>` - * - * @param expr Unary operation to transform - * @param expr_tree The AST tree to push transformed expressions into - * @param always_true Reference to the always_true sentinel literal - * @param visit_operands_fn Callable to visit operands and return the transformed operands - * @return The `always_true` expression - */ -template -[[nodiscard]] inline std::reference_wrapper apply_unary_membership_transform( - ast::operation const& expr, - ast::tree& expr_tree, - std::reference_wrapper const always_true, - VisitOperandsFn&& visit_operands_fn) -{ - // Visit the operands to validate column references and collect any nested literals, then discard - // the transformed operands and relax this operation to `always_true` - std::ignore = visit_operands_fn(expr.get_operands()); - expr_tree.push(ast::operation{ast::ast_operator::IDENTITY, always_true}); - return always_true; -} - /** * @brief Collects column names from the expression ignoring the `skip_names` */ From 91102a8eedc24558d696918f4541be74e0e4c18c Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Mon, 17 Aug 2026 20:40:53 +0000 Subject: [PATCH 8/9] Address comments --- .../ast/detail/expression_transformer.hpp | 4 +- cpp/src/ast/expressions.cpp | 2 +- .../parquet/expression_transform_helpers.cpp | 48 ++++++++++++++-- .../parquet/expression_transform_helpers.hpp | 15 ++++- .../experimental/hybrid_scan_filters_test.cpp | 12 ++-- cpp/tests/io/parquet_reader_test.cpp | 12 ++++ .../cudf/tests/input_output/test_parquet.py | 56 ++++++++++++++----- 7 files changed, 120 insertions(+), 29 deletions(-) diff --git a/cpp/include/cudf/ast/detail/expression_transformer.hpp b/cpp/include/cudf/ast/detail/expression_transformer.hpp index 55167039f2f7..2ecfec01b84b 100644 --- a/cpp/include/cudf/ast/detail/expression_transformer.hpp +++ b/cpp/include/cudf/ast/detail/expression_transformer.hpp @@ -7,6 +7,8 @@ #include +#include + namespace CUDF_EXPORT cudf { namespace ast::detail { /** @@ -59,7 +61,7 @@ class expression_transformer { * @return References to transformed expressions */ [[nodiscard]] std::vector> visit_operands( - std::vector> const& operands); + std::span const> operands); }; } // namespace ast::detail diff --git a/cpp/src/ast/expressions.cpp b/cpp/src/ast/expressions.cpp index f1ca81cd15b0..af678a348909 100644 --- a/cpp/src/ast/expressions.cpp +++ b/cpp/src/ast/expressions.cpp @@ -33,7 +33,7 @@ operation::operation(ast_operator op, expression const& left, expression const& std::vector> detail::expression_transformer::visit_operands( - std::vector> const& operands) + std::span const> operands) { std::vector> transformed_operands; transformed_operands.reserve(operands.size()); diff --git a/cpp/src/io/parquet/expression_transform_helpers.cpp b/cpp/src/io/parquet/expression_transform_helpers.cpp index ecd4613aedef..dadd70d9245b 100644 --- a/cpp/src/io/parquet/expression_transform_helpers.cpp +++ b/cpp/src/io/parquet/expression_transform_helpers.cpp @@ -88,6 +88,38 @@ std::optional de_morgan_operator(ast::ast_operator op) } } +bool is_boolean_valued(ast::expression const& expr) +{ + using cudf::ast::ast_operator; + + // A literal knows its own type + if (auto const* literal = dynamic_cast(&expr); literal != nullptr) { + return literal->get_data_type().id() == cudf::type_id::BOOL8; + } + + // A column reference cannot be typed here - the normalizer runs before the output data types are + // known - so report it as not provably boolean + auto const* operation = dynamic_cast(&expr); + if (operation == nullptr) { return false; } + + switch (operation->get_operator()) { + case ast_operator::EQUAL: [[fallthrough]]; + case ast_operator::NOT_EQUAL: [[fallthrough]]; + case ast_operator::NULL_EQUAL: [[fallthrough]]; + case ast_operator::LESS: [[fallthrough]]; + case ast_operator::GREATER: [[fallthrough]]; + case ast_operator::LESS_EQUAL: [[fallthrough]]; + case ast_operator::GREATER_EQUAL: [[fallthrough]]; + case ast_operator::LOGICAL_AND: [[fallthrough]]; + case ast_operator::LOGICAL_OR: [[fallthrough]]; + case ast_operator::NULL_LOGICAL_AND: [[fallthrough]]; + case ast_operator::NULL_LOGICAL_OR: [[fallthrough]]; + case ast_operator::IS_NULL: [[fallthrough]]; + case ast_operator::NOT: return true; + default: return false; + } +} + unary_operand extract_unary_operand(ast::operation const& expr) { auto const& operands = expr.get_operands(); @@ -189,11 +221,13 @@ parquet_filter_normalizer::push_down_negation(ast::expression const& operand) auto const op = child_operation->get_operator(); auto const operands = child_operation->get_operands(); - // `NOT(NOT(x))` is `x`, including when `x` is null - if (op == ast_operator::NOT) { return operands.front().get().accept(*this); } + // `NOT(NOT(x))` is `x`, including when `x` is null, but only when `x` is boolean. + if (op == ast_operator::NOT and is_boolean_valued(operands.front().get())) { + return operands.front().get().accept(*this); + } - // De Morgan's laws compute exact equivalences for both Kleene (`LOGICAL_*`) and `NULL_LOGICAL_*` - // operators, including when an operand is null + // De Morgan's laws compute exact equivalences for both the null-propagating (`LOGICAL_*`) and the + // Kleene (`NULL_LOGICAL_*`) operators, including when an operand is null if (auto const de_morgan_op = de_morgan_operator(op); de_morgan_op.has_value()) { // Negate both operands before emplacing the parent, as negating them appends to `_operators` auto const lhs = negate(operands.front().get()); @@ -329,6 +363,12 @@ offset_column_references::offset_column_references( _converted_expr = expr.value().get().accept(*this); } +std::optional> +parquet_filter_normalizer::get_converted_expr() const +{ + return _converted_expr; +} + std::optional> offset_column_references::get_converted_expr() const { diff --git a/cpp/src/io/parquet/expression_transform_helpers.hpp b/cpp/src/io/parquet/expression_transform_helpers.hpp index c31fa9590c59..1dc45eaddfaf 100644 --- a/cpp/src/io/parquet/expression_transform_helpers.hpp +++ b/cpp/src/io/parquet/expression_transform_helpers.hpp @@ -98,6 +98,18 @@ template */ [[nodiscard]] std::optional de_morgan_operator(ast::ast_operator op); +/** + * @brief Returns whether an expression is boolean-valued + * + * Comparison, logical and null-checking operations always yield `BOOL8`, and a literal knows its + * own type. A column reference cannot be typed here, as the filter is normalized before the output + * data types are known, so it is reported as not boolean. + * + * @param expr Expression to classify + * @return Whether the expression yields `BOOL8` + */ +[[nodiscard]] bool is_boolean_valued(ast::expression const& expr); + /** * @brief Collects column names from the expression ignoring the `skip_names` */ @@ -193,7 +205,8 @@ class parquet_filter_normalizer : public ast::detail::expression_transformer { * Only rewrites that are exact in every case cudf's AST evaluates are applied, as the converted * expression also filters the decoded rows: * - * - `NOT(NOT(x))` => `x` + * - `NOT(NOT(x))` => `x`, but only when `x` is provably boolean. `NOT` yields `bool` for any + * operand, so for a non-boolean `x` the double negation means `x != 0` rather than `x` * - De Morgan forms: `NOT(a AND b)` => `NOT(a) OR NOT(b)` for both the null-propagating * (`LOGICAL_*`) and the Kleene (`NULL_LOGICAL_*`) operators * - `NOT(a == b)` => `a != b` and vice versa diff --git a/cpp/tests/io/experimental/hybrid_scan_filters_test.cpp b/cpp/tests/io/experimental/hybrid_scan_filters_test.cpp index 82fdf2987719..7bedd0076f8b 100644 --- a/cpp/tests/io/experimental/hybrid_scan_filters_test.cpp +++ b/cpp/tests/io/experimental/hybrid_scan_filters_test.cpp @@ -488,17 +488,15 @@ TEST_F(HybridScanFiltersTest, FilterRowGroupsWithComplexExpressions) EXPECT_EQ(reader->total_rows_in_row_groups(stats_filtered), 2 * rows_per_row_group); } - // Filter: NOT(col0 > 50 AND col0 < 100) - // De Morgan returns col0 <= 50 OR col0 >= 100, stats transform: vmin <= 50 OR vmax >= 100. Every - // row group satisfies a disjunct, so nothing is pruned + // Filter: NOT(col0 != 50 AND col0 != 100) becomes col0 == 50 OR col0 == 100. Prunes RG0 and RG3. { auto literal_50_value = cudf::numeric_scalar(50, true, cudf::get_default_stream()); auto literal_50 = cudf::ast::literal(literal_50_value); auto literal_100_value = cudf::numeric_scalar(100, true, cudf::get_default_stream()); auto literal_100 = cudf::ast::literal(literal_100_value); - auto gt_50 = cudf::ast::operation(cudf::ast::ast_operator::GREATER, col_ref0, literal_50); - auto lt_100 = cudf::ast::operation(cudf::ast::ast_operator::LESS, col_ref0, literal_100); - auto inner = cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_AND, gt_50, lt_100); + auto ne_50 = cudf::ast::operation(cudf::ast::ast_operator::NOT_EQUAL, col_ref0, literal_50); + auto ne_100 = cudf::ast::operation(cudf::ast::ast_operator::NOT_EQUAL, col_ref0, literal_100); + auto inner = cudf::ast::operation(cudf::ast::ast_operator::LOGICAL_AND, ne_50, ne_100); auto filter = cudf::ast::operation(cudf::ast::ast_operator::NOT, inner); options.set_filter(filter); reader->reset_column_selection(); @@ -506,7 +504,7 @@ TEST_F(HybridScanFiltersTest, FilterRowGroupsWithComplexExpressions) auto input_row_group_indices = reader->all_row_groups(options); auto stats_filtered = reader->filter_row_groups_with_stats( input_row_group_indices, options, cudf::get_default_stream()); - EXPECT_EQ(stats_filtered.size(), 4); + EXPECT_EQ(stats_filtered.size(), 2); } // Filter: NOT(NOT(col0 < 100) OR col0 > 150) diff --git a/cpp/tests/io/parquet_reader_test.cpp b/cpp/tests/io/parquet_reader_test.cpp index c55dc9f9aa67..0e8b2c90e616 100644 --- a/cpp/tests/io/parquet_reader_test.cpp +++ b/cpp/tests/io/parquet_reader_test.cpp @@ -2005,6 +2005,18 @@ TEST_F(ParquetReaderTest, FilterNegationPushdown) expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, not_lt), 1); } + // 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); + auto not_not_a = cudf::ast::operation(cudf::ast::ast_operator::NOT, not_a); + + // Eliminating these would hand the reader an INT32 predicate instead of a BOOL8 one + expect_matches_unrewritten(not_not_a); + + // Whereas the triple negation still folds, since NOT(col_a) is boolean + expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, not_not_a)); + } + // NOT(col_a == 10) and NOT(col_a != 10) - complemented equality. col_a != 10 prunes nothing and // col_a == 10 keeps only the first row group expect_matches_unrewritten(cudf::ast::operation(cudf::ast::ast_operator::NOT, a_eq_10), 4); diff --git a/python/cudf/cudf/tests/input_output/test_parquet.py b/python/cudf/cudf/tests/input_output/test_parquet.py index a6202d329ff9..dcde50e7c77b 100644 --- a/python/cudf/cudf/tests/input_output/test_parquet.py +++ b/python/cudf/cudf/tests/input_output/test_parquet.py @@ -4693,7 +4693,7 @@ def test_parquet_reader_mismatched_nullability_structs(tmp_path): ], ) def test_parquet_bloom_filter_negated_equality(datadir, bloom_filter_fname): - """`NOT(col == v)` must prune identically to `col != v`""" + """Negated equality predicates must prune identically to their rewrites""" import pylibcudf as plc from pylibcudf.expressions import ( @@ -4712,25 +4712,51 @@ def read_with(filter_expr): options.set_filter(filter_expr) return plc.io.parquet.read_parquet(options) - negated_equality = read_with( - Operation( - ASTOperator.NOT, - Operation(ASTOperator.EQUAL, ColumnNameReference("str"), needle), + 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, ) - not_equal = read_with( - Operation(ASTOperator.NOT_EQUAL, ColumnNameReference("str"), needle) - ) + # 998 of the 1000 rows are not "FINDME". + assert_eq(negated_equality.tbl.num_rows(), 998) - # The two spellings are the same predicate and must prune identically - assert ( - negated_equality.num_row_groups_after_bloom_filter - == not_equal.num_row_groups_after_bloom_filter + assert_equivalent( + Operation(ASTOperator.NOT, str_ne), + str_eq, ) - assert negated_equality.tbl.to_arrow().equals(not_equal.tbl.to_arrow()) - # 998 of the 1000 rows are not "FINDME". - assert negated_equality.tbl.num_rows() == 998 + 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( From 1e7510d4193a87ff6e74d5c6dacf40d344a35aac Mon Sep 17 00:00:00 2001 From: Muhammad Haseeb <14217455+mhaseeb123@users.noreply.github.com> Date: Mon, 17 Aug 2026 22:19:32 +0000 Subject: [PATCH 9/9] Remove use of pylibcudf in cudf python tests --- .../cudf/tests/input_output/test_parquet.py | 75 +------------------ 1 file changed, 1 insertion(+), 74 deletions(-) diff --git a/python/cudf/cudf/tests/input_output/test_parquet.py b/python/cudf/cudf/tests/input_output/test_parquet.py index dcde50e7c77b..cbba44f70730 100644 --- a/python/cudf/cudf/tests/input_output/test_parquet.py +++ b/python/cudf/cudf/tests/input_output/test_parquet.py @@ -4685,80 +4685,6 @@ def test_parquet_reader_mismatched_nullability_structs(tmp_path): ) -@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/rapidsai/cudf/issues/17806", @@ -4780,6 +4706,7 @@ def assert_equivalent(lhs, rhs): "predicate,expected_len", [ ([[("str", "==", "FINDME")], [("fp64", "==", float(500))]], 2), + ([("str", "!=", "FINDME")], 998), ([("fixed_pt", "==", decimal.Decimal(float(500)))], 2), ([[("ui32", "==", np.uint32(500)), ("str", "==", "FINDME")]], 2), ([[("str", "==", "FINDME")], [("ui32", ">=", np.uint32(0))]], 1000),