-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix Parquet row group pruning for negated equality predicates #23580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
023cae7
b769625
07595b6
6c3e552
224b87e
1b56bad
11bd87a
91102a8
a9d26a5
1e7510d
d524604
151cee6
562afbf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,18 @@ operation::operation(ast_operator op, expression const& left, expression const& | |
| std::invalid_argument); | ||
| } | ||
|
|
||
| std::vector<std::reference_wrapper<expression const>> | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved from |
||
| detail::expression_transformer::visit_operands( | ||
| std::span<std::reference_wrapper<expression const> const> operands) | ||
| { | ||
| std::vector<std::reference_wrapper<expression const>> 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); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, *this, 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 | ||
|
|
@@ -556,16 +554,4 @@ std::vector<std::vector<ast::literal*>> equality_literals_collector::get_literal | |
| return std::move(_literals); | ||
| } | ||
|
|
||
| std::vector<std::reference_wrapper<ast::expression const>> | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not needed |
||
| equality_literals_collector::visit_operands( | ||
| cudf::host_span<std::reference_wrapper<ast::expression const> const> operands) | ||
| { | ||
| std::vector<std::reference_wrapper<ast::expression const>> 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -739,9 +739,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( | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simple rename |
||
| std::optional<std::reference_wrapper<ast::expression const>> expr, | ||
| table_metadata const& metadata, | ||
| std::vector<SchemaElement> const& schema_tree, | ||
|
|
@@ -763,7 +764,7 @@ named_to_reference_converter::named_to_reference_converter( | |
| expr.value().get().accept(*this); | ||
| } | ||
|
|
||
| std::reference_wrapper<ast::expression const> named_to_reference_converter::visit( | ||
| std::reference_wrapper<ast::expression const> parquet_filter_normalizer::visit( | ||
| ast::column_reference const& expr) | ||
| { | ||
| // Map the column index to its name | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -400,19 +400,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 | ||
|
Comment on lines
+403
to
+404
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The term of art in mathematical logic here is "negation normal form", fwiw |
||
| */ | ||
| class named_to_reference_converter : public parquet::detail::named_to_reference_converter { | ||
| class parquet_filter_normalizer : public parquet::detail::parquet_filter_normalizer { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simple rename |
||
| public: | ||
| named_to_reference_converter() = default; | ||
| parquet_filter_normalizer() = default; | ||
|
|
||
| named_to_reference_converter(std::optional<std::reference_wrapper<ast::expression const>> expr, | ||
| table_metadata const& metadata, | ||
| std::vector<SchemaElement> const& schema_tree, | ||
| cudf::io::parquet_reader_options const& options, | ||
| bool case_sensitive_names); | ||
| parquet_filter_normalizer(std::optional<std::reference_wrapper<ast::expression const>> expr, | ||
| table_metadata const& metadata, | ||
| std::vector<SchemaElement> 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& ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -245,15 +245,15 @@ void hybrid_scan_reader_impl::reset_column_selection() | |
| _is_payload_columns_selected = false; | ||
| } | ||
|
|
||
| std::pair<named_to_reference_converter, std::vector<cudf::data_type>> | ||
| std::pair<parquet_filter_normalizer, std::vector<cudf::data_type>> | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simple rename throughout here. |
||
| 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)}; | ||
|
|
@@ -673,8 +673,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)) { | ||
|
|
@@ -747,8 +747,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, {}); | ||
|
|
||
|
|
@@ -782,8 +782,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)) { | ||
|
|
@@ -949,8 +949,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, {}); | ||
| } | ||
|
|
@@ -1090,7 +1090,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(); | ||
| } | ||
|
|
||
|
|
@@ -1124,18 +1124,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; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -364,13 +364,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 | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, simple rename |
||
| * 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); | ||
|
|
||
| /** | ||
|
|
@@ -406,12 +406,12 @@ class hybrid_scan_reader_impl : public parquet::detail::reader_impl { | |
| std::span<std::vector<size_type> 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<named_to_reference_converter, std::vector<cudf::data_type>> | ||
| std::pair<parquet_filter_normalizer, std::vector<cudf::data_type>> | ||
| prepare_filter_and_output_types(parquet_reader_options const& options); | ||
|
|
||
| /** | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple rename in this PR. Happy to move to a separate PR if preferred but then until the follow up merges,
named_to_reference_converterwill be doing two jobs (convert names -> references and pushdown negations) instead of what it advertises.