Skip to content

Commit

Permalink
Remove SeekableInputStream.h include from FormatData.h (#10859)
Browse files Browse the repository at this point in the history
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 <google/protobuf/io/zero_copy_stream.h>
```
See prestodb/presto#23528

Pull Request resolved: #10859

Reviewed By: xiaoxmeng

Differential Revision: D61936364

Pulled By: kgpai

fbshipit-source-id: ec7c913110fe0a38a5ba00a37addbfeac55e7289
  • Loading branch information
majetideepak authored and facebook-github-bot committed Sep 10, 2024
1 parent de734a0 commit 7e4d626
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 26 deletions.
10 changes: 2 additions & 8 deletions velox/dwio/common/Closeable.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -67,7 +64,4 @@ class Closeable {
bool closed_;
};

} // namespace common
} // namespace dwio
} // namespace velox
} // namespace facebook
} // namespace facebook::velox::dwio::common
4 changes: 2 additions & 2 deletions velox/dwio/common/FormatData.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down
37 changes: 37 additions & 0 deletions velox/dwio/common/PositionProvider.h
Original file line number Diff line number Diff line change
@@ -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 <vector>

namespace facebook::velox::dwio::common {

class PositionProvider {
public:
explicit PositionProvider(const std::vector<uint64_t>& positions)
: position_{positions.begin()}, end_{positions.end()} {}

uint64_t next();

bool hasNext() const;

private:
std::vector<uint64_t>::const_iterator position_;
std::vector<uint64_t>::const_iterator end_;
};

} // namespace facebook::velox::dwio::common
17 changes: 1 addition & 16 deletions velox/dwio/common/SeekableInputStream.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,30 +16,15 @@

#pragma once

#include <vector>

#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<uint64_t>& positions)
: position_{positions.begin()}, end_{positions.end()} {}

uint64_t next();

bool hasNext() const;

private:
std::vector<uint64_t>::const_iterator position_;
std::vector<uint64_t>::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
Expand Down

0 comments on commit 7e4d626

Please sign in to comment.