From 7e4d6263cac225e830369e5b4951be12040a2e43 Mon Sep 17 00:00:00 2001 From: Deepak Majeti Date: Tue, 10 Sep 2024 10:36:45 -0700 Subject: [PATCH] Remove SeekableInputStream.h include from FormatData.h (#10859) Summary: SeekableInputStream.h is causing the protobuf dependency to leak into Velox clients. Move the PositionProvider class to its own file. ``` In file included from /Users/reetikaagrawal/Documents/Projects/Oss/prestodb-hivepoc/prestodb/presto-native-execution/velox/velox/connectors/hive/iceberg/IcebergSplitReader.cpp:17: In file included from /Users/reetikaagrawal/Documents/Projects/Oss/prestodb-hivepoc/prestodb/presto-native-execution/velox/velox/connectors/hive/iceberg/IcebergSplitReader.h:21: In file included from /Users/reetikaagrawal/Documents/Projects/Oss/prestodb-hivepoc/prestodb/presto-native-execution/velox/velox/connectors/hive/iceberg/PositionalDeleteFileReader.h:26: In file included from /Users/reetikaagrawal/Documents/Projects/Oss/prestodb-hivepoc/prestodb/presto-native-execution/velox/velox/dwio/common/Reader.h:27: In file included from /Users/reetikaagrawal/Documents/Projects/Oss/prestodb-hivepoc/prestodb/presto-native-execution/velox/velox/dwio/common/SelectiveColumnReader.h:22: In file included from /Users/reetikaagrawal/Documents/Projects/Oss/prestodb-hivepoc/prestodb/presto-native-execution/velox/velox/dwio/common/FormatData.h:21: In file included from /Users/reetikaagrawal/Documents/Projects/Oss/prestodb-hivepoc/prestodb/presto-native-execution/velox/velox/dwio/common/SeekableInputStream.h:23: /Users/reetikaagrawal/Documents/Projects/Oss/prestodb-hivepoc/prestodb/presto-native-execution/velox/velox/dwio/common/wrap/zero-copy-stream-wrapper.h:31:10: fatal error: 'google/protobuf/io/zero_copy_stream.h' file not found #include ``` See https://github.com/prestodb/presto/issues/23528 Pull Request resolved: https://github.com/facebookincubator/velox/pull/10859 Reviewed By: xiaoxmeng Differential Revision: D61936364 Pulled By: kgpai fbshipit-source-id: ec7c913110fe0a38a5ba00a37addbfeac55e7289 --- velox/dwio/common/Closeable.h | 10 ++----- velox/dwio/common/FormatData.h | 4 +-- velox/dwio/common/PositionProvider.h | 37 +++++++++++++++++++++++++ velox/dwio/common/SeekableInputStream.h | 17 +----------- 4 files changed, 42 insertions(+), 26 deletions(-) create mode 100644 velox/dwio/common/PositionProvider.h diff --git a/velox/dwio/common/Closeable.h b/velox/dwio/common/Closeable.h index 4572e08cf98a..bdb825437889 100644 --- a/velox/dwio/common/Closeable.h +++ b/velox/dwio/common/Closeable.h @@ -18,10 +18,7 @@ #include "velox/dwio/common/exception/Exception.h" -namespace facebook { -namespace velox { -namespace dwio { -namespace common { +namespace facebook::velox::dwio::common { // Base class for closeable object which need to be explicitly closed before // being destructed @@ -67,7 +64,4 @@ class Closeable { bool closed_; }; -} // namespace common -} // namespace dwio -} // namespace velox -} // namespace facebook +} // namespace facebook::velox::dwio::common diff --git a/velox/dwio/common/FormatData.h b/velox/dwio/common/FormatData.h index d0d37006a2a8..bff3c420bc50 100644 --- a/velox/dwio/common/FormatData.h +++ b/velox/dwio/common/FormatData.h @@ -17,8 +17,8 @@ #pragma once #include "velox/common/memory/Memory.h" +#include "velox/dwio/common/PositionProvider.h" #include "velox/dwio/common/ScanSpec.h" -#include "velox/dwio/common/SeekableInputStream.h" #include "velox/dwio/common/Statistics.h" #include "velox/dwio/common/TypeWithId.h" #include "velox/type/Filter.h" @@ -40,7 +40,7 @@ class FormatData { /// data. If there are no nulls, 'nulls' is set to nullptr, else to /// a suitable sized and padded Buffer. 'incomingNulls' may be given /// if there are enclosing level nulls that should be merged into - /// the read reasult. If provided, this has 'numValues' bits and + /// the read result. If provided, this has 'numValues' bits and /// each zero marks an incoming null for which no bit is read from /// the nulls stream of 'this'. For Parquet, 'nulls' is always set /// to nullptr because nulls are represented by the data pages diff --git a/velox/dwio/common/PositionProvider.h b/velox/dwio/common/PositionProvider.h new file mode 100644 index 000000000000..7be3bc7a1602 --- /dev/null +++ b/velox/dwio/common/PositionProvider.h @@ -0,0 +1,37 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed 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 + +namespace facebook::velox::dwio::common { + +class PositionProvider { + public: + explicit PositionProvider(const std::vector& positions) + : position_{positions.begin()}, end_{positions.end()} {} + + uint64_t next(); + + bool hasNext() const; + + private: + std::vector::const_iterator position_; + std::vector::const_iterator end_; +}; + +} // namespace facebook::velox::dwio::common diff --git a/velox/dwio/common/SeekableInputStream.h b/velox/dwio/common/SeekableInputStream.h index 021d78493ab6..ab402753b8c5 100644 --- a/velox/dwio/common/SeekableInputStream.h +++ b/velox/dwio/common/SeekableInputStream.h @@ -16,30 +16,15 @@ #pragma once -#include - #include "velox/dwio/common/DataBuffer.h" #include "velox/dwio/common/InputStream.h" +#include "velox/dwio/common/PositionProvider.h" #include "velox/dwio/common/wrap/zero-copy-stream-wrapper.h" namespace facebook::velox::dwio::common { void printBuffer(std::ostream& out, const char* buffer, uint64_t length); -class PositionProvider { - public: - explicit PositionProvider(const std::vector& positions) - : position_{positions.begin()}, end_{positions.end()} {} - - uint64_t next(); - - bool hasNext() const; - - private: - std::vector::const_iterator position_; - std::vector::const_iterator end_; -}; - /** * A subclass of Google's ZeroCopyInputStream that supports seek. * By extending Google's class, we get the ability to pass it directly