GH-36730: [Python] Add support for Cython 3.0.0

[kou]

Rationale for this change

Cython 3.0.0 is the latest release. PyArrow should work with Cython 3.0.0.

What changes are included in this PR?

• Don't use vector[XXX]&&
• Add a declaration for postincrement
  • See also: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#c-postincrement-postdecrement-operator
• Ignore C4551 warning (function call missing argument list) with MSVC
  • See also: [BUG] code emitted for Python 3.10 causes warning C4551 in MSVC cython/cython#4445
• Add missing const to CLocation's static methods.
• Don't use StopIteration to stop generator
  • See also: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#python-3-syntax-semantics

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

• Closes: [CI][Python] Cython 3.0 seems to make our build of pyarrow fail #36730

[github-actions]

⚠️ GitHub issue #36730has been automatically assigned in GitHub to PR creator.

[pitrou]

This may fix the compilation error but there are other issues during tests (see macOS CI?).

[kou]

I've fixed CI failures.

[jorisvandenbossche]

@kou there were also some runtime errors in the minimal build that are caused by cython 3 (but it's strange that this is only failing in a single build). Based on your diff, I don't think that will already be fixed here, but let me run them to be sure.

[jorisvandenbossche]

@github-actions crossbow submit example-python-minimal-build-fedora-conda

[github-actions]

Revision: dba6bb4c1adcb233aabce3d595706b5fc104b660

Submitted crossbow builds: ursacomputing/crossbow @ actions-f3bbf75ac8

Task	Status
example-python-minimal-build-fedora-conda

[pitrou]

Can this PR also undo the pinning on various Python builds?

[kou]

@github-actions crossbow submit example-python-minimal-build-fedora-conda

[github-actions]

Revision: 81493b1fa6e3db21630c29ccad0e92499a2a43ff

Submitted crossbow builds: ursacomputing/crossbow @ actions-492c6d5c7d

Task	Status
example-python-minimal-build-fedora-conda

[kou]

I've fixed the generator related problem.
I'll rebase on main and unpin Cython version.

[kou]

Hmm. Can we detect Cython version in .{pyx,pxi}?

[kou]

I want to write something like the following:

diff --git a/python/pyarrow/scalar.pxi b/python/pyarrow/scalar.pxi
index f438c8847..61de38203 100644
--- a/python/pyarrow/scalar.pxi+++ b/python/pyarrow/scalar.pxi@@ -793,7 +793,10 @@ cdef class MapScalar(ListScalar):
"""
arr = self.values
if array is None:
- raise StopIteration+ if Cython.__version__ < '3.0.0':+ raise StopIteration+ else:+ return
for k, v in zip(arr.field('key'), arr.field('value')):
yield (k.as_py(), v.as_py())

[pitrou]

There should not be any need to raise StopIteration.

[jorisvandenbossche]

I don't know if it is this code that gives problems to be written in a way that works for both cython 0.29 and 3, but a potential different way of writing it:

def __iter__(self):
return self
def __next__(self):
return self.read_next_message()

[kou]

Thanks! I'll try it.

[pitrou]

Do you actually test these changes before pushing them?

You can't use yield in a __next__ function.
Either you write __iter__ as a generator (using yield), or you write a pair of __iter__ and __next__ functions (without yield). You can't mix the two styles.

[jorisvandenbossche]

And FWIW, for this case, I would leave the __iter__ as is (in the other cases such as RecordBatchReader, we already have a public "next" method that can just be called from __next__)

[kou]

Sorry. I didn't test on local before I pushed a commit because I don't have enough time yesterday.

I reverted the __iter__ + __next__ change.

[lidavidm]

Flight changes LGTM, thanks!

[kou]

@jorisvandenbossche Do you have any idea why the following failure was happen?

https://github.com/apache/arrow/actions/runs/5615711782/job/15216554355?pr=36745#step:6:6074

 =================================== FAILURES ===================================
_______________________ test_cloudpickle_dataset[False] ________________________
tempdir = PosixPath('/tmp/pytest-of-root/pytest-0/test_cloudpickle_dataset_False0')
datadir = PosixPath('/opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/data/parquet')
use_legacy_dataset = False
@pytest.mark.pandas
@parametrize_legacy_dataset
def test_cloudpickle_dataset(tempdir, datadir, use_legacy_dataset):
cp = pytest.importorskip('cloudpickle')
dataset = _make_dataset_for_pickling(tempdir, use_legacy_dataset)
> _assert_dataset_is_picklable(
dataset, pickler=cp, use_legacy_dataset=use_legacy_dataset)
opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1572: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1540: in _assert_dataset_is_picklable
assert is_pickleable(dataset)
opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1538: in is_pickleable
return obj == pickler.loads(pickler.dumps(obj))
opt/conda/envs/arrow/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py:73: in dumps
cp.dump(obj)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <cloudpickle.cloudpickle_fast.CloudPickler object at 0x7f25aa27aa10>
obj = <pyarrow.parquet.core._ParquetDatasetV2 object at 0x7f25a9ec9690>
def dump(self, obj):
try:
> return Pickler.dump(self, obj)
E _pickle.PicklingError: Can't pickle <cyfunction ParquetFragmentScanOptions._reconstruct at 0x7f2660177140>: it's not the same object as pyarrow._dataset_parquet.ParquetFragmentScanOptions._reconstruct
opt/conda/envs/arrow/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py:632: PicklingError

[jorisvandenbossche]

Not directly an idea. We use the same pattern in many other places. Can you try this patch:

--- a/python/pyarrow/_dataset_parquet.pyx+++ b/python/pyarrow/_dataset_parquet.pyx@@ -741,7 +741,7 @@ cdef class ParquetFragmentScanOptions(FragmentScanOptions):
thrift_string_size_limit=self.thrift_string_size_limit,
thrift_container_size_limit=self.thrift_container_size_limit,
)
- return type(self)._reconstruct, (kwargs,)+ return ParquetFragmentScanOptions._reconstruct, (kwargs,)

(in theory it shouldn't matter for python, but maybe cython 3 has a bug that treats this different)

[kou]

Thanks! I've pushed your patch.

[h-vetinari]

If there are problems with compatibility between 0.29 and 3.0 (aside from the few intentional breaking changes), the Cython devs are usually quick to respond if you open an issue

[danepitkin]

The failing pickle test is only when using the cloudpickle module instead of the pickle5/pickle module. Perhaps they don't support cython3.0.0 yet? This is the only test in the entire codebase that tests cloudpickle.. not sure how important it is as a dependency to pyarrow.

@pytest.mark.pandas
@parametrize_legacy_dataset
def test_cloudpickle_dataset(tempdir, datadir, use_legacy_dataset):
cp = pytest.importorskip('cloudpickle')
dataset = _make_dataset_for_pickling(tempdir, use_legacy_dataset)
_assert_dataset_is_picklable(
dataset, pickler=cp, use_legacy_dataset=use_legacy_dataset)

[pitrou]

Well, first, this PR should be rebased, because there are conflicts.

[danepitkin]

I did rebase/squash to test things out here: #37097

I wanted to experiment without polluting this PR. I can merge any fixes I find back into this PR.

[kou]

Can we use #37097 instead of this?

[danepitkin]

Can we use #37097 instead of this?

Sure!

[kou]

I close this in favor of #37097.

[kou]

Rationale for this change

Cython 3.0.0 is the latest release. PyArrow should work with Cython 3.0.0.

What changes are included in this PR?

• Don't use vector[XXX]&&
• Add a declaration for postincrement
  • See also: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#c-postincrement-postdecrement-operator
• Ignore C4551 warning (function call missing argument list) with MSVC
  • See also: [BUG] code emitted for Python 3.10 causes warning C4551 in MSVC cython/cython#4445
• Add missing const to CLocation's static methods.
• Don't use StopIteration to stop generator
  • See also: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#python-3-syntax-semantics

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

• Closes: [CI][Python] Cython 3.0 seems to make our build of pyarrow fail #36730

[github-actions]

⚠️ GitHub issue #36730has been automatically assigned in GitHub to PR creator.

[pitrou]

This may fix the compilation error but there are other issues during tests (see macOS CI?).

[kou]

I've fixed CI failures.

[jorisvandenbossche]

@kou there were also some runtime errors in the minimal build that are caused by cython 3 (but it's strange that this is only failing in a single build). Based on your diff, I don't think that will already be fixed here, but let me run them to be sure.

[jorisvandenbossche]

@github-actions crossbow submit example-python-minimal-build-fedora-conda

[github-actions]

Revision: dba6bb4c1adcb233aabce3d595706b5fc104b660

Submitted crossbow builds: ursacomputing/crossbow @ actions-f3bbf75ac8

Task	Status
example-python-minimal-build-fedora-conda

[pitrou]

Can this PR also undo the pinning on various Python builds?

[kou]

@github-actions crossbow submit example-python-minimal-build-fedora-conda

[github-actions]

Revision: 81493b1fa6e3db21630c29ccad0e92499a2a43ff

Submitted crossbow builds: ursacomputing/crossbow @ actions-492c6d5c7d

Task	Status
example-python-minimal-build-fedora-conda

[kou]

I've fixed the generator related problem.
I'll rebase on main and unpin Cython version.

[kou]

Hmm. Can we detect Cython version in .{pyx,pxi}?

[kou]

I want to write something like the following:

diff --git a/python/pyarrow/scalar.pxi b/python/pyarrow/scalar.pxi
index f438c8847..61de38203 100644
--- a/python/pyarrow/scalar.pxi+++ b/python/pyarrow/scalar.pxi@@ -793,7 +793,10 @@ cdef class MapScalar(ListScalar):
"""
arr = self.values
if array is None:
- raise StopIteration+ if Cython.__version__ < '3.0.0':+ raise StopIteration+ else:+ return
for k, v in zip(arr.field('key'), arr.field('value')):
yield (k.as_py(), v.as_py())

[pitrou]

There should not be any need to raise StopIteration.

[jorisvandenbossche]

I don't know if it is this code that gives problems to be written in a way that works for both cython 0.29 and 3, but a potential different way of writing it:

def __iter__(self):
return self
def __next__(self):
return self.read_next_message()

[kou]

Thanks! I'll try it.

[pitrou]

Do you actually test these changes before pushing them?

You can't use yield in a __next__ function.
Either you write __iter__ as a generator (using yield), or you write a pair of __iter__ and __next__ functions (without yield). You can't mix the two styles.

[jorisvandenbossche]

And FWIW, for this case, I would leave the __iter__ as is (in the other cases such as RecordBatchReader, we already have a public "next" method that can just be called from __next__)

[kou]

Sorry. I didn't test on local before I pushed a commit because I don't have enough time yesterday.

I reverted the __iter__ + __next__ change.

[lidavidm]

Flight changes LGTM, thanks!

[kou]

@jorisvandenbossche Do you have any idea why the following failure was happen?

https://github.com/apache/arrow/actions/runs/5615711782/job/15216554355?pr=36745#step:6:6074

 =================================== FAILURES ===================================
_______________________ test_cloudpickle_dataset[False] ________________________
tempdir = PosixPath('/tmp/pytest-of-root/pytest-0/test_cloudpickle_dataset_False0')
datadir = PosixPath('/opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/data/parquet')
use_legacy_dataset = False
@pytest.mark.pandas
@parametrize_legacy_dataset
def test_cloudpickle_dataset(tempdir, datadir, use_legacy_dataset):
cp = pytest.importorskip('cloudpickle')
dataset = _make_dataset_for_pickling(tempdir, use_legacy_dataset)
> _assert_dataset_is_picklable(
dataset, pickler=cp, use_legacy_dataset=use_legacy_dataset)
opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1572: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1540: in _assert_dataset_is_picklable
assert is_pickleable(dataset)
opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1538: in is_pickleable
return obj == pickler.loads(pickler.dumps(obj))
opt/conda/envs/arrow/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py:73: in dumps
cp.dump(obj)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <cloudpickle.cloudpickle_fast.CloudPickler object at 0x7f25aa27aa10>
obj = <pyarrow.parquet.core._ParquetDatasetV2 object at 0x7f25a9ec9690>
def dump(self, obj):
try:
> return Pickler.dump(self, obj)
E _pickle.PicklingError: Can't pickle <cyfunction ParquetFragmentScanOptions._reconstruct at 0x7f2660177140>: it's not the same object as pyarrow._dataset_parquet.ParquetFragmentScanOptions._reconstruct
opt/conda/envs/arrow/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py:632: PicklingError

[jorisvandenbossche]

Not directly an idea. We use the same pattern in many other places. Can you try this patch:

--- a/python/pyarrow/_dataset_parquet.pyx+++ b/python/pyarrow/_dataset_parquet.pyx@@ -741,7 +741,7 @@ cdef class ParquetFragmentScanOptions(FragmentScanOptions):
thrift_string_size_limit=self.thrift_string_size_limit,
thrift_container_size_limit=self.thrift_container_size_limit,
)
- return type(self)._reconstruct, (kwargs,)+ return ParquetFragmentScanOptions._reconstruct, (kwargs,)

(in theory it shouldn't matter for python, but maybe cython 3 has a bug that treats this different)

[kou]

Thanks! I've pushed your patch.

[h-vetinari]

If there are problems with compatibility between 0.29 and 3.0 (aside from the few intentional breaking changes), the Cython devs are usually quick to respond if you open an issue

[danepitkin]

The failing pickle test is only when using the cloudpickle module instead of the pickle5/pickle module. Perhaps they don't support cython3.0.0 yet? This is the only test in the entire codebase that tests cloudpickle.. not sure how important it is as a dependency to pyarrow.

@pytest.mark.pandas
@parametrize_legacy_dataset
def test_cloudpickle_dataset(tempdir, datadir, use_legacy_dataset):
cp = pytest.importorskip('cloudpickle')
dataset = _make_dataset_for_pickling(tempdir, use_legacy_dataset)
_assert_dataset_is_picklable(
dataset, pickler=cp, use_legacy_dataset=use_legacy_dataset)

[pitrou]

Well, first, this PR should be rebased, because there are conflicts.

[danepitkin]

I did rebase/squash to test things out here: #37097

I wanted to experiment without polluting this PR. I can merge any fixes I find back into this PR.

[kou]

Can we use #37097 instead of this?

[danepitkin]

Can we use #37097 instead of this?

Sure!

[kou]

I close this in favor of #37097.

[kou]

Rationale for this change

Cython 3.0.0 is the latest release. PyArrow should work with Cython 3.0.0.

What changes are included in this PR?

• Don't use vector[XXX]&&
• Add a declaration for postincrement
  • See also: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#c-postincrement-postdecrement-operator
• Ignore C4551 warning (function call missing argument list) with MSVC
  • See also: [BUG] code emitted for Python 3.10 causes warning C4551 in MSVC cython/cython#4445
• Add missing const to CLocation's static methods.
• Don't use StopIteration to stop generator
  • See also: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#python-3-syntax-semantics

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

• Closes: [CI][Python] Cython 3.0 seems to make our build of pyarrow fail #36730

[github-actions]

⚠️ GitHub issue #36730has been automatically assigned in GitHub to PR creator.

[pitrou]

This may fix the compilation error but there are other issues during tests (see macOS CI?).

[kou]

I've fixed CI failures.

[jorisvandenbossche]

@kou there were also some runtime errors in the minimal build that are caused by cython 3 (but it's strange that this is only failing in a single build). Based on your diff, I don't think that will already be fixed here, but let me run them to be sure.

[jorisvandenbossche]

@github-actions crossbow submit example-python-minimal-build-fedora-conda

[github-actions]

Revision: dba6bb4c1adcb233aabce3d595706b5fc104b660

Submitted crossbow builds: ursacomputing/crossbow @ actions-f3bbf75ac8

Task	Status
example-python-minimal-build-fedora-conda

[pitrou]

Can this PR also undo the pinning on various Python builds?

[kou]

@github-actions crossbow submit example-python-minimal-build-fedora-conda

[github-actions]

Revision: 81493b1fa6e3db21630c29ccad0e92499a2a43ff

Submitted crossbow builds: ursacomputing/crossbow @ actions-492c6d5c7d

Task	Status
example-python-minimal-build-fedora-conda

[kou]

I've fixed the generator related problem.
I'll rebase on main and unpin Cython version.

[kou]

Hmm. Can we detect Cython version in .{pyx,pxi}?

[kou]

I want to write something like the following:

diff --git a/python/pyarrow/scalar.pxi b/python/pyarrow/scalar.pxi
index f438c8847..61de38203 100644
--- a/python/pyarrow/scalar.pxi+++ b/python/pyarrow/scalar.pxi@@ -793,7 +793,10 @@ cdef class MapScalar(ListScalar):
"""
arr = self.values
if array is None:
- raise StopIteration+ if Cython.__version__ < '3.0.0':+ raise StopIteration+ else:+ return
for k, v in zip(arr.field('key'), arr.field('value')):
yield (k.as_py(), v.as_py())

[pitrou]

There should not be any need to raise StopIteration.

[jorisvandenbossche]

I don't know if it is this code that gives problems to be written in a way that works for both cython 0.29 and 3, but a potential different way of writing it:

def __iter__(self):
return self
def __next__(self):
return self.read_next_message()

[kou]

Thanks! I'll try it.

[pitrou]

Do you actually test these changes before pushing them?

You can't use yield in a __next__ function.
Either you write __iter__ as a generator (using yield), or you write a pair of __iter__ and __next__ functions (without yield). You can't mix the two styles.

[jorisvandenbossche]

And FWIW, for this case, I would leave the __iter__ as is (in the other cases such as RecordBatchReader, we already have a public "next" method that can just be called from __next__)

[kou]

Sorry. I didn't test on local before I pushed a commit because I don't have enough time yesterday.

I reverted the __iter__ + __next__ change.

[lidavidm]

Flight changes LGTM, thanks!

[kou]

@jorisvandenbossche Do you have any idea why the following failure was happen?

https://github.com/apache/arrow/actions/runs/5615711782/job/15216554355?pr=36745#step:6:6074

 =================================== FAILURES ===================================
_______________________ test_cloudpickle_dataset[False] ________________________
tempdir = PosixPath('/tmp/pytest-of-root/pytest-0/test_cloudpickle_dataset_False0')
datadir = PosixPath('/opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/data/parquet')
use_legacy_dataset = False
@pytest.mark.pandas
@parametrize_legacy_dataset
def test_cloudpickle_dataset(tempdir, datadir, use_legacy_dataset):
cp = pytest.importorskip('cloudpickle')
dataset = _make_dataset_for_pickling(tempdir, use_legacy_dataset)
> _assert_dataset_is_picklable(
dataset, pickler=cp, use_legacy_dataset=use_legacy_dataset)
opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1572: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1540: in _assert_dataset_is_picklable
assert is_pickleable(dataset)
opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1538: in is_pickleable
return obj == pickler.loads(pickler.dumps(obj))
opt/conda/envs/arrow/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py:73: in dumps
cp.dump(obj)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <cloudpickle.cloudpickle_fast.CloudPickler object at 0x7f25aa27aa10>
obj = <pyarrow.parquet.core._ParquetDatasetV2 object at 0x7f25a9ec9690>
def dump(self, obj):
try:
> return Pickler.dump(self, obj)
E _pickle.PicklingError: Can't pickle <cyfunction ParquetFragmentScanOptions._reconstruct at 0x7f2660177140>: it's not the same object as pyarrow._dataset_parquet.ParquetFragmentScanOptions._reconstruct
opt/conda/envs/arrow/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py:632: PicklingError

[jorisvandenbossche]

Not directly an idea. We use the same pattern in many other places. Can you try this patch:

--- a/python/pyarrow/_dataset_parquet.pyx+++ b/python/pyarrow/_dataset_parquet.pyx@@ -741,7 +741,7 @@ cdef class ParquetFragmentScanOptions(FragmentScanOptions):
thrift_string_size_limit=self.thrift_string_size_limit,
thrift_container_size_limit=self.thrift_container_size_limit,
)
- return type(self)._reconstruct, (kwargs,)+ return ParquetFragmentScanOptions._reconstruct, (kwargs,)

(in theory it shouldn't matter for python, but maybe cython 3 has a bug that treats this different)

[kou]

Thanks! I've pushed your patch.

[h-vetinari]

If there are problems with compatibility between 0.29 and 3.0 (aside from the few intentional breaking changes), the Cython devs are usually quick to respond if you open an issue

[danepitkin]

The failing pickle test is only when using the cloudpickle module instead of the pickle5/pickle module. Perhaps they don't support cython3.0.0 yet? This is the only test in the entire codebase that tests cloudpickle.. not sure how important it is as a dependency to pyarrow.

@pytest.mark.pandas
@parametrize_legacy_dataset
def test_cloudpickle_dataset(tempdir, datadir, use_legacy_dataset):
cp = pytest.importorskip('cloudpickle')
dataset = _make_dataset_for_pickling(tempdir, use_legacy_dataset)
_assert_dataset_is_picklable(
dataset, pickler=cp, use_legacy_dataset=use_legacy_dataset)

[pitrou]

Well, first, this PR should be rebased, because there are conflicts.

[danepitkin]

I did rebase/squash to test things out here: #37097

I wanted to experiment without polluting this PR. I can merge any fixes I find back into this PR.

[kou]

Can we use #37097 instead of this?

[danepitkin]

Can we use #37097 instead of this?

Sure!

[kou]

I close this in favor of #37097.

[kou]

Rationale for this change

Cython 3.0.0 is the latest release. PyArrow should work with Cython 3.0.0.

What changes are included in this PR?

• Don't use vector[XXX]&&
• Add a declaration for postincrement
  • See also: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#c-postincrement-postdecrement-operator
• Ignore C4551 warning (function call missing argument list) with MSVC
  • See also: [BUG] code emitted for Python 3.10 causes warning C4551 in MSVC cython/cython#4445
• Add missing const to CLocation's static methods.
• Don't use StopIteration to stop generator
  • See also: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#python-3-syntax-semantics

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

• Closes: [CI][Python] Cython 3.0 seems to make our build of pyarrow fail #36730

[github-actions]

⚠️ GitHub issue #36730has been automatically assigned in GitHub to PR creator.

[pitrou]

This may fix the compilation error but there are other issues during tests (see macOS CI?).

[kou]

I've fixed CI failures.

[jorisvandenbossche]

@kou there were also some runtime errors in the minimal build that are caused by cython 3 (but it's strange that this is only failing in a single build). Based on your diff, I don't think that will already be fixed here, but let me run them to be sure.

[jorisvandenbossche]

@github-actions crossbow submit example-python-minimal-build-fedora-conda

[github-actions]

Revision: dba6bb4c1adcb233aabce3d595706b5fc104b660

Submitted crossbow builds: ursacomputing/crossbow @ actions-f3bbf75ac8

Task	Status
example-python-minimal-build-fedora-conda

[pitrou]

Can this PR also undo the pinning on various Python builds?

[kou]

@github-actions crossbow submit example-python-minimal-build-fedora-conda

[github-actions]

Revision: 81493b1fa6e3db21630c29ccad0e92499a2a43ff

Submitted crossbow builds: ursacomputing/crossbow @ actions-492c6d5c7d

Task	Status
example-python-minimal-build-fedora-conda

[kou]

I've fixed the generator related problem.
I'll rebase on main and unpin Cython version.

[kou]

Hmm. Can we detect Cython version in .{pyx,pxi}?

[kou]

I want to write something like the following:

diff --git a/python/pyarrow/scalar.pxi b/python/pyarrow/scalar.pxi
index f438c8847..61de38203 100644
--- a/python/pyarrow/scalar.pxi+++ b/python/pyarrow/scalar.pxi@@ -793,7 +793,10 @@ cdef class MapScalar(ListScalar):
"""
arr = self.values
if array is None:
- raise StopIteration+ if Cython.__version__ < '3.0.0':+ raise StopIteration+ else:+ return
for k, v in zip(arr.field('key'), arr.field('value')):
yield (k.as_py(), v.as_py())

[pitrou]

There should not be any need to raise StopIteration.

[jorisvandenbossche]

I don't know if it is this code that gives problems to be written in a way that works for both cython 0.29 and 3, but a potential different way of writing it:

def __iter__(self):
return self
def __next__(self):
return self.read_next_message()

[kou]

Thanks! I'll try it.

[pitrou]

Do you actually test these changes before pushing them?

You can't use yield in a __next__ function.
Either you write __iter__ as a generator (using yield), or you write a pair of __iter__ and __next__ functions (without yield). You can't mix the two styles.

[jorisvandenbossche]

And FWIW, for this case, I would leave the __iter__ as is (in the other cases such as RecordBatchReader, we already have a public "next" method that can just be called from __next__)

[kou]

Sorry. I didn't test on local before I pushed a commit because I don't have enough time yesterday.

I reverted the __iter__ + __next__ change.

[lidavidm]

Flight changes LGTM, thanks!

[kou]

@jorisvandenbossche Do you have any idea why the following failure was happen?

https://github.com/apache/arrow/actions/runs/5615711782/job/15216554355?pr=36745#step:6:6074

 =================================== FAILURES ===================================
_______________________ test_cloudpickle_dataset[False] ________________________
tempdir = PosixPath('/tmp/pytest-of-root/pytest-0/test_cloudpickle_dataset_False0')
datadir = PosixPath('/opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/data/parquet')
use_legacy_dataset = False
@pytest.mark.pandas
@parametrize_legacy_dataset
def test_cloudpickle_dataset(tempdir, datadir, use_legacy_dataset):
cp = pytest.importorskip('cloudpickle')
dataset = _make_dataset_for_pickling(tempdir, use_legacy_dataset)
> _assert_dataset_is_picklable(
dataset, pickler=cp, use_legacy_dataset=use_legacy_dataset)
opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1572: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1540: in _assert_dataset_is_picklable
assert is_pickleable(dataset)
opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1538: in is_pickleable
return obj == pickler.loads(pickler.dumps(obj))
opt/conda/envs/arrow/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py:73: in dumps
cp.dump(obj)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <cloudpickle.cloudpickle_fast.CloudPickler object at 0x7f25aa27aa10>
obj = <pyarrow.parquet.core._ParquetDatasetV2 object at 0x7f25a9ec9690>
def dump(self, obj):
try:
> return Pickler.dump(self, obj)
E _pickle.PicklingError: Can't pickle <cyfunction ParquetFragmentScanOptions._reconstruct at 0x7f2660177140>: it's not the same object as pyarrow._dataset_parquet.ParquetFragmentScanOptions._reconstruct
opt/conda/envs/arrow/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py:632: PicklingError

[jorisvandenbossche]

Not directly an idea. We use the same pattern in many other places. Can you try this patch:

--- a/python/pyarrow/_dataset_parquet.pyx+++ b/python/pyarrow/_dataset_parquet.pyx@@ -741,7 +741,7 @@ cdef class ParquetFragmentScanOptions(FragmentScanOptions):
thrift_string_size_limit=self.thrift_string_size_limit,
thrift_container_size_limit=self.thrift_container_size_limit,
)
- return type(self)._reconstruct, (kwargs,)+ return ParquetFragmentScanOptions._reconstruct, (kwargs,)

(in theory it shouldn't matter for python, but maybe cython 3 has a bug that treats this different)

[kou]

Thanks! I've pushed your patch.

[h-vetinari]

If there are problems with compatibility between 0.29 and 3.0 (aside from the few intentional breaking changes), the Cython devs are usually quick to respond if you open an issue

[danepitkin]

The failing pickle test is only when using the cloudpickle module instead of the pickle5/pickle module. Perhaps they don't support cython3.0.0 yet? This is the only test in the entire codebase that tests cloudpickle.. not sure how important it is as a dependency to pyarrow.

@pytest.mark.pandas
@parametrize_legacy_dataset
def test_cloudpickle_dataset(tempdir, datadir, use_legacy_dataset):
cp = pytest.importorskip('cloudpickle')
dataset = _make_dataset_for_pickling(tempdir, use_legacy_dataset)
_assert_dataset_is_picklable(
dataset, pickler=cp, use_legacy_dataset=use_legacy_dataset)

[pitrou]

Well, first, this PR should be rebased, because there are conflicts.

[danepitkin]

I did rebase/squash to test things out here: #37097

I wanted to experiment without polluting this PR. I can merge any fixes I find back into this PR.

[kou]

Can we use #37097 instead of this?

[danepitkin]

Can we use #37097 instead of this?

Sure!

[kou]

I close this in favor of #37097.

[kou]

Rationale for this change

Cython 3.0.0 is the latest release. PyArrow should work with Cython 3.0.0.

What changes are included in this PR?

• Don't use vector[XXX]&&
• Add a declaration for postincrement
  • See also: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#c-postincrement-postdecrement-operator
• Ignore C4551 warning (function call missing argument list) with MSVC
  • See also: [BUG] code emitted for Python 3.10 causes warning C4551 in MSVC cython/cython#4445
• Add missing const to CLocation's static methods.
• Don't use StopIteration to stop generator
  • See also: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#python-3-syntax-semantics

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

• Closes: [CI][Python] Cython 3.0 seems to make our build of pyarrow fail #36730

[github-actions]

⚠️ GitHub issue #36730has been automatically assigned in GitHub to PR creator.

[pitrou]

This may fix the compilation error but there are other issues during tests (see macOS CI?).

[kou]

I've fixed CI failures.

[jorisvandenbossche]

@kou there were also some runtime errors in the minimal build that are caused by cython 3 (but it's strange that this is only failing in a single build). Based on your diff, I don't think that will already be fixed here, but let me run them to be sure.

[jorisvandenbossche]

@github-actions crossbow submit example-python-minimal-build-fedora-conda

[github-actions]

Revision: dba6bb4c1adcb233aabce3d595706b5fc104b660

Submitted crossbow builds: ursacomputing/crossbow @ actions-f3bbf75ac8

Task	Status
example-python-minimal-build-fedora-conda

[pitrou]

Can this PR also undo the pinning on various Python builds?

[kou]

@github-actions crossbow submit example-python-minimal-build-fedora-conda

[github-actions]

Revision: 81493b1fa6e3db21630c29ccad0e92499a2a43ff

Submitted crossbow builds: ursacomputing/crossbow @ actions-492c6d5c7d

Task	Status
example-python-minimal-build-fedora-conda

[kou]

I've fixed the generator related problem.
I'll rebase on main and unpin Cython version.

[kou]

Hmm. Can we detect Cython version in .{pyx,pxi}?

[kou]

I want to write something like the following:

diff --git a/python/pyarrow/scalar.pxi b/python/pyarrow/scalar.pxi
index f438c8847..61de38203 100644
--- a/python/pyarrow/scalar.pxi+++ b/python/pyarrow/scalar.pxi@@ -793,7 +793,10 @@ cdef class MapScalar(ListScalar):
"""
arr = self.values
if array is None:
- raise StopIteration+ if Cython.__version__ < '3.0.0':+ raise StopIteration+ else:+ return
for k, v in zip(arr.field('key'), arr.field('value')):
yield (k.as_py(), v.as_py())

[pitrou]

There should not be any need to raise StopIteration.

[jorisvandenbossche]

I don't know if it is this code that gives problems to be written in a way that works for both cython 0.29 and 3, but a potential different way of writing it:

def __iter__(self):
return self
def __next__(self):
return self.read_next_message()

[kou]

Thanks! I'll try it.

[pitrou]

Do you actually test these changes before pushing them?

You can't use yield in a __next__ function.
Either you write __iter__ as a generator (using yield), or you write a pair of __iter__ and __next__ functions (without yield). You can't mix the two styles.

[jorisvandenbossche]

And FWIW, for this case, I would leave the __iter__ as is (in the other cases such as RecordBatchReader, we already have a public "next" method that can just be called from __next__)

[kou]

Sorry. I didn't test on local before I pushed a commit because I don't have enough time yesterday.

I reverted the __iter__ + __next__ change.

[lidavidm]

Flight changes LGTM, thanks!

[kou]

@jorisvandenbossche Do you have any idea why the following failure was happen?

https://github.com/apache/arrow/actions/runs/5615711782/job/15216554355?pr=36745#step:6:6074

 =================================== FAILURES ===================================
_______________________ test_cloudpickle_dataset[False] ________________________
tempdir = PosixPath('/tmp/pytest-of-root/pytest-0/test_cloudpickle_dataset_False0')
datadir = PosixPath('/opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/data/parquet')
use_legacy_dataset = False
@pytest.mark.pandas
@parametrize_legacy_dataset
def test_cloudpickle_dataset(tempdir, datadir, use_legacy_dataset):
cp = pytest.importorskip('cloudpickle')
dataset = _make_dataset_for_pickling(tempdir, use_legacy_dataset)
> _assert_dataset_is_picklable(
dataset, pickler=cp, use_legacy_dataset=use_legacy_dataset)
opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1572: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1540: in _assert_dataset_is_picklable
assert is_pickleable(dataset)
opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1538: in is_pickleable
return obj == pickler.loads(pickler.dumps(obj))
opt/conda/envs/arrow/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py:73: in dumps
cp.dump(obj)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <cloudpickle.cloudpickle_fast.CloudPickler object at 0x7f25aa27aa10>
obj = <pyarrow.parquet.core._ParquetDatasetV2 object at 0x7f25a9ec9690>
def dump(self, obj):
try:
> return Pickler.dump(self, obj)
E _pickle.PicklingError: Can't pickle <cyfunction ParquetFragmentScanOptions._reconstruct at 0x7f2660177140>: it's not the same object as pyarrow._dataset_parquet.ParquetFragmentScanOptions._reconstruct
opt/conda/envs/arrow/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py:632: PicklingError

[jorisvandenbossche]

Not directly an idea. We use the same pattern in many other places. Can you try this patch:

--- a/python/pyarrow/_dataset_parquet.pyx+++ b/python/pyarrow/_dataset_parquet.pyx@@ -741,7 +741,7 @@ cdef class ParquetFragmentScanOptions(FragmentScanOptions):
thrift_string_size_limit=self.thrift_string_size_limit,
thrift_container_size_limit=self.thrift_container_size_limit,
)
- return type(self)._reconstruct, (kwargs,)+ return ParquetFragmentScanOptions._reconstruct, (kwargs,)

(in theory it shouldn't matter for python, but maybe cython 3 has a bug that treats this different)

[kou]

Thanks! I've pushed your patch.

[h-vetinari]

If there are problems with compatibility between 0.29 and 3.0 (aside from the few intentional breaking changes), the Cython devs are usually quick to respond if you open an issue

[danepitkin]

The failing pickle test is only when using the cloudpickle module instead of the pickle5/pickle module. Perhaps they don't support cython3.0.0 yet? This is the only test in the entire codebase that tests cloudpickle.. not sure how important it is as a dependency to pyarrow.

@pytest.mark.pandas
@parametrize_legacy_dataset
def test_cloudpickle_dataset(tempdir, datadir, use_legacy_dataset):
cp = pytest.importorskip('cloudpickle')
dataset = _make_dataset_for_pickling(tempdir, use_legacy_dataset)
_assert_dataset_is_picklable(
dataset, pickler=cp, use_legacy_dataset=use_legacy_dataset)

[pitrou]

Well, first, this PR should be rebased, because there are conflicts.

[danepitkin]

I did rebase/squash to test things out here: #37097

I wanted to experiment without polluting this PR. I can merge any fixes I find back into this PR.

[kou]

Can we use #37097 instead of this?

[danepitkin]

Can we use #37097 instead of this?

Sure!

[kou]

I close this in favor of #37097.

[kou]

Rationale for this change

Cython 3.0.0 is the latest release. PyArrow should work with Cython 3.0.0.

What changes are included in this PR?

• Don't use vector[XXX]&&
• Add a declaration for postincrement
  • See also: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#c-postincrement-postdecrement-operator
• Ignore C4551 warning (function call missing argument list) with MSVC
  • See also: [BUG] code emitted for Python 3.10 causes warning C4551 in MSVC cython/cython#4445
• Add missing const to CLocation's static methods.
• Don't use StopIteration to stop generator
  • See also: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#python-3-syntax-semantics

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

• Closes: [CI][Python] Cython 3.0 seems to make our build of pyarrow fail #36730

[github-actions]

⚠️ GitHub issue #36730has been automatically assigned in GitHub to PR creator.

[pitrou]

This may fix the compilation error but there are other issues during tests (see macOS CI?).

[kou]

I've fixed CI failures.

[jorisvandenbossche]

@kou there were also some runtime errors in the minimal build that are caused by cython 3 (but it's strange that this is only failing in a single build). Based on your diff, I don't think that will already be fixed here, but let me run them to be sure.

[jorisvandenbossche]

@github-actions crossbow submit example-python-minimal-build-fedora-conda

[github-actions]

Revision: dba6bb4c1adcb233aabce3d595706b5fc104b660

Submitted crossbow builds: ursacomputing/crossbow @ actions-f3bbf75ac8

Task	Status
example-python-minimal-build-fedora-conda

[pitrou]

Can this PR also undo the pinning on various Python builds?

[kou]

@github-actions crossbow submit example-python-minimal-build-fedora-conda

[github-actions]

Revision: 81493b1fa6e3db21630c29ccad0e92499a2a43ff

Submitted crossbow builds: ursacomputing/crossbow @ actions-492c6d5c7d

Task	Status
example-python-minimal-build-fedora-conda

[kou]

I've fixed the generator related problem.
I'll rebase on main and unpin Cython version.

[kou]

Hmm. Can we detect Cython version in .{pyx,pxi}?

[kou]

I want to write something like the following:

diff --git a/python/pyarrow/scalar.pxi b/python/pyarrow/scalar.pxi
index f438c8847..61de38203 100644
--- a/python/pyarrow/scalar.pxi+++ b/python/pyarrow/scalar.pxi@@ -793,7 +793,10 @@ cdef class MapScalar(ListScalar):
"""
arr = self.values
if array is None:
- raise StopIteration+ if Cython.__version__ < '3.0.0':+ raise StopIteration+ else:+ return
for k, v in zip(arr.field('key'), arr.field('value')):
yield (k.as_py(), v.as_py())

[pitrou]

There should not be any need to raise StopIteration.

[jorisvandenbossche]

I don't know if it is this code that gives problems to be written in a way that works for both cython 0.29 and 3, but a potential different way of writing it:

def __iter__(self):
return self
def __next__(self):
return self.read_next_message()

[kou]

Thanks! I'll try it.

[pitrou]

Do you actually test these changes before pushing them?

You can't use yield in a __next__ function.
Either you write __iter__ as a generator (using yield), or you write a pair of __iter__ and __next__ functions (without yield). You can't mix the two styles.

[jorisvandenbossche]

And FWIW, for this case, I would leave the __iter__ as is (in the other cases such as RecordBatchReader, we already have a public "next" method that can just be called from __next__)

[kou]

Sorry. I didn't test on local before I pushed a commit because I don't have enough time yesterday.

I reverted the __iter__ + __next__ change.

[lidavidm]

Flight changes LGTM, thanks!

[kou]

@jorisvandenbossche Do you have any idea why the following failure was happen?

https://github.com/apache/arrow/actions/runs/5615711782/job/15216554355?pr=36745#step:6:6074

 =================================== FAILURES ===================================
_______________________ test_cloudpickle_dataset[False] ________________________
tempdir = PosixPath('/tmp/pytest-of-root/pytest-0/test_cloudpickle_dataset_False0')
datadir = PosixPath('/opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/data/parquet')
use_legacy_dataset = False
@pytest.mark.pandas
@parametrize_legacy_dataset
def test_cloudpickle_dataset(tempdir, datadir, use_legacy_dataset):
cp = pytest.importorskip('cloudpickle')
dataset = _make_dataset_for_pickling(tempdir, use_legacy_dataset)
> _assert_dataset_is_picklable(
dataset, pickler=cp, use_legacy_dataset=use_legacy_dataset)
opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1572: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1540: in _assert_dataset_is_picklable
assert is_pickleable(dataset)
opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1538: in is_pickleable
return obj == pickler.loads(pickler.dumps(obj))
opt/conda/envs/arrow/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py:73: in dumps
cp.dump(obj)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <cloudpickle.cloudpickle_fast.CloudPickler object at 0x7f25aa27aa10>
obj = <pyarrow.parquet.core._ParquetDatasetV2 object at 0x7f25a9ec9690>
def dump(self, obj):
try:
> return Pickler.dump(self, obj)
E _pickle.PicklingError: Can't pickle <cyfunction ParquetFragmentScanOptions._reconstruct at 0x7f2660177140>: it's not the same object as pyarrow._dataset_parquet.ParquetFragmentScanOptions._reconstruct
opt/conda/envs/arrow/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py:632: PicklingError

[jorisvandenbossche]

Not directly an idea. We use the same pattern in many other places. Can you try this patch:

--- a/python/pyarrow/_dataset_parquet.pyx+++ b/python/pyarrow/_dataset_parquet.pyx@@ -741,7 +741,7 @@ cdef class ParquetFragmentScanOptions(FragmentScanOptions):
thrift_string_size_limit=self.thrift_string_size_limit,
thrift_container_size_limit=self.thrift_container_size_limit,
)
- return type(self)._reconstruct, (kwargs,)+ return ParquetFragmentScanOptions._reconstruct, (kwargs,)

(in theory it shouldn't matter for python, but maybe cython 3 has a bug that treats this different)

[kou]

Thanks! I've pushed your patch.

[h-vetinari]

If there are problems with compatibility between 0.29 and 3.0 (aside from the few intentional breaking changes), the Cython devs are usually quick to respond if you open an issue

[danepitkin]

The failing pickle test is only when using the cloudpickle module instead of the pickle5/pickle module. Perhaps they don't support cython3.0.0 yet? This is the only test in the entire codebase that tests cloudpickle.. not sure how important it is as a dependency to pyarrow.

@pytest.mark.pandas
@parametrize_legacy_dataset
def test_cloudpickle_dataset(tempdir, datadir, use_legacy_dataset):
cp = pytest.importorskip('cloudpickle')
dataset = _make_dataset_for_pickling(tempdir, use_legacy_dataset)
_assert_dataset_is_picklable(
dataset, pickler=cp, use_legacy_dataset=use_legacy_dataset)

[pitrou]

Well, first, this PR should be rebased, because there are conflicts.

[danepitkin]

I did rebase/squash to test things out here: #37097

I wanted to experiment without polluting this PR. I can merge any fixes I find back into this PR.

[kou]

Can we use #37097 instead of this?

[danepitkin]

Can we use #37097 instead of this?

Sure!

[kou]

I close this in favor of #37097.

[kou]

Rationale for this change

Cython 3.0.0 is the latest release. PyArrow should work with Cython 3.0.0.

What changes are included in this PR?

• Don't use vector[XXX]&&
• Add a declaration for postincrement
  • See also: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#c-postincrement-postdecrement-operator
• Ignore C4551 warning (function call missing argument list) with MSVC
  • See also: [BUG] code emitted for Python 3.10 causes warning C4551 in MSVC cython/cython#4445
• Add missing const to CLocation's static methods.
• Don't use StopIteration to stop generator
  • See also: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#python-3-syntax-semantics

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

• Closes: [CI][Python] Cython 3.0 seems to make our build of pyarrow fail #36730

[github-actions]

⚠️ GitHub issue #36730has been automatically assigned in GitHub to PR creator.

[pitrou]

This may fix the compilation error but there are other issues during tests (see macOS CI?).

[kou]

I've fixed CI failures.

[jorisvandenbossche]

@kou there were also some runtime errors in the minimal build that are caused by cython 3 (but it's strange that this is only failing in a single build). Based on your diff, I don't think that will already be fixed here, but let me run them to be sure.

[jorisvandenbossche]

@github-actions crossbow submit example-python-minimal-build-fedora-conda

[github-actions]

Revision: dba6bb4c1adcb233aabce3d595706b5fc104b660

Submitted crossbow builds: ursacomputing/crossbow @ actions-f3bbf75ac8

Task	Status
example-python-minimal-build-fedora-conda

[pitrou]

Can this PR also undo the pinning on various Python builds?

[kou]

@github-actions crossbow submit example-python-minimal-build-fedora-conda

[github-actions]

Revision: 81493b1fa6e3db21630c29ccad0e92499a2a43ff

Submitted crossbow builds: ursacomputing/crossbow @ actions-492c6d5c7d

Task	Status
example-python-minimal-build-fedora-conda

[kou]

I've fixed the generator related problem.
I'll rebase on main and unpin Cython version.

[kou]

Hmm. Can we detect Cython version in .{pyx,pxi}?

[kou]

I want to write something like the following:

diff --git a/python/pyarrow/scalar.pxi b/python/pyarrow/scalar.pxi
index f438c8847..61de38203 100644
--- a/python/pyarrow/scalar.pxi+++ b/python/pyarrow/scalar.pxi@@ -793,7 +793,10 @@ cdef class MapScalar(ListScalar):
"""
arr = self.values
if array is None:
- raise StopIteration+ if Cython.__version__ < '3.0.0':+ raise StopIteration+ else:+ return
for k, v in zip(arr.field('key'), arr.field('value')):
yield (k.as_py(), v.as_py())

[pitrou]

There should not be any need to raise StopIteration.

[jorisvandenbossche]

I don't know if it is this code that gives problems to be written in a way that works for both cython 0.29 and 3, but a potential different way of writing it:

def __iter__(self):
return self
def __next__(self):
return self.read_next_message()

[kou]

Thanks! I'll try it.

[pitrou]

Do you actually test these changes before pushing them?

You can't use yield in a __next__ function.
Either you write __iter__ as a generator (using yield), or you write a pair of __iter__ and __next__ functions (without yield). You can't mix the two styles.

[jorisvandenbossche]

And FWIW, for this case, I would leave the __iter__ as is (in the other cases such as RecordBatchReader, we already have a public "next" method that can just be called from __next__)

[kou]

Sorry. I didn't test on local before I pushed a commit because I don't have enough time yesterday.

I reverted the __iter__ + __next__ change.

[lidavidm]

Flight changes LGTM, thanks!

[kou]

@jorisvandenbossche Do you have any idea why the following failure was happen?

https://github.com/apache/arrow/actions/runs/5615711782/job/15216554355?pr=36745#step:6:6074

 =================================== FAILURES ===================================
_______________________ test_cloudpickle_dataset[False] ________________________
tempdir = PosixPath('/tmp/pytest-of-root/pytest-0/test_cloudpickle_dataset_False0')
datadir = PosixPath('/opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/data/parquet')
use_legacy_dataset = False
@pytest.mark.pandas
@parametrize_legacy_dataset
def test_cloudpickle_dataset(tempdir, datadir, use_legacy_dataset):
cp = pytest.importorskip('cloudpickle')
dataset = _make_dataset_for_pickling(tempdir, use_legacy_dataset)
> _assert_dataset_is_picklable(
dataset, pickler=cp, use_legacy_dataset=use_legacy_dataset)
opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1572: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1540: in _assert_dataset_is_picklable
assert is_pickleable(dataset)
opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1538: in is_pickleable
return obj == pickler.loads(pickler.dumps(obj))
opt/conda/envs/arrow/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py:73: in dumps
cp.dump(obj)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <cloudpickle.cloudpickle_fast.CloudPickler object at 0x7f25aa27aa10>
obj = <pyarrow.parquet.core._ParquetDatasetV2 object at 0x7f25a9ec9690>
def dump(self, obj):
try:
> return Pickler.dump(self, obj)
E _pickle.PicklingError: Can't pickle <cyfunction ParquetFragmentScanOptions._reconstruct at 0x7f2660177140>: it's not the same object as pyarrow._dataset_parquet.ParquetFragmentScanOptions._reconstruct
opt/conda/envs/arrow/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py:632: PicklingError

[jorisvandenbossche]

Not directly an idea. We use the same pattern in many other places. Can you try this patch:

--- a/python/pyarrow/_dataset_parquet.pyx+++ b/python/pyarrow/_dataset_parquet.pyx@@ -741,7 +741,7 @@ cdef class ParquetFragmentScanOptions(FragmentScanOptions):
thrift_string_size_limit=self.thrift_string_size_limit,
thrift_container_size_limit=self.thrift_container_size_limit,
)
- return type(self)._reconstruct, (kwargs,)+ return ParquetFragmentScanOptions._reconstruct, (kwargs,)

(in theory it shouldn't matter for python, but maybe cython 3 has a bug that treats this different)

[kou]

Thanks! I've pushed your patch.

[h-vetinari]

If there are problems with compatibility between 0.29 and 3.0 (aside from the few intentional breaking changes), the Cython devs are usually quick to respond if you open an issue

[danepitkin]

The failing pickle test is only when using the cloudpickle module instead of the pickle5/pickle module. Perhaps they don't support cython3.0.0 yet? This is the only test in the entire codebase that tests cloudpickle.. not sure how important it is as a dependency to pyarrow.

@pytest.mark.pandas
@parametrize_legacy_dataset
def test_cloudpickle_dataset(tempdir, datadir, use_legacy_dataset):
cp = pytest.importorskip('cloudpickle')
dataset = _make_dataset_for_pickling(tempdir, use_legacy_dataset)
_assert_dataset_is_picklable(
dataset, pickler=cp, use_legacy_dataset=use_legacy_dataset)

[pitrou]

Well, first, this PR should be rebased, because there are conflicts.

[danepitkin]

I did rebase/squash to test things out here: #37097

I wanted to experiment without polluting this PR. I can merge any fixes I find back into this PR.

[kou]

Can we use #37097 instead of this?

[danepitkin]

Can we use #37097 instead of this?

Sure!

[kou]

I close this in favor of #37097.

[kou]

Rationale for this change

Cython 3.0.0 is the latest release. PyArrow should work with Cython 3.0.0.

What changes are included in this PR?

• Don't use vector[XXX]&&
• Add a declaration for postincrement
  • See also: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#c-postincrement-postdecrement-operator
• Ignore C4551 warning (function call missing argument list) with MSVC
  • See also: [BUG] code emitted for Python 3.10 causes warning C4551 in MSVC cython/cython#4445
• Add missing const to CLocation's static methods.
• Don't use StopIteration to stop generator
  • See also: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#python-3-syntax-semantics

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

• Closes: [CI][Python] Cython 3.0 seems to make our build of pyarrow fail #36730

[github-actions]

⚠️ GitHub issue #36730has been automatically assigned in GitHub to PR creator.

[pitrou]

This may fix the compilation error but there are other issues during tests (see macOS CI?).

[kou]

I've fixed CI failures.

[jorisvandenbossche]

@kou there were also some runtime errors in the minimal build that are caused by cython 3 (but it's strange that this is only failing in a single build). Based on your diff, I don't think that will already be fixed here, but let me run them to be sure.

[jorisvandenbossche]

@github-actions crossbow submit example-python-minimal-build-fedora-conda

[github-actions]

Revision: dba6bb4c1adcb233aabce3d595706b5fc104b660

Submitted crossbow builds: ursacomputing/crossbow @ actions-f3bbf75ac8

Task	Status
example-python-minimal-build-fedora-conda

[pitrou]

Can this PR also undo the pinning on various Python builds?

[kou]

@github-actions crossbow submit example-python-minimal-build-fedora-conda

[github-actions]

Revision: 81493b1fa6e3db21630c29ccad0e92499a2a43ff

Submitted crossbow builds: ursacomputing/crossbow @ actions-492c6d5c7d

Task	Status
example-python-minimal-build-fedora-conda

[kou]

I've fixed the generator related problem.
I'll rebase on main and unpin Cython version.

[kou]

Hmm. Can we detect Cython version in .{pyx,pxi}?

[kou]

I want to write something like the following:

diff --git a/python/pyarrow/scalar.pxi b/python/pyarrow/scalar.pxi
index f438c8847..61de38203 100644
--- a/python/pyarrow/scalar.pxi+++ b/python/pyarrow/scalar.pxi@@ -793,7 +793,10 @@ cdef class MapScalar(ListScalar):
"""
arr = self.values
if array is None:
- raise StopIteration+ if Cython.__version__ < '3.0.0':+ raise StopIteration+ else:+ return
for k, v in zip(arr.field('key'), arr.field('value')):
yield (k.as_py(), v.as_py())

[pitrou]

There should not be any need to raise StopIteration.

[jorisvandenbossche]

I don't know if it is this code that gives problems to be written in a way that works for both cython 0.29 and 3, but a potential different way of writing it:

def __iter__(self):
return self
def __next__(self):
return self.read_next_message()

[kou]

Thanks! I'll try it.

[pitrou]

Do you actually test these changes before pushing them?

You can't use yield in a __next__ function.
Either you write __iter__ as a generator (using yield), or you write a pair of __iter__ and __next__ functions (without yield). You can't mix the two styles.

[jorisvandenbossche]

And FWIW, for this case, I would leave the __iter__ as is (in the other cases such as RecordBatchReader, we already have a public "next" method that can just be called from __next__)

[kou]

Sorry. I didn't test on local before I pushed a commit because I don't have enough time yesterday.

I reverted the __iter__ + __next__ change.

[lidavidm]

Flight changes LGTM, thanks!

[kou]

@jorisvandenbossche Do you have any idea why the following failure was happen?

https://github.com/apache/arrow/actions/runs/5615711782/job/15216554355?pr=36745#step:6:6074

 =================================== FAILURES ===================================
_______________________ test_cloudpickle_dataset[False] ________________________
tempdir = PosixPath('/tmp/pytest-of-root/pytest-0/test_cloudpickle_dataset_False0')
datadir = PosixPath('/opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/data/parquet')
use_legacy_dataset = False
@pytest.mark.pandas
@parametrize_legacy_dataset
def test_cloudpickle_dataset(tempdir, datadir, use_legacy_dataset):
cp = pytest.importorskip('cloudpickle')
dataset = _make_dataset_for_pickling(tempdir, use_legacy_dataset)
> _assert_dataset_is_picklable(
dataset, pickler=cp, use_legacy_dataset=use_legacy_dataset)
opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1572: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1540: in _assert_dataset_is_picklable
assert is_pickleable(dataset)
opt/conda/envs/arrow/lib/python3.8/site-packages/pyarrow/tests/parquet/test_dataset.py:1538: in is_pickleable
return obj == pickler.loads(pickler.dumps(obj))
opt/conda/envs/arrow/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py:73: in dumps
cp.dump(obj)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <cloudpickle.cloudpickle_fast.CloudPickler object at 0x7f25aa27aa10>
obj = <pyarrow.parquet.core._ParquetDatasetV2 object at 0x7f25a9ec9690>
def dump(self, obj):
try:
> return Pickler.dump(self, obj)
E _pickle.PicklingError: Can't pickle <cyfunction ParquetFragmentScanOptions._reconstruct at 0x7f2660177140>: it's not the same object as pyarrow._dataset_parquet.ParquetFragmentScanOptions._reconstruct
opt/conda/envs/arrow/lib/python3.8/site-packages/cloudpickle/cloudpickle_fast.py:632: PicklingError

[jorisvandenbossche]

Not directly an idea. We use the same pattern in many other places. Can you try this patch:

--- a/python/pyarrow/_dataset_parquet.pyx+++ b/python/pyarrow/_dataset_parquet.pyx@@ -741,7 +741,7 @@ cdef class ParquetFragmentScanOptions(FragmentScanOptions):
thrift_string_size_limit=self.thrift_string_size_limit,
thrift_container_size_limit=self.thrift_container_size_limit,
)
- return type(self)._reconstruct, (kwargs,)+ return ParquetFragmentScanOptions._reconstruct, (kwargs,)

(in theory it shouldn't matter for python, but maybe cython 3 has a bug that treats this different)

[kou]

Thanks! I've pushed your patch.

[h-vetinari]

If there are problems with compatibility between 0.29 and 3.0 (aside from the few intentional breaking changes), the Cython devs are usually quick to respond if you open an issue

[danepitkin]

The failing pickle test is only when using the cloudpickle module instead of the pickle5/pickle module. Perhaps they don't support cython3.0.0 yet? This is the only test in the entire codebase that tests cloudpickle.. not sure how important it is as a dependency to pyarrow.

@pytest.mark.pandas
@parametrize_legacy_dataset
def test_cloudpickle_dataset(tempdir, datadir, use_legacy_dataset):
cp = pytest.importorskip('cloudpickle')
dataset = _make_dataset_for_pickling(tempdir, use_legacy_dataset)
_assert_dataset_is_picklable(
dataset, pickler=cp, use_legacy_dataset=use_legacy_dataset)

[pitrou]

Well, first, this PR should be rebased, because there are conflicts.

[danepitkin]

I did rebase/squash to test things out here: #37097

I wanted to experiment without polluting this PR. I can merge any fixes I find back into this PR.

[kou]

Can we use #37097 instead of this?

[danepitkin]

Can we use #37097 instead of this?

Sure!

[kou]

I close this in favor of #37097.