ARROW-13091: [Python] Add compression_level argument to IpcWriteOptions constructor #10717
Conversation
westonpace
changed the title
python/pyarrow/tests/test_ipc.py
Outdated
| table = pa.Table.from_arrays([values], names=["values"]) | ||
|
|
||
| options = pa.ipc.IpcWriteOptions(compression='zstd', compression_level=1) | ||
| writer = pa.ipc.RecordBatchFileWriter(sink, table.schema, options=options) |
There was a problem hiding this comment.
Not really a major thing, but given that the RecordBatchFileWriter can act as a context manager and sometimes people look at the tests to learn how to use things, it might be a good idea to write this in the form of
with pa.ipc.RecordBatchFileWriter(sink, table.schema, options=options) as writer:
writer.write_table(table)
|
@westonpace We already have a |
westonpace
force-pushed
the
feature/ARROW-13091--python-add-compression_level-argument-to-ipcwrit
branch
from
c6a019c to
898cf7c
Compare
|
@kszucs Done. I also added some helper methods to make working with compression_level a little easier. It ended up being a bit more involved than I expected. |
…esent to generate stable random numbers
…ompression_level pairs. Added documentation and methods around compression_level
…ession codecs. Changed generic Exception to TypeError in an error message
westonpace
force-pushed
the
feature/ARROW-13091--python-add-compression_level-argument-to-ipcwrit
branch
from
d623496 to
82472b4
Compare
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
Thanks for creating the pytest marks as well.
|
I think this is ready for merge. @kszucs Any last thoughts? |
| self.c_options.codec = shared_ptr[CCodec](GetResultValue( | ||
| CCodec.Create(_ensure_compression(value))).release()) | ||
| elif isinstance(value, Codec): | ||
| self.c_options.codec = (<Codec>value).wrapped |
There was a problem hiding this comment.
Just a nit, but we could use .unwrap() here.
There was a problem hiding this comment.
There already was an unwrap() but it returned CCodec* and not shared_ptr<CCoded>. I think I'd need to investigate where the current unwrap() was used.
| @pytest.mark.pandas | ||
| @pytest.mark.lz4 | ||
| @pytest.mark.snappy | ||
| @pytest.mark.zstd |
There was a problem hiding this comment.
Could we expand this as:
pytest.mark.parametrize("codec", [
pytest.param("lz4", marks=pytest.mark.lz4),
pytest.param("snappy", marks=pytest.mark.snappy),
pytest.param("zstd", marks=pytest.mark.zstd),
])By letting pytest to iterate through the cases rather than doing it ourselves?
This would enable us to run this case just for the available plugins.
There was a problem hiding this comment.
Thanks Weston! Just a few nits related to test parametrization.
Possibly it would be better to create global compression related fixtures in a follow-up, so we can include this in the release.
Created one: https://issues.apache.org/jira/browse/ARROW-13380
|
@github-actions crossbow submit homebrew-r-autobrew |
|
Revision: 91a6ea4 Submitted crossbow builds: ursacomputing/crossbow @ actions-611
|
|
(I accidentally submitted master to Crossbow in an unrelated build and noticed a new failure that looked related to here, want to confirm that) |
|
See ARROW-13384 as followup |
|
Seems like we have more version issues in our crossbow builds: #10659 (comment) |
westonpace
deleted the
feature/ARROW-13091--python-add-compression_level-argument-to-ipcwrit
branch
4 participants
Uh oh!
There was an error while loading. Please reload this page.