kou commented on code in PR #15049:
URL: https://github.com/apache/arrow/pull/15049#discussion_r1054793482
##########
cpp/src/arrow/dataset/file_base.h:
##########
@@ -200,6 +200,9 @@ class ARROW_DS_EXPORT FileFormat : public
std::enable_shared_from_this<FileForma
fs::FileLocator destination_locator) const = 0;
/// \brief Get default write options for this format.
+ ///
+ /// May return nullptr if this file format does not yet support writing
Review Comment:
How about using `empty shared_ptr` instead of `nullptr`?
This method always returns `std::shared_ptr<FileWriteOptions>` but this may
return `std::shared_ptr<FileWriteOptions>` that stores `nullptr`.
It seems that a `std::shared_ptr` that stores `nullptr` is called `empty`:
https://en.cppreference.com/w/cpp/memory/shared_ptr
> A shared_ptr may also own no objects, in which case it is called empty
##########
python/pyarrow/_dataset.pyx:
##########
@@ -775,6 +775,14 @@ cdef class FileWriteOptions(_Weakrefable):
@staticmethod
cdef wrap(const shared_ptr[CFileWriteOptions]& sp):
+
+ if sp.get() == nullptr:
+ # DefaultWriteOptions() may return `nullptr` which means that
+ # the format does not yet support writing datasets.
+ raise NotImplementedError(
+ "Writing datasets not yet implemented for this file format."
+ )
Review Comment:
How about doing this in `FileFormat.make_write_options()` not here?
```diff
diff --git a/python/pyarrow/_dataset.pyx b/python/pyarrow/_dataset.pyx
index 26c9f503bd..b2512806c3 100644
--- a/python/pyarrow/_dataset.pyx
+++ b/python/pyarrow/_dataset.pyx
@@ -887,7 +887,14 @@ cdef class FileFormat(_Weakrefable):
return Fragment.wrap(move(c_fragment))
def make_write_options(self):
- return FileWriteOptions.wrap(self.format.DefaultWriteOptions())
+ sp_write_options = self.format.DefaultWriteOptions()
+ if sp_write_options.get() == nullptr:
+ # DefaultWriteOptions() may return `nullptr` which means that
+ # the format does not yet support writing datasets.
+ raise NotImplementedError(
+ "Writing datasets not yet implemented for this file format."
+ )
+ return FileWriteOptions.wrap(sp_write_options)
@property
def default_extname(self):
```
If we should check a return value of `WriteDefaultFormat()`, we should do it
where `WriteDefaultFormats()` is used.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
