amol- commented on code in PR #13342:
URL: https://github.com/apache/arrow/pull/13342#discussion_r896601428
##########
python/pyarrow/ipc.pxi:
##########
@@ -792,10 +793,10 @@ cdef class _RecordBatchStreamReader(RecordBatchReader):
pass
def _open(self, source, IpcReadOptions options=IpcReadOptions(),
- MemoryPool memory_pool=None):
+ MemoryPool memory_pool=None, use_memory_map=False):
Review Comment:
Let's not change the signature of methods, here too one can pass a
`MemoryMappedFile`.
```suggestion
MemoryPool memory_pool=None):
```
##########
python/pyarrow/ipc.pxi:
##########
@@ -839,15 +840,15 @@ cdef class _RecordBatchFileReader(_Weakrefable):
def _open(self, source, footer_offset=None,
IpcReadOptions options=IpcReadOptions(),
- MemoryPool memory_pool=None):
+ MemoryPool memory_pool=None, use_memory_map=False):
Review Comment:
```suggestion
MemoryPool memory_pool=None):
```
##########
python/pyarrow/ipc.pxi:
##########
@@ -839,15 +840,15 @@ cdef class _RecordBatchFileReader(_Weakrefable):
def _open(self, source, footer_offset=None,
IpcReadOptions options=IpcReadOptions(),
- MemoryPool memory_pool=None):
+ MemoryPool memory_pool=None, use_memory_map=False):
self.options = options.c_options
self.options.memory_pool = maybe_unbox_memory_pool(memory_pool)
try:
source = as_buffer(source)
except TypeError:
pass
- get_reader(source, True, &self.file)
+ get_reader(source, use_memory_map, &self.file)
Review Comment:
As previous cases it should support `MemoryMappedFile`
```suggestion
get_reader(source, False, &self.file)
```
##########
python/pyarrow/ipc.pxi:
##########
@@ -1075,6 +1076,8 @@ def read_schema(obj, DictionaryMemo dictionary_memo=None):
dictionary_memo : DictionaryMemo, optional
Needed to be able to reconstruct dictionary-encoded fields
with read_record_batch
+ use_memory_map : boolean, default False
+ Use memory mapping when opening file on disk
Review Comment:
```suggestion
```
##########
python/pyarrow/ipc.py:
##########
@@ -44,11 +44,16 @@ class RecordBatchStreamReader(lib._RecordBatchStreamReader):
If None, default values will be used.
memory_pool : MemoryPool, default None
If None, default memory pool is used.
+ use_memory_map : bool, default False
+ If the source is a file path, use a memory map to read file, which can
+ improve performance in some environments.
Review Comment:
Let's add to the `source` parameter documentation that you are expected to
use a `MemoryMappedFile` if you want to rely on memory mapping.
```suggestion
```
##########
python/pyarrow/ipc.py:
##########
@@ -44,11 +44,16 @@ class RecordBatchStreamReader(lib._RecordBatchStreamReader):
If None, default values will be used.
memory_pool : MemoryPool, default None
If None, default memory pool is used.
+ use_memory_map : bool, default False
+ If the source is a file path, use a memory map to read file, which can
+ improve performance in some environments.
"""
- def __init__(self, source, *, options=None, memory_pool=None):
+ def __init__(self, source, *, options=None, memory_pool=None,
+ use_memory_map=False):
options = _ensure_default_ipc_read_options(options)
- self._open(source, options=options, memory_pool=memory_pool)
+ self._open(source, options=options, memory_pool=memory_pool,
+ use_memory_map=use_memory_map)
Review Comment:
```suggestion
self._open(source, options=options, memory_pool=memory_pool)
```
##########
python/pyarrow/ipc.py:
##########
@@ -100,13 +105,16 @@ class RecordBatchFileReader(lib._RecordBatchFileReader):
If None, default values will be used.
memory_pool : MemoryPool, default None
If None, default memory pool is used.
+ use_memory_map : boolean, default False
+ Use memory mapping when opening file on disk
"""
def __init__(self, source, footer_offset=None, *, options=None,
- memory_pool=None):
+ memory_pool=None, use_memory_map=False):
options = _ensure_default_ipc_read_options(options)
self._open(source, footer_offset=footer_offset,
- options=options, memory_pool=memory_pool)
+ options=options, memory_pool=memory_pool,
+ use_memory_map=use_memory_map)
Review Comment:
```suggestion
options=options, memory_pool=memory_pool)
```
##########
python/pyarrow/serialization.pxi:
##########
@@ -425,7 +425,7 @@ def serialize_to(object value, sink, SerializationContext
context=None):
serialized.write_to(sink)
-def read_serialized(source, base=None):
+def read_serialized(source, base=None, use_memory_map=False):
Review Comment:
```suggestion
def read_serialized(source, base=None):
```
##########
python/pyarrow/serialization.pxi:
##########
@@ -442,18 +442,20 @@ def read_serialized(source, base=None):
base : object
This object will be the base object of all the numpy arrays
contained in the sequence.
+ use_memory_map : boolean, default False
+ Use memory mapping when opening file on disk
Returns
-------
serialized : the serialized data
"""
_deprecate_serialization("read_serialized")
- return _read_serialized(source, base=base)
+ return _read_serialized(source, base=base, use_memory_map=use_memory_map)
Review Comment:
```suggestion
return _read_serialized(source, base=base)
```
##########
python/pyarrow/serialization.pxi:
##########
@@ -485,14 +487,17 @@ def deserialize_from(source, object base,
SerializationContext context=None):
contained in the sequence.
context : SerializationContext
Custom serialization and deserialization context.
+ use_memory_map : boolean, default False
+ Use memory mapping when opening file on disk
Returns
-------
object
Python object for the deserialized sequence.
"""
_deprecate_serialization("deserialize_from")
- serialized = _read_serialized(source, base=base)
+ serialized = _read_serialized(
+ source, base=base, use_memory_map=use_memory_map)
Review Comment:
```suggestion
serialized = _read_serialized(source, base=base)
```
##########
python/pyarrow/serialization.pxi:
##########
@@ -550,7 +555,8 @@ def deserialize(obj, SerializationContext context=None):
return _deserialize(obj, context=context)
-def _deserialize(obj, SerializationContext context=None):
+def _deserialize(obj, SerializationContext context=None, use_memory_map=False):
Review Comment:
```suggestion
def _deserialize(obj, SerializationContext context=None):
```
##########
python/pyarrow/ipc.pxi:
##########
@@ -399,21 +399,23 @@ cdef class MessageReader(_Weakrefable):
"instead.".format(self.__class__.__name__))
@staticmethod
- def open_stream(source):
+ def open_stream(source, use_memory_map=False):
Review Comment:
```suggestion
def open_stream(source):
```
##########
python/pyarrow/ipc.pxi:
##########
@@ -562,14 +564,14 @@ cdef class _RecordBatchStreamWriter(_CRecordBatchWriter):
self.options))
-cdef _get_input_stream(object source, shared_ptr[CInputStream]* out):
+cdef _get_input_stream(object source, shared_ptr[CInputStream]* out,
use_memory_map=False):
Review Comment:
```suggestion
cdef _get_input_stream(object source, shared_ptr[CInputStream]* out):
```
##########
python/pyarrow/ipc.pxi:
##########
@@ -792,10 +793,10 @@ cdef class _RecordBatchStreamReader(RecordBatchReader):
pass
def _open(self, source, IpcReadOptions options=IpcReadOptions(),
- MemoryPool memory_pool=None):
+ MemoryPool memory_pool=None, use_memory_map=False):
self.options = options.c_options
self.options.memory_pool = maybe_unbox_memory_pool(memory_pool)
- _get_input_stream(source, &self.in_stream)
+ _get_input_stream(source, &self.in_stream, use_memory_map)
Review Comment:
`_get_input_stream` has no `use_memory_map` argument
```suggestion
_get_input_stream(source, &self.in_stream)
```
##########
python/pyarrow/ipc.py:
##########
@@ -44,11 +44,16 @@ class RecordBatchStreamReader(lib._RecordBatchStreamReader):
If None, default values will be used.
memory_pool : MemoryPool, default None
If None, default memory pool is used.
+ use_memory_map : bool, default False
+ If the source is a file path, use a memory map to read file, which can
+ improve performance in some environments.
"""
- def __init__(self, source, *, options=None, memory_pool=None):
+ def __init__(self, source, *, options=None, memory_pool=None,
+ use_memory_map=False):
Review Comment:
```suggestion
def __init__(self, source, *, options=None, memory_pool=None):
```
##########
python/pyarrow/ipc.pxi:
##########
@@ -1089,7 +1092,7 @@ def read_schema(obj, DictionaryMemo dictionary_memo=None):
if isinstance(obj, Message):
raise NotImplementedError(type(obj))
- get_reader(obj, True, &cpp_file)
+ get_reader(obj, use_memory_map, &cpp_file)
Review Comment:
```suggestion
get_reader(obj, False, &cpp_file)
```
##########
python/pyarrow/ipc.pxi:
##########
@@ -1065,7 +1066,7 @@ def read_message(source):
return result
-def read_schema(obj, DictionaryMemo dictionary_memo=None):
+def read_schema(obj, DictionaryMemo dictionary_memo=None,
use_memory_map=False):
Review Comment:
```suggestion
def read_schema(obj, DictionaryMemo dictionary_memo=None):
```
This is expected to work on `buffer` or `Message`, none of them can be
memory_mapped
##########
python/pyarrow/ipc.py:
##########
@@ -100,13 +105,16 @@ class RecordBatchFileReader(lib._RecordBatchFileReader):
If None, default values will be used.
memory_pool : MemoryPool, default None
If None, default memory pool is used.
+ use_memory_map : boolean, default False
+ Use memory mapping when opening file on disk
"""
def __init__(self, source, footer_offset=None, *, options=None,
- memory_pool=None):
+ memory_pool=None, use_memory_map=False):
Review Comment:
```suggestion
memory_pool=None):
```
##########
python/pyarrow/ipc.pxi:
##########
@@ -562,14 +564,14 @@ cdef class _RecordBatchStreamWriter(_CRecordBatchWriter):
self.options))
-cdef _get_input_stream(object source, shared_ptr[CInputStream]* out):
+cdef _get_input_stream(object source, shared_ptr[CInputStream]* out,
use_memory_map=False):
try:
source = as_buffer(source)
except TypeError:
# Non-buffer-like
pass
- get_input_stream(source, True, out)
+ get_input_stream(source, use_memory_map, out)
Review Comment:
```suggestion
get_input_stream(source, False, out)
```
##########
python/pyarrow/ipc.py:
##########
@@ -100,13 +105,16 @@ class RecordBatchFileReader(lib._RecordBatchFileReader):
If None, default values will be used.
memory_pool : MemoryPool, default None
If None, default memory pool is used.
+ use_memory_map : boolean, default False
+ Use memory mapping when opening file on disk
Review Comment:
```suggestion
```
As for previous cases, let's add to `source` documentation that it can be a
`MemoryMappedFile`
##########
python/pyarrow/ipc.pxi:
##########
@@ -399,21 +399,23 @@ cdef class MessageReader(_Weakrefable):
"instead.".format(self.__class__.__name__))
@staticmethod
- def open_stream(source):
+ def open_stream(source, use_memory_map=False):
"""
Open stream from source.
Parameters
----------
source
A readable source, like an InputStream
+ use_memory_map : boolean, default False
+ Use memory mapping when opening file on disk
Review Comment:
Let's add to the `source` docstring that you can pass `MemoryMappedFile` if
you want to use memory mapping.
##########
python/pyarrow/serialization.pxi:
##########
@@ -442,18 +442,20 @@ def read_serialized(source, base=None):
base : object
This object will be the base object of all the numpy arrays
contained in the sequence.
+ use_memory_map : boolean, default False
+ Use memory mapping when opening file on disk
Returns
-------
serialized : the serialized data
"""
_deprecate_serialization("read_serialized")
- return _read_serialized(source, base=base)
+ return _read_serialized(source, base=base, use_memory_map=use_memory_map)
-def _read_serialized(source, base=None):
+def _read_serialized(source, base=None, use_memory_map=False):
cdef shared_ptr[CRandomAccessFile] stream
- get_reader(source, True, &stream)
+ get_reader(source, use_memory_map, &stream)
Review Comment:
```suggestion
get_reader(source, False, &stream)
```
##########
python/pyarrow/serialization.pxi:
##########
@@ -442,18 +442,20 @@ def read_serialized(source, base=None):
base : object
This object will be the base object of all the numpy arrays
contained in the sequence.
+ use_memory_map : boolean, default False
+ Use memory mapping when opening file on disk
Returns
-------
serialized : the serialized data
"""
_deprecate_serialization("read_serialized")
- return _read_serialized(source, base=base)
+ return _read_serialized(source, base=base, use_memory_map=use_memory_map)
-def _read_serialized(source, base=None):
+def _read_serialized(source, base=None, use_memory_map=False):
Review Comment:
```suggestion
def _read_serialized(source, base=None):
```
##########
python/pyarrow/serialization.pxi:
##########
@@ -442,18 +442,20 @@ def read_serialized(source, base=None):
base : object
This object will be the base object of all the numpy arrays
contained in the sequence.
+ use_memory_map : boolean, default False
+ Use memory mapping when opening file on disk
Review Comment:
```suggestion
```
##########
python/pyarrow/serialization.pxi:
##########
@@ -485,14 +487,17 @@ def deserialize_from(source, object base,
SerializationContext context=None):
contained in the sequence.
context : SerializationContext
Custom serialization and deserialization context.
+ use_memory_map : boolean, default False
+ Use memory mapping when opening file on disk
Review Comment:
```suggestion
```
##########
python/pyarrow/serialization.pxi:
##########
@@ -463,7 +465,7 @@ def _read_serialized(source, base=None):
return serialized
-def deserialize_from(source, object base, SerializationContext context=None):
+def deserialize_from(source, object base, SerializationContext context=None,
use_memory_map=False):
Review Comment:
```suggestion
def deserialize_from(source, object base, SerializationContext context=None):
```
##########
python/pyarrow/ipc.pxi:
##########
@@ -399,21 +399,23 @@ cdef class MessageReader(_Weakrefable):
"instead.".format(self.__class__.__name__))
@staticmethod
- def open_stream(source):
+ def open_stream(source, use_memory_map=False):
"""
Open stream from source.
Parameters
----------
source
A readable source, like an InputStream
+ use_memory_map : boolean, default False
+ Use memory mapping when opening file on disk
Review Comment:
```suggestion
```
##########
python/pyarrow/ipc.pxi:
##########
@@ -399,21 +399,23 @@ cdef class MessageReader(_Weakrefable):
"instead.".format(self.__class__.__name__))
@staticmethod
- def open_stream(source):
+ def open_stream(source, use_memory_map=False):
"""
Open stream from source.
Parameters
----------
source
A readable source, like an InputStream
+ use_memory_map : boolean, default False
+ Use memory mapping when opening file on disk
"""
cdef:
MessageReader result = MessageReader.__new__(MessageReader)
shared_ptr[CInputStream] in_stream
unique_ptr[CMessageReader] reader
- _get_input_stream(source, &in_stream)
+ _get_input_stream(source, &in_stream, use_memory_map)
Review Comment:
```suggestion
_get_input_stream(source, &in_stream)
```
##########
python/pyarrow/serialization.pxi:
##########
@@ -550,7 +555,8 @@ def deserialize(obj, SerializationContext context=None):
return _deserialize(obj, context=context)
-def _deserialize(obj, SerializationContext context=None):
+def _deserialize(obj, SerializationContext context=None, use_memory_map=False):
source = BufferReader(obj)
- serialized = _read_serialized(source, base=obj)
+ serialized = _read_serialized(
+ source, base=obj, use_memory_map=use_memory_map)
Review Comment:
```suggestion
serialized = _read_serialized(source, base=obj)
```
--
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]
