pitrou commented on a change in pull request #8305:
URL: https://github.com/apache/arrow/pull/8305#discussion_r500202061
##########
File path: cpp/src/arrow/util/mutex.h
##########
@@ -31,16 +31,20 @@ namespace util {
class ARROW_EXPORT Mutex {
public:
Mutex();
+ Mutex(Mutex&&) = default;
+ Mutex& operator=(Mutex&&) = default;
/// A Guard is falsy if a lock could not be acquired.
- class Guard {
+ class ARROW_EXPORT Guard {
public:
Guard() : locked_(NULLPTR, [](Mutex* mutex) {}) {}
Guard(Guard&&) = default;
Guard& operator=(Guard&&) = default;
explicit operator bool() const { return bool(locked_); }
+ void Unlock();
Review comment:
I would expect a `Lock()` method as well.
##########
File path: cpp/src/arrow/dataset/file_ipc.cc
##########
@@ -159,18 +163,44 @@ Result<ScanTaskIterator>
IpcFileFormat::ScanFile(std::shared_ptr<ScanOptions> op
fragment->source());
}
-Status IpcFileFormat::WriteFragment(RecordBatchReader* batches,
- io::OutputStream* destination) const {
- ARROW_ASSIGN_OR_RAISE(auto writer, ipc::MakeFileWriter(destination,
batches->schema()));
+///
+/// IpcFileWriter, IpcFileWriteOptions
+///
Review comment:
Avoid triple-slashes for regular comments.
##########
File path: cpp/src/arrow/util/string.h
##########
@@ -56,5 +57,9 @@ bool AsciiEqualsCaseInsensitive(util::string_view left,
util::string_view right)
ARROW_EXPORT
std::string AsciiToLower(util::string_view value);
+ARROW_EXPORT
Review comment:
Add a docstring? Especially to explain if only one replacement is done.
##########
File path: cpp/src/arrow/dataset/file_parquet.cc
##########
@@ -440,6 +427,48 @@ Result<std::shared_ptr<FileFragment>>
ParquetFileFormat::MakeFragment(
std::move(physical_schema), {}));
}
+///
Review comment:
Same here.
##########
File path: cpp/src/arrow/util/mutex.cc
##########
@@ -19,25 +19,36 @@
#include <mutex>
+#include "arrow/util/logging.h"
+
namespace arrow {
namespace util {
-struct Mutex::Impl {
- std::mutex mutex_;
-};
+struct Mutex::Impl : std::mutex {};
Review comment:
I'd really prefer we keep using containment instead of inheritance here.
This will make things less confusing.
##########
File path: cpp/src/arrow/dataset/file_base.h
##########
@@ -128,6 +128,9 @@ class ARROW_DS_EXPORT FileFormat : public
std::enable_shared_from_this<FileForma
/// \brief Return true if fragments of this format can benefit from parallel
scanning.
virtual bool splittable() const { return false; }
+ virtual bool Equals(const FileFormat& other) const = 0;
+ // virtual std::string ToString() const = 0;
Review comment:
Is this a leftover?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
