Skip to content

Commit 8b648be

Browse files
authored
GH-48877: [C++][Parquet] Fix writer not to throw for bloom filter on disabled bool column (#48878)
### Rationale for this change Now that #37400 is merged. After a quick integration, I found that the Parquet writer throws if bloom filter is enabled and there is at least one boolean column (even when it is not enabled). ### What changes are included in this PR? Move the bloom filter type check after bloom filter enablement check. ### Are these changes tested? Yes, added test to verify this does not throw. ### Are there any user-facing changes? No. * GitHub Issue: #48877 Authored-by: Gang Wu <ustcwg@gmail.com> Signed-off-by: Gang Wu <ustcwg@gmail.com>
1 parent e4c9ed2 commit 8b648be

File tree

2 files changed

+10
-6
lines changed

2 files changed

+10
-6
lines changed

cpp/src/parquet/bloom_filter_reader_writer_test.cc

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,8 @@ TEST(BloomFilterBuilder, BasicRoundTrip) {
153153
TEST(BloomFilterBuilder, InvalidOperations) {
154154
SchemaDescriptor schema;
155155
schema::NodePtr root = schema::GroupNode::Make(
156-
"schema", Repetition::REPEATED, {schema::ByteArray("c1"), schema::Boolean("c2")});
156+
"schema", Repetition::REPEATED,
157+
{schema::ByteArray("c1"), schema::Boolean("c2"), schema::Boolean("c3")});
157158
schema.Init(root);
158159

159160
WriterProperties::Builder properties_builder;
@@ -173,17 +174,20 @@ TEST(BloomFilterBuilder, InvalidOperations) {
173174

174175
// Column ordinal is out of bound.
175176
bloom_filter_builder->AppendRowGroup();
176-
EXPECT_THROW_THAT([&]() { bloom_filter_builder->CreateBloomFilter(2); },
177+
EXPECT_THROW_THAT([&]() { bloom_filter_builder->CreateBloomFilter(3); },
177178
ParquetException,
178-
::testing::Property(&ParquetException::what,
179-
::testing::HasSubstr("Invalid column ordinal")));
179+
::testing::Property(
180+
&ParquetException::what,
181+
::testing::HasSubstr("Invalid Column Index: 3 Num columns: 3")));
180182

181183
// Boolean type is not supported.
182184
EXPECT_THROW_THAT(
183185
[&]() { bloom_filter_builder->CreateBloomFilter(1); }, ParquetException,
184186
::testing::Property(
185187
&ParquetException::what,
186188
::testing::HasSubstr("BloomFilterBuilder does not support boolean type")));
189+
// Null should be returned instead of throwing exception for non-enabled boolean type.
190+
EXPECT_EQ(bloom_filter_builder->CreateBloomFilter(2), nullptr);
187191

188192
// Create a created bloom filter should throw.
189193
ASSERT_NO_THROW(bloom_filter_builder->CreateBloomFilter(0));

cpp/src/parquet/bloom_filter_writer.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -199,13 +199,13 @@ void BloomFilterBuilderImpl::AppendRowGroup() {
199199
}
200200

201201
BloomFilter* BloomFilterBuilderImpl::CreateBloomFilter(int32_t column_ordinal) {
202-
CheckState(column_ordinal);
203-
204202
auto opts = properties_->bloom_filter_options(schema_->Column(column_ordinal)->path());
205203
if (!opts.has_value()) {
206204
return nullptr;
207205
}
208206

207+
CheckState(column_ordinal);
208+
209209
auto& curr_rg_bfs = *bloom_filters_.rbegin();
210210
if (curr_rg_bfs.find(column_ordinal) != curr_rg_bfs.cend()) {
211211
std::stringstream ss;

0 commit comments

Comments
 (0)