You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "kevinjqliu (via GitHub)" <gi...@apache.org> on 2024/10/28 22:21:46 UTC
[I] explore removing `numpy` [iceberg-python]
kevinjqliu opened a new issue, #1259:
URL: https://github.com/apache/iceberg-python/issues/1259
### Feature Request / Improvement
#1256 mentioned removing `numpy` as a dependency. `numpy` is currently only used in 1 function
https://github.com/apache/iceberg-python/blob/583a7e97db28afa4259cdf504611845222338893/pyiceberg/io/pyarrow.py#L810-L815
and 2 tests
https://github.com/apache/iceberg-python/blob/583a7e97db28afa4259cdf504611845222338893/tests/integration/test_writes/test_writes.py#L1348-L1383
https://github.com/apache/iceberg-python/blob/583a7e97db28afa4259cdf504611845222338893/tests/integration/test_writes/test_writes.py#L1404-L1412
It seems like we can replace the `_combine_positional_deletes` use of `numpy` with pure python operators or pyarrow operators
--
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: issues-unsubscribe@iceberg.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] explore removing `numpy` as a dependency [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on issue #1259:
URL: https://github.com/apache/iceberg-python/issues/1259#issuecomment-2442788004
This ChatGPT-generated function seems to be logically equivalent. And pass integration tests.
But we should double check this
```
def _combine_positional_deletes(positional_deletes: List[pa.ChunkedArray], start_index: int, end_index: int) -> pa.Array:
if len(positional_deletes) == 1:
all_chunks = positional_deletes[0]
else:
all_chunks = pa.chunked_array(itertools.chain(*[arr.chunks for arr in positional_deletes]))
# Create the full range array with pyarrow
full_range = pa.array(range(start_index, end_index))
# Filter out values in all_chunks from full_range
result = pc.filter(full_range, pc.invert(pc.is_in(full_range, value_set=all_chunks)))
# Subtract the start_index from each element in the result
adjusted_result = pc.subtract(result, pa.scalar(start_index))
return adjusted_result
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Remove `numpy` as a dependency [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on issue #1259:
URL: https://github.com/apache/iceberg-python/issues/1259#issuecomment-2444005081
@kevinjqliu That looks about right to me :) Could you do some benchmarking to see if we don't get too much of a performance regression?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Remove `numpy` as a dependency [iceberg-python]
Posted by "corleyma (via GitHub)" <gi...@apache.org>.
corleyma commented on issue #1259:
URL: https://github.com/apache/iceberg-python/issues/1259#issuecomment-2448429340
Arrow's release cycles are... not quick, but I have no doubt that logic will land there at some point.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Remove `numpy` as a dependency [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on issue #1259:
URL: https://github.com/apache/iceberg-python/issues/1259#issuecomment-2446285669
I took the liberty of checking this, so we can fix this for 0.8.0:
It looks like we're spending a lot of time on generating the sequence. I think it would be good to add this to Arrow itself: https://github.com/apache/arrow/issues/44583
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Remove `numpy` as a dependency [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko closed issue #1259: Remove `numpy` as a dependency
URL: https://github.com/apache/iceberg-python/issues/1259
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Remove `numpy` as a dependency [iceberg-python]
Posted by "corleyma (via GitHub)" <gi...@apache.org>.
corleyma commented on issue #1259:
URL: https://github.com/apache/iceberg-python/issues/1259#issuecomment-2448393527
> It looks like we're spending a lot of time on generating the sequence. I think it would be good to add this to Arrow itself: https://github.com/apache/arrow/issues/44583 I think we can move this forward since it doesn't happy very often that a file is affected by multiple positional deletes. Once it has been added to Arrow, we can also add it to PyIceberg
If we wanted, I suspect we could use the pyarrow cython/C++ API to allocate and create the range array much faster.
Something like:
```cython
# range_impl.pyx
# distutils: language=c++
from pyarrow.lib cimport *
import pyarrow as pa
import pyarrow.compute as pc
from libcpp.memory cimport shared_ptr, make_shared
from libcpp.vector cimport vector
from libc.stdint cimport int64_t, uint8_t
cdef extern from "arrow/array.h" namespace "arrow":
cdef cppclass CInt64Array "arrow::Int64Array":
CInt64Array(const shared_ptr[CDataType]& type,
int64_t length,
const shared_ptr[CBuffer]& data)
def create_arrow_range(int64_t start, int64_t end):
if start >= end:
raise ValueError("start must be less than end")
cdef:
vector[int64_t] values
int64_t i
shared_ptr[CArray] out
shared_ptr[CBuffer] buffer
shared_ptr[CDataType] dtype
# Pre-allocate the vector size
values.reserve(end - start)
# Fill vector with range values
for i in range(start, end):
values.push_back(i)
# Create buffer from vector data
buffer = make_shared[CBuffer](
<uint8_t*>values.data(),
values.size() * sizeof(int64_t)
)
# Get int64 datatype
dtype = pyarrow_unwrap_data_type(pa.int64())
# Create Int64Array
out = make_shared[CInt64Array](dtype, end - start, buffer)
# Wrap and return as Python object
return pyarrow_wrap_array(out)
```
```python
# setup.py
import os
import pyarrow as pa
from Cython.Build import cythonize
from setuptools import Extension, setup
# Ensure you have the Arrow C++ shared libraries available
try:
pa.create_library_symlinks()
except:
pass
ext_modules = cythonize(
"range_impl.pyx",
compiler_directives={
"language_level": "3",
"embedsignature": True,
},
)
for ext in ext_modules:
# We only need numpy headers at build time
ext.include_dirs.append(pa.get_include())
ext.libraries.extend(pa.get_libraries())
ext.library_dirs.extend(pa.get_library_dirs())
# Enable C++17
if os.name == "posix":
ext.extra_compile_args.append("-std=c++17")
setup(
name="arrow_range",
ext_modules=ext_modules,
setup_requires=["pyarrow"], # Only needed during build
install_requires=["pyarrow"], # Runtime dependency
)
```
and using this to benchmark:
```python
# benchmark.py
import time
import pyarrow as pa
from range_impl import create_arrow_range
def test_python_range(start, end):
return pa.array(range(start, end))
def test_cython_range(start, end):
return create_arrow_range(start, end)
def benchmark(start, end, n_runs=5):
times_python = []
times_cython = []
print(f"Testing range from {start} to {end} ({end-start} elements)")
# Warm up
_ = test_python_range(0, 1000)
_ = test_cython_range(0, 1000)
for i in range(n_runs):
# Test Python range
t0 = time.perf_counter()
arr_py = test_python_range(start, end)
t1 = time.perf_counter()
times_python.append(t1 - t0)
# Test Cython range
t0 = time.perf_counter()
arr_cy = test_cython_range(start, end)
t1 = time.perf_counter()
times_cython.append(t1 - t0)
# Verify results match
assert arr_py.equals(arr_cy), "Arrays don't match!"
avg_python = sum(times_python) / len(times_python)
avg_cython = sum(times_cython) / len(times_cython)
print(f"Python average: {avg_python:.4f}s")
print(f"Cython average: {avg_cython:.4f}s")
print(f"Speedup: {avg_python/avg_cython:.2f}x")
if __name__ == "__main__":
benchmark(0, 10_000_000)
```
Gives me this on my m1:
```sh
python setup.py build_ext --inplace
python benchmark.py
running build_ext
Testing range from 0 to 10000000 (10000000 elements)
Python average: 0.9227s
Cython average: 0.0125s
Speedup: 73.64x
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Remove `numpy` as a dependency [iceberg-python]
Posted by "geruh (via GitHub)" <gi...@apache.org>.
geruh commented on issue #1259:
URL: https://github.com/apache/iceberg-python/issues/1259#issuecomment-3060166331
They added support for the arange function in pyarrow in https://github.com/apache/arrow/pull/46778/files. I pulled the latest Arrow branch, built it locally, and integrated it into PyIceberg. The updated implementation would looks like this:
```python
def _combine_positional_deletes(positional_deletes: List[pa.ChunkedArray], start_index: int, end_index: int) -> pa.Array:
if len(positional_deletes) == 1:
all_chunks = positional_deletes[0]
else:
all_chunks = pa.chunked_array(itertools.chain(*[arr.chunks for arr in positional_deletes]))
full_range = pa.arange(start_index, end_index)
result = pc.filter(full_range, pc.invert(pc.is_in(full_range, value_set=all_chunks)))
return pc.subtract(result, pa.scalar(start_index))
```
Using the benchmark from @corleyma, I get roughly:
```
Testing range from 0 to 10000000 (10000000 elements)
Python average: 0.6944s
Cython average: 0.0096s
Speedup: 71.96x
```
Maybe in the next arrow release we can add this if an upgrade isn't to burdensome.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Remove `numpy` as a dependency [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on issue #1259:
URL: https://github.com/apache/iceberg-python/issues/1259#issuecomment-2449171063
Hey @corleyma that looks awesome, and that's some amazing speedup 🤤 I think the best place to put this would be Arrow itself.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Remove `numpy` as a dependency [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on issue #1259:
URL: https://github.com/apache/iceberg-python/issues/1259#issuecomment-2448411602
supercool! As a project, I don't want to maintain an implementation of the `range` function 😄 Is there a way to contribute `create_arrow_range` to Arrow?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org