Repository navigation
Fix hybrid scan parquet reader incorrectly parse bloom filter #22901
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 9 commits
a18c546
b609755
a32a95f
03c1834
df8fc33
be4fde6
48517bc
f59589e
12d607b
518d278
d9f37e2
c568dc2
6d87652
9cf8b38
15ddaee
61dbf3b
50bbc17
4ee65aa
cb9067f
442cb67
8981d57
6e6443a
b3cd38b
3969eed
3d438af
b82a758
be4f00f
e73fc76
177f978
588187d
cbb0da3
7606a90
44a5743
b3741f9
344a3b3
d36cddc
d0af7d2
59e7df3
4f458b1
3851e9f
1159a8b
dc50d9d
dfc2efe
37d10c2
c562afd
f6f8fb3
4de4bcf
083e63d
66a58d3
afb13f6
069e642
1872860
1145ad5
4cb0596
2e7b1a7
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 | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| #include "io/comp/common.hpp" | ||||||||||||||||||||||||||
| #include "io/parquet/parquet_common.hpp" | ||||||||||||||||||||||||||
| #include "io/parquet/reader_impl_helpers.hpp" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| #include <cudf/detail/nvtx/ranges.hpp> | ||||||||||||||||||||||||||
| #include <cudf/detail/utilities/cuda_memcpy.hpp> | ||||||||||||||||||||||||||
|
|
@@ -25,15 +26,19 @@ | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| #include <algorithm> | ||||||||||||||||||||||||||
| #include <array> | ||||||||||||||||||||||||||
| #include <cstdint> | ||||||||||||||||||||||||||
| #include <cstring> | ||||||||||||||||||||||||||
| #include <format> | ||||||||||||||||||||||||||
| #include <functional> | ||||||||||||||||||||||||||
| #include <iterator> | ||||||||||||||||||||||||||
| #include <mutex> | ||||||||||||||||||||||||||
| #include <numeric> | ||||||||||||||||||||||||||
| #include <optional> | ||||||||||||||||||||||||||
| #include <stdexcept> | ||||||||||||||||||||||||||
| #include <string> | ||||||||||||||||||||||||||
| #include <tuple> | ||||||||||||||||||||||||||
| #include <type_traits> | ||||||||||||||||||||||||||
| #include <utility> | ||||||||||||||||||||||||||
| #include <vector> | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||
|
|
@@ -375,6 +380,69 @@ fetch_byte_ranges_to_device_async_impl( | |||||||||||||||||||||||||
| std::async(std::launch::deferred, sync_function, std::move(device_read_tasks))}; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| std::tuple<std::vector<rmm::device_buffer>, | ||||||||||||||||||||||||||
| std::vector<device_spans_per_source_type>, | ||||||||||||||||||||||||||
| std::future<void>> | ||||||||||||||||||||||||||
| fetch_bloom_filters_to_device_async_impl( | ||||||||||||||||||||||||||
|
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. Mostly same as the This is the bench result with a wide BI fact table (10M rows, 82 row groups, 11 cols, bloom on 4 dims):
Takeaway: the branch wins when the read is bloom-fetch bound (selective / needle queries) and is a |
||||||||||||||||||||||||||
| cudf::host_span<std::reference_wrapper<cudf::io::datasource> const> datasources, | ||||||||||||||||||||||||||
| cudf::host_span<cudf::host_span<cudf::io::text::byte_range_info const> const> | ||||||||||||||||||||||||||
| bloom_filter_byte_ranges_per_source, | ||||||||||||||||||||||||||
| rmm::cuda_stream_view stream, | ||||||||||||||||||||||||||
| rmm::device_async_resource_ref mr) | ||||||||||||||||||||||||||
|
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.
Suggested change
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.
We now use
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. not addressed in the current version (missing commits?)
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. See #22901 (comment) |
||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| auto const num_sources = datasources.size(); | ||||||||||||||||||||||||||
| CUDF_EXPECTS(num_sources == bloom_filter_byte_ranges_per_source.size(), | ||||||||||||||||||||||||||
| "Encountered mismatch in number of datasources and bloom filter byte range spans"); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Read + parse a single bloom filter header to host and return its bitset-only byte range | ||||||||||||||||||||||||||
|
qbacpey marked this conversation as resolved.
Resolved
|
||||||||||||||||||||||||||
| auto const fetch_bitset_range = | ||||||||||||||||||||||||||
| [](cudf::io::datasource& datasource, | ||||||||||||||||||||||||||
| cudf::io::text::byte_range_info const& bloom_range) -> cudf::io::text::byte_range_info { | ||||||||||||||||||||||||||
| // placeholder for a chunk whose column has no bloom filter written | ||||||||||||||||||||||||||
| if (bloom_range.is_empty()) { return {0, 0}; } | ||||||||||||||||||||||||||
| auto const header_read_size = | ||||||||||||||||||||||||||
| std::min<int64_t>(bloom_range.size(), detail::bloom_filter_header_max_size); | ||||||||||||||||||||||||||
| auto const header = datasource.host_read(static_cast<std::size_t>(bloom_range.offset()), | ||||||||||||||||||||||||||
| static_cast<std::size_t>(header_read_size)); | ||||||||||||||||||||||||||
| auto const header_info = detail::parse_bloom_filter_header({header->data(), header->size()}); | ||||||||||||||||||||||||||
| CUDF_EXPECTS(header_info.has_value(), "Encountered an invalid bloom filter header"); | ||||||||||||||||||||||||||
| auto const [header_size, bitset_size] = header_info.value(); | ||||||||||||||||||||||||||
| return {bloom_range.offset() + header_size, static_cast<int64_t>(bitset_size)}; | ||||||||||||||||||||||||||
|
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. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win HIGH: Bound-check the parsed bitset against the supplied bloom range. Issue: Line 410 trusts 🐛 Proposed fix auto const header_info = detail::parse_bloom_filter_header({header->data(), header->size()});
CUDF_EXPECTS(header_info.has_value(), "Encountered an invalid bloom filter header");
auto const [header_size, bitset_size] = header_info.value();
+ CUDF_EXPECTS(header_size >= 0 and
+ header_size <= bloom_range.size() and
+ bitset_size <= static_cast<std::size_t>(bloom_range.size() - header_size),
+ "Bloom filter byte range does not contain the complete bitset");
return {bloom_range.offset() + header_size, static_cast<int64_t>(bitset_size)};As per coding guidelines, logic errors producing wrong row-group pruning are high-impact correctness issues. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Parse each source's bloom filter headers with one task per source | ||||||||||||||||||||||||||
| auto const bitset_byte_ranges_per_source = | ||||||||||||||||||||||||||
| dispatch_fetch_tasks(num_sources, [&](std::size_t source_idx) { | ||||||||||||||||||||||||||
| auto const& bloom_ranges = bloom_filter_byte_ranges_per_source[source_idx]; | ||||||||||||||||||||||||||
| auto& datasource = datasources[source_idx].get(); | ||||||||||||||||||||||||||
| std::vector<cudf::io::text::byte_range_info> bitset_ranges; | ||||||||||||||||||||||||||
| bitset_ranges.reserve(bloom_ranges.size()); | ||||||||||||||||||||||||||
| std::transform( | ||||||||||||||||||||||||||
| bloom_ranges.begin(), | ||||||||||||||||||||||||||
| bloom_ranges.end(), | ||||||||||||||||||||||||||
| std::back_inserter(bitset_ranges), | ||||||||||||||||||||||||||
| [&](auto const& bloom_range) { return fetch_bitset_range(datasource, bloom_range); }); | ||||||||||||||||||||||||||
| return bitset_ranges; | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
qbacpey marked this conversation as resolved.
Resolved
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Fetch only the header-free bitsets to device | ||||||||||||||||||||||||||
| auto result = | ||||||||||||||||||||||||||
| fetch_byte_ranges_to_device_async(datasources, bitset_byte_ranges_per_source, stream, mr); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| auto const& bitset_spans_per_source = std::get<1>(result); | ||||||||||||||||||||||||||
| CUDF_EXPECTS(std::all_of(bitset_spans_per_source.begin(), | ||||||||||||||||||||||||||
| bitset_spans_per_source.end(), | ||||||||||||||||||||||||||
| [](auto const& spans) { | ||||||||||||||||||||||||||
| return std::all_of(spans.begin(), spans.end(), [](auto const& span) { | ||||||||||||||||||||||||||
| return span.empty() or | ||||||||||||||||||||||||||
| (reinterpret_cast<std::uintptr_t>(span.data()) % 32) == 0; | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||
| "Bloom filter bitset is not 32-byte aligned"); | ||||||||||||||||||||||||||
|
qbacpey marked this conversation as resolved.
Resolved
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return result; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| } // namespace | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| [[nodiscard]] std::size_t metadata_size_hint() | ||||||||||||||||||||||||||
|
|
@@ -471,4 +539,29 @@ fetch_byte_ranges_to_device_async( | |||||||||||||||||||||||||
| mr); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| std::tuple<std::vector<rmm::device_buffer>, | ||||||||||||||||||||||||||
| std::vector<cudf::device_span<uint8_t const>>, | ||||||||||||||||||||||||||
| std::future<void>> | ||||||||||||||||||||||||||
| fetch_bloom_filters_to_device_async( | ||||||||||||||||||||||||||
| cudf::io::datasource& datasource, | ||||||||||||||||||||||||||
| cudf::host_span<cudf::io::text::byte_range_info const> bloom_filter_byte_ranges, | ||||||||||||||||||||||||||
| rmm::cuda_stream_view stream, | ||||||||||||||||||||||||||
| rmm::device_async_resource_ref mr) | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| CUDF_FUNC_RANGE(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Wrap the inputs into arrays and delegate to the multi-source implementation | ||||||||||||||||||||||||||
| std::array<std::reference_wrapper<cudf::io::datasource>, 1> datasources{std::ref(datasource)}; | ||||||||||||||||||||||||||
| std::array<cudf::host_span<cudf::io::text::byte_range_info const>, 1> | ||||||||||||||||||||||||||
| bloom_filter_byte_ranges_per_source{bloom_filter_byte_ranges}; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| auto [buffers, fetched_byte_ranges, fut] = fetch_bloom_filters_to_device_async_impl( | ||||||||||||||||||||||||||
| {datasources.data(), datasources.size()}, | ||||||||||||||||||||||||||
| {bloom_filter_byte_ranges_per_source.data(), bloom_filter_byte_ranges_per_source.size()}, | ||||||||||||||||||||||||||
| stream, | ||||||||||||||||||||||||||
| mr); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return {std::move(buffers), std::move(fetched_byte_ranges.front()), std::move(fut)}; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| } // namespace cudf::io::parquet | ||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,10 +15,12 @@ | |
|
|
||
| #include <cstddef> | ||
| #include <functional> | ||
| #include <optional> | ||
| #include <string> | ||
| #include <string_view> | ||
| #include <tuple> | ||
| #include <unordered_map> | ||
| #include <utility> | ||
| #include <vector> | ||
|
|
||
| namespace cudf::io::parquet::detail { | ||
|
|
@@ -142,6 +144,22 @@ struct surviving_row_group_metrics { | |
| std::optional<size_type> after_bloom_filter; // number of surviving row groups after bloom filter | ||
| }; | ||
|
|
||
| /** | ||
| * @brief Upper bound on the size in bytes of a Parquet `BloomFilterHeader` | ||
| */ | ||
| inline constexpr int64_t bloom_filter_header_max_size = 256; | ||
|
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. If the |
||
|
|
||
| /** | ||
| * @brief Parses and validates a Parquet `BloomFilterHeader` from the front of `bytes` | ||
| * | ||
| * @param bytes Host bytes starting at the beginning of a bloom filter (header followed by bitset) | ||
| * | ||
| * @return A pair of the bloom filter header size and the bitset size in bytes, or `std::nullopt` | ||
| * if the header is missing or unsupported | ||
| */ | ||
| [[nodiscard]] std::optional<std::pair<int64_t, std::size_t>> parse_bloom_filter_header( | ||
|
qbacpey marked this conversation as resolved.
Resolved
|
||
| host_span<uint8_t const> bytes); | ||
|
|
||
| class aggregate_reader_metadata { | ||
| protected: | ||
| std::vector<metadata> per_file_metadata; | ||
|
|
||

Uh oh!
There was an error while loading. Please reload this page.