You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by we...@apache.org on 2020/06/12 16:26:04 UTC
[arrow] branch master updated: ARROW-8510: [C++][Datasets] Do not
use variant in WritePlan to fix compiler error with VS 2017
This is an automated email from the ASF dual-hosted git repository.
wesm pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/master by this push:
new 2c3e84d ARROW-8510: [C++][Datasets] Do not use variant in WritePlan to fix compiler error with VS 2017
2c3e84d is described below
commit 2c3e84d8d1c7b03477691ebe34b3ff0663f4e8d5
Author: Wes McKinney <we...@apache.org>
AuthorDate: Fri Jun 12 11:25:44 2020 -0500
ARROW-8510: [C++][Datasets] Do not use variant in WritePlan to fix compiler error with VS 2017
Even without the compiler error on VS 2017 I am not a fan of using variant to solve problems like these (so to me this new code is strictly preferable to the old code), nor exposing a variant in public (or "public") APIs unless it is definitely needed.
Closes #7419 from wesm/ARROW-8510
Authored-by: Wes McKinney <we...@apache.org>
Signed-off-by: Wes McKinney <we...@apache.org>
---
cpp/src/arrow/dataset/file_base.cc | 4 +--
cpp/src/arrow/dataset/file_base.h | 53 +++++++++++++++++++++++----------
cpp/src/arrow/dataset/partition_test.cc | 26 ++++++----------
3 files changed, 49 insertions(+), 34 deletions(-)
diff --git a/cpp/src/arrow/dataset/file_base.cc b/cpp/src/arrow/dataset/file_base.cc
index 9d3a427..8f663bb 100644
--- a/cpp/src/arrow/dataset/file_base.cc
+++ b/cpp/src/arrow/dataset/file_base.cc
@@ -162,10 +162,10 @@ Result<std::shared_ptr<FileSystemDataset>> FileSystemDataset::Write(
std::vector<std::shared_ptr<FileFragment>> fragments;
for (size_t i = 0; i < plan.paths.size(); ++i) {
const auto& op = plan.fragment_or_partition_expressions[i];
- if (util::holds_alternative<std::shared_ptr<Fragment>>(op)) {
+ if (op.kind() == WritePlan::FragmentOrPartitionExpression::FRAGMENT) {
auto path = partition_base_dir + plan.paths[i] + extension;
- const auto& input_fragment = util::get<std::shared_ptr<Fragment>>(op);
+ const auto& input_fragment = op.fragment();
FileSource dest(path, filesystem);
ARROW_ASSIGN_OR_RAISE(
diff --git a/cpp/src/arrow/dataset/file_base.h b/cpp/src/arrow/dataset/file_base.h
index 18c641a..9d747b4 100644
--- a/cpp/src/arrow/dataset/file_base.h
+++ b/cpp/src/arrow/dataset/file_base.h
@@ -35,7 +35,6 @@
#include "arrow/filesystem/path_forest.h"
#include "arrow/io/file.h"
#include "arrow/util/compression.h"
-#include "arrow/util/variant.h"
namespace arrow {
@@ -51,17 +50,22 @@ class ARROW_DS_EXPORT FileSource {
FileSource(std::string path, std::shared_ptr<fs::FileSystem> filesystem,
Compression::type compression = Compression::UNCOMPRESSED,
bool writable = true)
- : impl_(PathAndFileSystem{std::move(path), std::move(filesystem)}),
+ : source_kind_(PATH),
+ path_(std::move(path)),
+ filesystem_(std::move(filesystem)),
compression_(compression),
writable_(writable) {}
explicit FileSource(std::shared_ptr<Buffer> buffer,
Compression::type compression = Compression::UNCOMPRESSED)
- : impl_(std::move(buffer)), compression_(compression) {}
+ : source_kind_(BUFFER), buffer_(std::move(buffer)), compression_(compression) {}
explicit FileSource(std::shared_ptr<ResizableBuffer> buffer,
Compression::type compression = Compression::UNCOMPRESSED)
- : impl_(std::move(buffer)), compression_(compression), writable_(true) {}
+ : source_kind_(BUFFER),
+ buffer_(std::move(buffer)),
+ compression_(compression),
+ writable_(true) {}
bool operator==(const FileSource& other) const {
if (id() != other.id()) {
@@ -77,7 +81,7 @@ class ARROW_DS_EXPORT FileSource {
/// \brief The kind of file, whether stored in a filesystem, memory
/// resident, or other
- SourceKind id() const { return static_cast<SourceKind>(impl_.index()); }
+ SourceKind id() const { return source_kind_; }
/// \brief Return the type of raw compression on the file, if any
Compression::type compression() const { return compression_; }
@@ -89,21 +93,21 @@ class ARROW_DS_EXPORT FileSource {
/// type is PATH
const std::string& path() const {
static std::string buffer_path = "<Buffer>";
- return id() == PATH ? util::get<PATH>(impl_).path : buffer_path;
+ return id() == PATH ? path_ : buffer_path;
}
/// \brief Return the filesystem, if any. Only non null when file
/// source type is PATH
const std::shared_ptr<fs::FileSystem>& filesystem() const {
static std::shared_ptr<fs::FileSystem> no_fs = NULLPTR;
- return id() == PATH ? util::get<PATH>(impl_).filesystem : no_fs;
+ return id() == PATH ? filesystem_ : no_fs;
}
/// \brief Return the buffer containing the file, if any. Only value
/// when file source type is BUFFER
const std::shared_ptr<Buffer>& buffer() const {
static std::shared_ptr<Buffer> path_buffer = NULLPTR;
- return id() == BUFFER ? util::get<BUFFER>(impl_) : path_buffer;
+ return id() == BUFFER ? buffer_ : path_buffer;
}
/// \brief Get a RandomAccessFile which views this file source
@@ -113,12 +117,13 @@ class ARROW_DS_EXPORT FileSource {
Result<std::shared_ptr<arrow::io::OutputStream>> OpenWritable() const;
private:
- struct PathAndFileSystem {
- std::string path;
- std::shared_ptr<fs::FileSystem> filesystem;
- };
+ SourceKind source_kind_;
+
+ std::string path_;
+ std::shared_ptr<fs::FileSystem> filesystem_;
+
+ std::shared_ptr<Buffer> buffer_;
- util::variant<PathAndFileSystem, std::shared_ptr<Buffer>> impl_;
Compression::type compression_;
bool writable_ = false;
};
@@ -280,8 +285,26 @@ class ARROW_DS_EXPORT WritePlan {
std::shared_ptr<fs::FileSystem> filesystem;
std::string partition_base_dir;
- using FragmentOrPartitionExpression =
- util::variant<std::shared_ptr<Expression>, std::shared_ptr<Fragment>>;
+ class FragmentOrPartitionExpression {
+ public:
+ enum Kind { EXPRESSION, FRAGMENT };
+
+ explicit FragmentOrPartitionExpression(std::shared_ptr<Expression> partition_expr)
+ : kind_(EXPRESSION), partition_expr_(std::move(partition_expr)) {}
+
+ explicit FragmentOrPartitionExpression(std::shared_ptr<Fragment> fragment)
+ : kind_(FRAGMENT), fragment_(std::move(fragment)) {}
+
+ Kind kind() const { return kind_; }
+
+ const std::shared_ptr<Expression>& partition_expr() const { return partition_expr_; }
+ const std::shared_ptr<Fragment>& fragment() const { return fragment_; }
+
+ private:
+ Kind kind_;
+ std::shared_ptr<Expression> partition_expr_;
+ std::shared_ptr<Fragment> fragment_;
+ };
/// If fragment_or_partition_expressions[i] is a Fragment, that Fragment will be
/// written to paths[i]. If it is an Expression, a directory representing that partition
diff --git a/cpp/src/arrow/dataset/partition_test.cc b/cpp/src/arrow/dataset/partition_test.cc
index a08345f..ceb6a11 100644
--- a/cpp/src/arrow/dataset/partition_test.cc
+++ b/cpp/src/arrow/dataset/partition_test.cc
@@ -350,28 +350,20 @@ class TestPartitioningWritePlan : public ::testing::Test {
ExpectedWritePlan() = default;
ExpectedWritePlan(const WritePlan& actual_plan, const FragmentVector& fragments) {
- struct {
- int i;
- ExpectedWritePlan* this_;
- const FragmentVector& fragments;
- const WritePlan& actual_plan;
-
- void operator()(const std::shared_ptr<Fragment>& fragment) {
+ int i = 0;
+ for (const auto& op : actual_plan.fragment_or_partition_expressions) {
+ if (op.kind() == WritePlan::FragmentOrPartitionExpression::FRAGMENT) {
+ auto fragment = op.fragment();
auto fragment_index =
static_cast<int>(std::find(fragments.begin(), fragments.end(), fragment) -
fragments.begin());
auto path = fs::internal::GetAbstractPathParent(actual_plan.paths[i]).first;
- this_->dirs_[path + "/"].fragments.push_back(fragment_index);
- }
-
- void operator()(const std::shared_ptr<Expression>& partition_expression) {
- this_->dirs_[actual_plan.paths[i]].partition_expression = partition_expression;
+ dirs_[path + "/"].fragments.push_back(fragment_index);
+ } else {
+ auto partition_expression = op.partition_expr();
+ dirs_[actual_plan.paths[i]].partition_expression = partition_expression;
}
- } actual = {0, this, fragments, actual_plan};
-
- for (const auto& op : actual_plan.fragment_or_partition_expressions) {
- util::visit(actual, op);
- ++actual.i;
+ ++i;
}
}