You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mapleFU (via GitHub)" <gi...@apache.org> on 2023/03/24 20:02:21 UTC
[GitHub] [arrow] mapleFU opened a new pull request, #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
mapleFU opened a new pull request, #34728:
URL: https://github.com/apache/arrow/pull/34728
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Allow reading bloom filter in parquet file reader.
### What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
1. [x] Add Basic BloomFilterReader
2. [ ] Test BloomFilterReader
### Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
3. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Will add
### Are there any user-facing changes?
User can read bf from file now.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wjones127 commented on a diff in pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #34728:
URL: https://github.com/apache/arrow/pull/34728#discussion_r1148084289
##########
cpp/src/parquet/bloom_filter_reader.h:
##########
@@ -0,0 +1,51 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "parquet/bloom_filter.h"
+#include "parquet/encryption/internal_file_decryptor.h"
+
+namespace parquet {
+
+class PARQUET_EXPORT RowGroupBloomFilterReader {
+ public:
+ virtual ~RowGroupBloomFilterReader() = default;
+
+ virtual std::unique_ptr<BloomFilter> GetColumnBloomFilter(int i) = 0;
+};
+
+/// \brief Interface for reading the bloom filter for a Parquet file.
+class PARQUET_EXPORT BloomFilterReader {
+ public:
+ virtual ~BloomFilterReader() = default;
+
+ /// \brief Create a BloomFilterReader instance.
+ /// \returns a BloomFilterReader instance.
+ /// WARNING: The returned BloomFilterReader references to all the input parameters, so
Review Comment:
Does this just apply to `ReaderProperties`? Since the other inputs are `shared_ptr` which should manage the lifetime, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mapleFU commented on pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #34728:
URL: https://github.com/apache/arrow/pull/34728#issuecomment-1487892090
@westonpace @wjones127 Mind take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mapleFU commented on pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #34728:
URL: https://github.com/apache/arrow/pull/34728#issuecomment-1483345184
@wjones127 @wgtmac @westonpace Do you think this interface is ok? If ok, I will continue work on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mapleFU commented on pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #34728:
URL: https://github.com/apache/arrow/pull/34728#issuecomment-1486225697
@wgtmac PTAL again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mapleFU commented on a diff in pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #34728:
URL: https://github.com/apache/arrow/pull/34728#discussion_r1149972827
##########
cpp/src/parquet/file_reader.h:
##########
@@ -146,6 +148,11 @@ class PARQUET_EXPORT ParquetFileReader {
/// WARNING: The returned PageIndexReader must not outlive the ParquetFileReader.
std::shared_ptr<PageIndexReader> GetPageIndexReader();
+ /// Returns the BloomFilterReader. Only one instance is ever created.
+ ///
+ /// WARNING: The returned BloomFilterReader must not outlive the ParquetFileReader.
Review Comment:
Good idea, let me try it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace merged pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace merged PR #34728:
URL: https://github.com/apache/arrow/pull/34728
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wjones127 commented on a diff in pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #34728:
URL: https://github.com/apache/arrow/pull/34728#discussion_r1148085059
##########
cpp/src/parquet/bloom_filter_reader.h:
##########
@@ -0,0 +1,51 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "parquet/bloom_filter.h"
+#include "parquet/encryption/internal_file_decryptor.h"
+
+namespace parquet {
+
+class PARQUET_EXPORT RowGroupBloomFilterReader {
+ public:
+ virtual ~RowGroupBloomFilterReader() = default;
+
+ virtual std::unique_ptr<BloomFilter> GetColumnBloomFilter(int i) = 0;
Review Comment:
This is the bloom filter for column `i`? And returns nullptr if it doesn't have one for that column?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wgtmac commented on a diff in pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #34728:
URL: https://github.com/apache/arrow/pull/34728#discussion_r1148373819
##########
cpp/src/parquet/bloom_filter_reader.h:
##########
@@ -0,0 +1,51 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "parquet/bloom_filter.h"
+#include "parquet/encryption/internal_file_decryptor.h"
+
+namespace parquet {
+
+class PARQUET_EXPORT RowGroupBloomFilterReader {
+ public:
+ virtual ~RowGroupBloomFilterReader() = default;
+
+ virtual std::unique_ptr<BloomFilter> GetColumnBloomFilter(int i) = 0;
+};
+
+/// \brief Interface for reading the bloom filter for a Parquet file.
+class PARQUET_EXPORT BloomFilterReader {
+ public:
+ virtual ~BloomFilterReader() = default;
+
+ /// \brief Create a BloomFilterReader instance.
+ /// \returns a BloomFilterReader instance.
+ /// WARNING: The returned BloomFilterReader references to all the input parameters, so
+ /// it must not outlive all of the input parameters. Usually these input parameters
+ /// come from the same ParquetFileReader object, so it must not outlive the reader
+ /// that creates this BloomFilterReader.
+ static std::shared_ptr<BloomFilterReader> Make(
+ std::shared_ptr<::arrow::io::RandomAccessFile> input,
+ std::shared_ptr<FileMetaData> file_metadata, const ReaderProperties& properties,
+ std::shared_ptr<InternalFileDecryptor> file_decryptor = NULLPTR);
+
+ virtual std::shared_ptr<RowGroupBloomFilterReader> RowGroup(int i) = 0;
Review Comment:
Please add doc string for the public api.
##########
cpp/src/parquet/bloom_filter_reader.cc:
##########
@@ -0,0 +1,114 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/bloom_filter_reader.h"
+#include "metadata.h"
+#include "parquet/exception.h"
+
+namespace parquet {
+
+class RowGroupBloomFilterReaderImpl final : public RowGroupBloomFilterReader {
+ public:
+ RowGroupBloomFilterReaderImpl(std::shared_ptr<::arrow::io::RandomAccessFile> input,
+ std::shared_ptr<RowGroupMetaData> row_group_metadata,
+ const ReaderProperties& properties)
+ : input_(std::move(input)),
+ row_group_metadata_(std::move(row_group_metadata)),
+ properties_(properties) {}
+
+ std::unique_ptr<BloomFilter> GetColumnBloomFilter(int i) override;
+
+ private:
+ /// The input stream that can perform random access read.
+ std::shared_ptr<::arrow::io::RandomAccessFile> input_;
+
+ /// The row group metadata to get column chunk metadata.
+ std::shared_ptr<RowGroupMetaData> row_group_metadata_;
+
+ /// Reader properties used to deserialize thrift object.
+ const ReaderProperties& properties_;
+};
+
+std::unique_ptr<BloomFilter> RowGroupBloomFilterReaderImpl::GetColumnBloomFilter(int i) {
+ if (i < 0 || i >= row_group_metadata_->num_columns()) {
+ throw ParquetException("Invalid column index at column ordinal ", i);
+ }
+
+ auto col_chunk = row_group_metadata_->ColumnChunk(i);
+ std::unique_ptr<ColumnCryptoMetaData> crypto_metadata = col_chunk->crypto_metadata();
+ if (crypto_metadata != nullptr) {
+ ParquetException::NYI("Cannot read encrypted bloom filter yet");
+ }
+
+ auto bloom_filter_offset = col_chunk->bloom_filter_offset();
+ if (!bloom_filter_offset.has_value()) {
+ return nullptr;
+ }
+ PARQUET_ASSIGN_OR_THROW(auto file_size, input_->GetSize());
+ if (file_size <= *bloom_filter_offset) {
+ throw ParquetException("file size less or equal than bloom offset");
+ }
+ auto stream = ::arrow::io::RandomAccessFile::GetStream(
+ input_, *bloom_filter_offset, file_size - *bloom_filter_offset);
+ auto bloom_filter = BlockSplitBloomFilter::Deserialize(properties_, stream->get());
+ return std::make_unique<BlockSplitBloomFilter>(std::move(bloom_filter));
+}
+
+class BloomFilterReaderImpl final : public BloomFilterReader {
+ public:
+ BloomFilterReaderImpl(std::shared_ptr<::arrow::io::RandomAccessFile> input,
+ std::shared_ptr<FileMetaData> file_metadata,
+ const ReaderProperties& properties,
+ std::shared_ptr<InternalFileDecryptor> file_decryptor)
+ : input_(std::move(input)),
+ file_metadata_(std::move(file_metadata)),
+ properties_(properties) {
+ if (file_decryptor != nullptr) {
+ ParquetException::NYI("BloomFilter encryption not support");
+ }
+ }
+
+ std::shared_ptr<RowGroupBloomFilterReader> RowGroup(int i) {
+ if (i < 0 || i >= file_metadata_->num_row_groups()) {
+ throw ParquetException("Invalid row group ordinal: ", i);
+ }
+
+ auto row_group_metadata = file_metadata_->RowGroup(i);
+ return std::make_shared<RowGroupBloomFilterReaderImpl>(
Review Comment:
Should we check if none of bloom filter exists and return nullptr?
##########
cpp/src/parquet/bloom_filter_reader.h:
##########
@@ -0,0 +1,51 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "parquet/bloom_filter.h"
+#include "parquet/encryption/internal_file_decryptor.h"
+
+namespace parquet {
+
+class PARQUET_EXPORT RowGroupBloomFilterReader {
+ public:
+ virtual ~RowGroupBloomFilterReader() = default;
+
+ virtual std::unique_ptr<BloomFilter> GetColumnBloomFilter(int i) = 0;
Review Comment:
+1 for adding comment for every public api.
##########
cpp/src/parquet/bloom_filter_reader.cc:
##########
@@ -0,0 +1,114 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/bloom_filter_reader.h"
+#include "metadata.h"
Review Comment:
```suggestion
#include "parquet/metadata.h"
```
##########
cpp/src/parquet/bloom_filter_reader.h:
##########
@@ -0,0 +1,51 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "parquet/bloom_filter.h"
+#include "parquet/encryption/internal_file_decryptor.h"
+
+namespace parquet {
+
+class PARQUET_EXPORT RowGroupBloomFilterReader {
+ public:
+ virtual ~RowGroupBloomFilterReader() = default;
+
+ virtual std::unique_ptr<BloomFilter> GetColumnBloomFilter(int i) = 0;
+};
+
+/// \brief Interface for reading the bloom filter for a Parquet file.
+class PARQUET_EXPORT BloomFilterReader {
Review Comment:
Do you plan to support an api to provide hints of successive reads for better I/O planning?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mapleFU commented on a diff in pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #34728:
URL: https://github.com/apache/arrow/pull/34728#discussion_r1148395302
##########
cpp/src/parquet/bloom_filter_reader.cc:
##########
@@ -0,0 +1,114 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/bloom_filter_reader.h"
+#include "metadata.h"
+#include "parquet/exception.h"
+
+namespace parquet {
+
+class RowGroupBloomFilterReaderImpl final : public RowGroupBloomFilterReader {
+ public:
+ RowGroupBloomFilterReaderImpl(std::shared_ptr<::arrow::io::RandomAccessFile> input,
+ std::shared_ptr<RowGroupMetaData> row_group_metadata,
+ const ReaderProperties& properties)
+ : input_(std::move(input)),
+ row_group_metadata_(std::move(row_group_metadata)),
+ properties_(properties) {}
+
+ std::unique_ptr<BloomFilter> GetColumnBloomFilter(int i) override;
+
+ private:
+ /// The input stream that can perform random access read.
+ std::shared_ptr<::arrow::io::RandomAccessFile> input_;
+
+ /// The row group metadata to get column chunk metadata.
+ std::shared_ptr<RowGroupMetaData> row_group_metadata_;
+
+ /// Reader properties used to deserialize thrift object.
+ const ReaderProperties& properties_;
+};
+
+std::unique_ptr<BloomFilter> RowGroupBloomFilterReaderImpl::GetColumnBloomFilter(int i) {
+ if (i < 0 || i >= row_group_metadata_->num_columns()) {
+ throw ParquetException("Invalid column index at column ordinal ", i);
+ }
+
+ auto col_chunk = row_group_metadata_->ColumnChunk(i);
+ std::unique_ptr<ColumnCryptoMetaData> crypto_metadata = col_chunk->crypto_metadata();
+ if (crypto_metadata != nullptr) {
+ ParquetException::NYI("Cannot read encrypted bloom filter yet");
+ }
+
+ auto bloom_filter_offset = col_chunk->bloom_filter_offset();
+ if (!bloom_filter_offset.has_value()) {
+ return nullptr;
+ }
+ PARQUET_ASSIGN_OR_THROW(auto file_size, input_->GetSize());
+ if (file_size <= *bloom_filter_offset) {
+ throw ParquetException("file size less or equal than bloom offset");
+ }
+ auto stream = ::arrow::io::RandomAccessFile::GetStream(
+ input_, *bloom_filter_offset, file_size - *bloom_filter_offset);
+ auto bloom_filter = BlockSplitBloomFilter::Deserialize(properties_, stream->get());
+ return std::make_unique<BlockSplitBloomFilter>(std::move(bloom_filter));
+}
+
+class BloomFilterReaderImpl final : public BloomFilterReader {
+ public:
+ BloomFilterReaderImpl(std::shared_ptr<::arrow::io::RandomAccessFile> input,
+ std::shared_ptr<FileMetaData> file_metadata,
+ const ReaderProperties& properties,
+ std::shared_ptr<InternalFileDecryptor> file_decryptor)
+ : input_(std::move(input)),
+ file_metadata_(std::move(file_metadata)),
+ properties_(properties) {
+ if (file_decryptor != nullptr) {
+ ParquetException::NYI("BloomFilter encryption not support");
+ }
+ }
+
+ std::shared_ptr<RowGroupBloomFilterReader> RowGroup(int i) {
+ if (i < 0 || i >= file_metadata_->num_row_groups()) {
+ throw ParquetException("Invalid row group ordinal: ", i);
+ }
+
+ auto row_group_metadata = file_metadata_->RowGroup(i);
+ return std::make_shared<RowGroupBloomFilterReaderImpl>(
Review Comment:
I guess we don't know if bloom-filter exists here. We can only get it from ColumnChunk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mapleFU commented on a diff in pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #34728:
URL: https://github.com/apache/arrow/pull/34728#discussion_r1150022654
##########
cpp/src/parquet/bloom_filter_reader.cc:
##########
@@ -0,0 +1,114 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/bloom_filter_reader.h"
+#include "parquet/exception.h"
+#include "parquet/metadata.h"
+
+namespace parquet {
+
+class RowGroupBloomFilterReaderImpl final : public RowGroupBloomFilterReader {
+ public:
+ RowGroupBloomFilterReaderImpl(std::shared_ptr<::arrow::io::RandomAccessFile> input,
+ std::shared_ptr<RowGroupMetaData> row_group_metadata,
+ const ReaderProperties& properties)
+ : input_(std::move(input)),
+ row_group_metadata_(std::move(row_group_metadata)),
+ properties_(properties) {}
+
+ std::unique_ptr<BloomFilter> GetColumnBloomFilter(int i) override;
+
+ private:
+ /// The input stream that can perform random access read.
+ std::shared_ptr<::arrow::io::RandomAccessFile> input_;
+
+ /// The row group metadata to get column chunk metadata.
+ std::shared_ptr<RowGroupMetaData> row_group_metadata_;
+
+ /// Reader properties used to deserialize thrift object.
+ const ReaderProperties& properties_;
+};
+
+std::unique_ptr<BloomFilter> RowGroupBloomFilterReaderImpl::GetColumnBloomFilter(int i) {
+ if (i < 0 || i >= row_group_metadata_->num_columns()) {
+ throw ParquetException("Invalid column index at column ordinal ", i);
+ }
+
+ auto col_chunk = row_group_metadata_->ColumnChunk(i);
+ std::unique_ptr<ColumnCryptoMetaData> crypto_metadata = col_chunk->crypto_metadata();
+ if (crypto_metadata != nullptr) {
+ ParquetException::NYI("Cannot read encrypted bloom filter yet");
+ }
+
+ auto bloom_filter_offset = col_chunk->bloom_filter_offset();
+ if (!bloom_filter_offset.has_value()) {
+ return nullptr;
+ }
+ PARQUET_ASSIGN_OR_THROW(auto file_size, input_->GetSize());
+ if (file_size <= *bloom_filter_offset) {
+ throw ParquetException("file size less or equal than bloom offset");
+ }
+ auto stream = ::arrow::io::RandomAccessFile::GetStream(
+ input_, *bloom_filter_offset, file_size - *bloom_filter_offset);
+ auto bloom_filter = BlockSplitBloomFilter::Deserialize(properties_, stream->get());
Review Comment:
I think current interface is ok, move SPBF is lightweight.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mapleFU commented on a diff in pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #34728:
URL: https://github.com/apache/arrow/pull/34728#discussion_r1148314444
##########
cpp/src/parquet/bloom_filter_reader.h:
##########
@@ -0,0 +1,51 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "parquet/bloom_filter.h"
+#include "parquet/encryption/internal_file_decryptor.h"
+
+namespace parquet {
+
+class PARQUET_EXPORT RowGroupBloomFilterReader {
+ public:
+ virtual ~RowGroupBloomFilterReader() = default;
+
+ virtual std::unique_ptr<BloomFilter> GetColumnBloomFilter(int i) = 0;
+};
+
+/// \brief Interface for reading the bloom filter for a Parquet file.
+class PARQUET_EXPORT BloomFilterReader {
+ public:
+ virtual ~BloomFilterReader() = default;
+
+ /// \brief Create a BloomFilterReader instance.
+ /// \returns a BloomFilterReader instance.
+ /// WARNING: The returned BloomFilterReader references to all the input parameters, so
Review Comment:
Yes, firstly I use `ReaderProperties` and `input`, but then I find it hard to build a `inputStream` for `BlockSplitBloomFilter::Deserialize`, so I change it to `shared_ptr`, but forgot to change the comment
##########
cpp/src/parquet/bloom_filter_reader.h:
##########
@@ -0,0 +1,51 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "parquet/bloom_filter.h"
+#include "parquet/encryption/internal_file_decryptor.h"
+
+namespace parquet {
+
+class PARQUET_EXPORT RowGroupBloomFilterReader {
+ public:
+ virtual ~RowGroupBloomFilterReader() = default;
+
+ virtual std::unique_ptr<BloomFilter> GetColumnBloomFilter(int i) = 0;
Review Comment:
Yes, I'll add comment for that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mapleFU commented on pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #34728:
URL: https://github.com/apache/arrow/pull/34728#issuecomment-1486192490
Python test failed:
```
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
> ???
E pyarrow._flight.FlightCancelledError: Flight cancelled call, with message: CANCELLED. gRPC client debug context: UNKNOWN:Error received from peer localhost:49542 {grpc_message:"CANCELLED", grpc_status:1, created_time:"2023-03-28T03:52:31.535724+00:00"}. Client context: IOError: Server never sent a data message. Detail: Internal
pyarrow/_flight.pyx:62: FlightCancelledError
----------------------------- Captured stderr call -----------------------------
E0328 03:52:31.504445000 123145443127296 ssl_transport_security.cc:1854] No match found for server name: fakehostname.
E0328 03:52:31.515087000 123145443127296 ssl_transport_security.cc:1854] No match found for server name: fakehostname.
=============================== warnings summary ===============================
tests/test_plasma.py::TestPlasmaClient::test_subscribe_socket
/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/_pytest/python.py:195: ResourceWarning: unclosed <socket.socket fd=303, family=1, type=1, proto=0>
result = testfunction(**testargs)
Enable tracemalloc to get traceback where the object was allocated.
See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info.
tests/test_substrait.py::test_named_table_invalid_table_name
/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/_pytest/unraisableexception.py:78: PytestUnraisableExceptionWarning: Exception ignored in: 'pyarrow._substrait._create_named_table_provider'
Traceback (most recent call last):
File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/pyarrow/tests/test_substrait.py", line 250, in table_provider
raise Exception("Unrecognized table name")
Exception: Unrecognized table name
warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
```
Seems this isn't caused by me?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mapleFU commented on pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #34728:
URL: https://github.com/apache/arrow/pull/34728#issuecomment-1486149769
Currently, we don't have BloomFilterWriter, so I can only test bloomfilter from `parquet-testing` repo. In my next patch, I'll add bloomfilter writer, and then we can have more tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34728:
URL: https://github.com/apache/arrow/pull/34728#issuecomment-1483344901
* Closes: #34665
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mapleFU commented on a diff in pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #34728:
URL: https://github.com/apache/arrow/pull/34728#discussion_r1148385793
##########
cpp/src/parquet/bloom_filter_reader.h:
##########
@@ -0,0 +1,51 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "parquet/bloom_filter.h"
+#include "parquet/encryption/internal_file_decryptor.h"
+
+namespace parquet {
+
+class PARQUET_EXPORT RowGroupBloomFilterReader {
+ public:
+ virtual ~RowGroupBloomFilterReader() = default;
+
+ virtual std::unique_ptr<BloomFilter> GetColumnBloomFilter(int i) = 0;
+};
+
+/// \brief Interface for reading the bloom filter for a Parquet file.
+class PARQUET_EXPORT BloomFilterReader {
Review Comment:
Currently, we don't know the size of BF, so, it's tricky to `WillNeed` here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wgtmac commented on a diff in pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #34728:
URL: https://github.com/apache/arrow/pull/34728#discussion_r1148385882
##########
cpp/src/parquet/bloom_filter_reader.h:
##########
@@ -0,0 +1,51 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "parquet/bloom_filter.h"
+#include "parquet/encryption/internal_file_decryptor.h"
+
+namespace parquet {
+
+class PARQUET_EXPORT RowGroupBloomFilterReader {
+ public:
+ virtual ~RowGroupBloomFilterReader() = default;
+
+ virtual std::unique_ptr<BloomFilter> GetColumnBloomFilter(int i) = 0;
+};
+
+/// \brief Interface for reading the bloom filter for a Parquet file.
+class PARQUET_EXPORT BloomFilterReader {
Review Comment:
Ah, I forgot that. Thanks for reminding me!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mapleFU commented on pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #34728:
URL: https://github.com/apache/arrow/pull/34728#issuecomment-1483858230
Comment fixed and test is added, mind take a look @wjones127 @wgtmac
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34728:
URL: https://github.com/apache/arrow/pull/34728#issuecomment-1489585606
Benchmark runs are scheduled for baseline = 9e092a99d2f4d6bd9d633d9b5ad76b884691f805 and contender = ddd0a337174e57cdc80b1ee30dc7e787acfc09f6. ddd0a337174e57cdc80b1ee30dc7e787acfc09f6 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/8919e689acb249cc9aec8be98d349015...966677bc24374757a873bdaac1ab198a/)
[Failed :arrow_down:1.04% :arrow_up:1.36%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/eb82ef4c0cd444ceb3e2ea72e699530b...cdadcaf2281741c1a26e85d0c9ff8572/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/642491fb52684eefa3ea4eefc78f320d...4b1c122cd6d84248b82ba8b8c1b064f1/)
[Finished :arrow_down:0.65% :arrow_up:0.03%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/5cc5a60467fb45009c0f9cdecfc8e368...a58fe1082bae4792822f321194e9dd59/)
Buildkite builds:
[Finished] [`ddd0a337` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2595)
[Finished] [`ddd0a337` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2625)
[Finished] [`ddd0a337` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2593)
[Finished] [`ddd0a337` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2616)
[Finished] [`9e092a99` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2594)
[Failed] [`9e092a99` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2624)
[Finished] [`9e092a99` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2592)
[Finished] [`9e092a99` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2615)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34728:
URL: https://github.com/apache/arrow/pull/34728#discussion_r1149623978
##########
cpp/src/parquet/file_reader.h:
##########
@@ -146,6 +148,11 @@ class PARQUET_EXPORT ParquetFileReader {
/// WARNING: The returned PageIndexReader must not outlive the ParquetFileReader.
std::shared_ptr<PageIndexReader> GetPageIndexReader();
+ /// Returns the BloomFilterReader. Only one instance is ever created.
+ ///
+ /// WARNING: The returned BloomFilterReader must not outlive the ParquetFileReader.
Review Comment:
Should we return a raw pointer (or maybe const reference) instead of a `shared_ptr`? Would it be more clear that the lifetime of the `BloomFilterReader` is bound to the lifetime of the `ParquetFileReader`.
##########
cpp/src/parquet/bloom_filter_reader_test.cc:
##########
@@ -0,0 +1,59 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+
+#include "arrow/testing/gtest_util.h"
+
+#include "parquet/bloom_filter_reader.h"
+#include "parquet/file_reader.h"
+#include "parquet/test_util.h"
+
+namespace parquet {
+namespace test {
+
+TEST(BloomFilterReader, ReadBloomFilter) {
+ std::string dir_string(parquet::test::get_data_dir());
+ std::string path = dir_string + "/data_index_bloom_encoding_stats.parquet";
+ auto reader = ParquetFileReader::OpenFile(path, false);
+ auto file_metadata = reader->metadata();
Review Comment:
Is this used?
##########
cpp/src/parquet/bloom_filter_reader_test.cc:
##########
@@ -0,0 +1,59 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+
+#include "arrow/testing/gtest_util.h"
+
+#include "parquet/bloom_filter_reader.h"
+#include "parquet/file_reader.h"
+#include "parquet/test_util.h"
+
+namespace parquet {
+namespace test {
+
+TEST(BloomFilterReader, ReadBloomFilter) {
Review Comment:
Can we add a test case for a file that does not have a bloom filter?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wgtmac commented on a diff in pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #34728:
URL: https://github.com/apache/arrow/pull/34728#discussion_r1150007066
##########
cpp/src/parquet/CMakeLists.txt:
##########
@@ -339,6 +340,7 @@ add_parquet_test(internals-test
encoding_test.cc
metadata_test.cc
page_index_test.cc
+ bloom_filter_reader_test.cc
Review Comment:
ditto
##########
cpp/src/parquet/bloom_filter_reader.cc:
##########
@@ -0,0 +1,114 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/bloom_filter_reader.h"
+#include "parquet/exception.h"
+#include "parquet/metadata.h"
+
+namespace parquet {
+
+class RowGroupBloomFilterReaderImpl final : public RowGroupBloomFilterReader {
+ public:
+ RowGroupBloomFilterReaderImpl(std::shared_ptr<::arrow::io::RandomAccessFile> input,
+ std::shared_ptr<RowGroupMetaData> row_group_metadata,
+ const ReaderProperties& properties)
+ : input_(std::move(input)),
+ row_group_metadata_(std::move(row_group_metadata)),
+ properties_(properties) {}
+
+ std::unique_ptr<BloomFilter> GetColumnBloomFilter(int i) override;
+
+ private:
+ /// The input stream that can perform random access read.
+ std::shared_ptr<::arrow::io::RandomAccessFile> input_;
+
+ /// The row group metadata to get column chunk metadata.
+ std::shared_ptr<RowGroupMetaData> row_group_metadata_;
+
+ /// Reader properties used to deserialize thrift object.
+ const ReaderProperties& properties_;
+};
+
+std::unique_ptr<BloomFilter> RowGroupBloomFilterReaderImpl::GetColumnBloomFilter(int i) {
+ if (i < 0 || i >= row_group_metadata_->num_columns()) {
+ throw ParquetException("Invalid column index at column ordinal ", i);
+ }
+
+ auto col_chunk = row_group_metadata_->ColumnChunk(i);
+ std::unique_ptr<ColumnCryptoMetaData> crypto_metadata = col_chunk->crypto_metadata();
+ if (crypto_metadata != nullptr) {
+ ParquetException::NYI("Cannot read encrypted bloom filter yet");
+ }
+
+ auto bloom_filter_offset = col_chunk->bloom_filter_offset();
+ if (!bloom_filter_offset.has_value()) {
+ return nullptr;
+ }
+ PARQUET_ASSIGN_OR_THROW(auto file_size, input_->GetSize());
+ if (file_size <= *bloom_filter_offset) {
+ throw ParquetException("file size less or equal than bloom offset");
+ }
+ auto stream = ::arrow::io::RandomAccessFile::GetStream(
+ input_, *bloom_filter_offset, file_size - *bloom_filter_offset);
+ auto bloom_filter = BlockSplitBloomFilter::Deserialize(properties_, stream->get());
+ return std::make_unique<BlockSplitBloomFilter>(std::move(bloom_filter));
+}
+
+class BloomFilterReaderImpl final : public BloomFilterReader {
+ public:
+ BloomFilterReaderImpl(std::shared_ptr<::arrow::io::RandomAccessFile> input,
+ std::shared_ptr<FileMetaData> file_metadata,
+ const ReaderProperties& properties,
+ std::shared_ptr<InternalFileDecryptor> file_decryptor)
+ : input_(std::move(input)),
+ file_metadata_(std::move(file_metadata)),
+ properties_(properties) {
+ if (file_decryptor != nullptr) {
+ ParquetException::NYI("BloomFilter encryption not support");
Review Comment:
```suggestion
ParquetException::NYI("BloomFilter decryption is not yet supported");
```
##########
cpp/src/parquet/file_reader.cc:
##########
@@ -319,6 +321,25 @@ class SerializedFile : public ParquetFileReader::Contents {
return page_index_reader_;
}
+ BloomFilterReader& GetBloomFilterReader() override {
+ if (!file_metadata_) {
+ // Usually this won't happen if user calls one of the static Open() functions
+ // to create a ParquetFileReader instance. But if user calls the constructor
+ // directly and calls GetBloomFilterReader() before Open() then this could happen.
+ throw ParquetException(
+ "Cannot call GetBloomFilterReader() due to missing file metadata. Did you "
+ "forget to call ParquetFileReader::Open() first?");
+ }
+ if (!bloom_filter_reader_) {
Review Comment:
We need to document its thread safety.
##########
cpp/src/parquet/file_reader.h:
##########
@@ -146,6 +148,11 @@ class PARQUET_EXPORT ParquetFileReader {
/// WARNING: The returned PageIndexReader must not outlive the ParquetFileReader.
std::shared_ptr<PageIndexReader> GetPageIndexReader();
+ /// Returns the BloomFilterReader. Only one instance is ever created.
+ ///
+ /// WARNING: The returned BloomFilterReader must not outlive the ParquetFileReader.
Review Comment:
I'd prefer returning a `const BloomFilterReader*` or `BloomFilterReader*` so we can have an optimization to return a nullptr when we are sure none of bloom filter exists.
##########
cpp/src/parquet/CMakeLists.txt:
##########
@@ -170,6 +170,7 @@ set(PARQUET_SRCS
metadata.cc
xxhasher.cc
page_index.cc
+ bloom_filter_reader.cc
Review Comment:
please sort it alphabetically
##########
cpp/src/parquet/file_reader.cc:
##########
@@ -33,6 +33,8 @@
#include "arrow/util/int_util_overflow.h"
#include "arrow/util/logging.h"
#include "arrow/util/ubsan.h"
+#include "parquet/bloom_filter.h"
Review Comment:
This is already included in the bloom_filter_reader.h and can be removed safely here.
##########
cpp/src/parquet/file_reader.cc:
##########
@@ -540,6 +561,7 @@ class SerializedFile : public ParquetFileReader::Contents {
std::shared_ptr<FileMetaData> file_metadata_;
ReaderProperties properties_;
std::shared_ptr<PageIndexReader> page_index_reader_;
+ std::shared_ptr<BloomFilterReader> bloom_filter_reader_;
Review Comment:
is unique_ptr enough?
##########
cpp/src/parquet/bloom_filter_reader.h:
##########
@@ -0,0 +1,64 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include "parquet/bloom_filter.h"
+#include "parquet/encryption/internal_file_decryptor.h"
Review Comment:
Use forward declaration here and move it to source file.
##########
cpp/src/parquet/file_reader.h:
##########
@@ -33,6 +33,7 @@ namespace parquet {
class ColumnReader;
class FileMetaData;
class PageIndexReader;
+class BloomFilterReader;
Review Comment:
sort it alphabetically
##########
cpp/src/parquet/bloom_filter_reader.cc:
##########
@@ -0,0 +1,114 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/bloom_filter_reader.h"
+#include "parquet/exception.h"
+#include "parquet/metadata.h"
+
+namespace parquet {
+
+class RowGroupBloomFilterReaderImpl final : public RowGroupBloomFilterReader {
+ public:
+ RowGroupBloomFilterReaderImpl(std::shared_ptr<::arrow::io::RandomAccessFile> input,
+ std::shared_ptr<RowGroupMetaData> row_group_metadata,
+ const ReaderProperties& properties)
+ : input_(std::move(input)),
+ row_group_metadata_(std::move(row_group_metadata)),
+ properties_(properties) {}
+
+ std::unique_ptr<BloomFilter> GetColumnBloomFilter(int i) override;
+
+ private:
+ /// The input stream that can perform random access read.
+ std::shared_ptr<::arrow::io::RandomAccessFile> input_;
+
+ /// The row group metadata to get column chunk metadata.
+ std::shared_ptr<RowGroupMetaData> row_group_metadata_;
+
+ /// Reader properties used to deserialize thrift object.
+ const ReaderProperties& properties_;
+};
+
+std::unique_ptr<BloomFilter> RowGroupBloomFilterReaderImpl::GetColumnBloomFilter(int i) {
+ if (i < 0 || i >= row_group_metadata_->num_columns()) {
+ throw ParquetException("Invalid column index at column ordinal ", i);
+ }
+
+ auto col_chunk = row_group_metadata_->ColumnChunk(i);
+ std::unique_ptr<ColumnCryptoMetaData> crypto_metadata = col_chunk->crypto_metadata();
+ if (crypto_metadata != nullptr) {
+ ParquetException::NYI("Cannot read encrypted bloom filter yet");
+ }
+
+ auto bloom_filter_offset = col_chunk->bloom_filter_offset();
+ if (!bloom_filter_offset.has_value()) {
+ return nullptr;
+ }
+ PARQUET_ASSIGN_OR_THROW(auto file_size, input_->GetSize());
+ if (file_size <= *bloom_filter_offset) {
+ throw ParquetException("file size less or equal than bloom offset");
+ }
+ auto stream = ::arrow::io::RandomAccessFile::GetStream(
+ input_, *bloom_filter_offset, file_size - *bloom_filter_offset);
+ auto bloom_filter = BlockSplitBloomFilter::Deserialize(properties_, stream->get());
Review Comment:
How about changing its signature to return `std::unique_ptr<BlockSplitBloomFilter>` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mapleFU commented on a diff in pull request #34728: GH-34665: [Parquet][C++] Allow Reading BloomFilter
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #34728:
URL: https://github.com/apache/arrow/pull/34728#discussion_r1150032460
##########
cpp/src/parquet/file_reader.cc:
##########
@@ -540,6 +561,7 @@ class SerializedFile : public ParquetFileReader::Contents {
std::shared_ptr<FileMetaData> file_metadata_;
ReaderProperties properties_;
std::shared_ptr<PageIndexReader> page_index_reader_;
+ std::shared_ptr<BloomFilterReader> bloom_filter_reader_;
Review Comment:
Yes, let me update it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org