ngoldbaum commented on code in PR #50769: URL: https://github.com/apache/arrow/pull/50769#discussion_r3705313690
########## python/pyarrow/pandas-shim.pxi: ########## Review Comment: I don't think this is needed anymore as well as all its uses below ########## python/pyarrow/pandas-shim.pxi: ########## Review Comment: I think this is dead compat code for old Pandas versions. Pre-1.0 versions, in fact, and it was removed [in 2017](https://github.com/pandas-dev/pandas/issues/15537)). So you can call `infer_dtype` without the try/except now. ########## python/pyarrow/pandas-shim.pxi: ########## Review Comment: While we're here, I think this is hard-coded as `False` and never mutated (maybe I'm missing something with my greps), so I think you could delete this attribute and the [only consumer](https://github.com/apache/arrow/blob/88c809455a97ff4681a79563ba6856923294a5b4/python/pyarrow/feather.py#L163). `SparseDataFrame` was removed in Pandas 1.0. There's also an always-skipped test for this in `test_feather.py`: https://github.com/apache/arrow/blob/88c809455a97ff4681a79563ba6856923294a5b4/python/pyarrow/tests/test_feather.py#L595-L596 ########## python/CMakeLists.txt: ########## @@ -278,6 +269,10 @@ set(EXECUTABLE_OUTPUT_PATH "${BUILD_OUTPUT_ROOT_DIRECTORY}") # Python and Numpy libraries find_package(Python3Alt REQUIRED) +if(Python3_NumPy_VERSION AND Python3_NumPy_VERSION VERSION_LESS "2.0") + message(FATAL_ERROR "PyArrow requires NumPy 2.0 or newer, found ${Python3_NumPy_VERSION}" + ) +endif() Review Comment: Maybe there should be an equivalent runtime check in PyArrow's `lib.pyx`, where the NumPy import happens? Or maybe in the spot that imports the NumPy C API: https://github.com/apache/arrow/blob/88c809455a97ff4681a79563ba6856923294a5b4/python/pyarrow/src/arrow/python/numpy_init.cc#L27-L30. If you can introspect on what numpy version is available there you might be able to make arrow generate a nicer error message than NumPy's error about ABI mismatch. ########## python/pyarrow/pandas-shim.pxi: ########## Review Comment: `is_ge_v23` is always true now so I think this bare try/except can also be deleted, please double check that though - I'm not totally sure why it's not an `except AttributeError`. Seems bad that it catches `KeyboardInterrupt` now! ########## python/pyarrow/pandas-shim.pxi: ########## Review Comment: I think this branch is unreachable as well. ########## python/pyarrow/pandas_compat.py: ########## Review Comment: `attrs` always exists on Pandas 1.0 or newer, I think ########## python/pyarrow/pandas-shim.pxi: ########## Review Comment: I think you can remove the `hasattr` and use plain attribute access now ########## python/pyarrow/pandas-shim.pxi: ########## Review Comment: I think this branch is unreachable now. ########## python/pyarrow/pandas_compat.py: ########## Review Comment: Here on the python side you can simplify this a lot by just doing plain attribute accesses and deleting `get_rangeindex_attribute` on the cython side. I doubt whatever performance optimization it was trying to do here is worth it anymore by putting it in Cython. ########## python/pyarrow/pandas_compat.py: ########## Review Comment: the if statement is always false now, I think ########## python/pyarrow/tests/test_extension_type.py: ########## Review Comment: Similarly, I think this skip can be deleted too? https://github.com/apache/arrow/blob/88c809455a97ff4681a79563ba6856923294a5b4/python/pyarrow/tests/parquet/test_pandas.py#L433-L436 -- 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]
