-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-36730: [Python] Add support for Cython 3.0.0 #37097
Changes from 38 commits
93c61c7
dd9f5e5
2da3f31
01025e3
cf1ea3f
e2d631e
d1e7d8a
f2937d8
25ba028
097f6f3
3263eaf
5241c24
021e133
ee9f049
0cd3439
148d285
1d99984
fe24f0b
ac2d11f
32a4402
3b10534
bdec809
2df074f
91257db
de35878
a9d99d4
2d59f16
e5e3cea
f47f806
676b538
d89fde1
59fffdc
7c56dcc
27d1128
23e8a3e
4ec5188
90f5321
6f32fcf
ec5754c
eee7573
3c4f581
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
# Licensed to the Apache Software Foundation (ASF) under one | ||
# or more contributor license agreements. See the NOTICE file | ||
# distributed with this work for additional information | ||
# regarding copyright ownership. The ASF licenses this file | ||
# to you under the Apache License, Version 2.0 (the | ||
# "License"); you may not use this file except in compliance | ||
# with the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, | ||
# software distributed under the License is distributed on an | ||
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
|
||
ARG repo | ||
ARG arch | ||
ARG python=3.8 | ||
FROM ${repo}:${arch}-conda-python-${python} | ||
|
||
RUN mamba install -q -y "cython<3" && \ | ||
mamba clean --all |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,7 +29,7 @@ from pyarrow.includes.libarrow_substrait cimport * | |
|
||
cdef CDeclaration _create_named_table_provider( | ||
dict named_args, const std_vector[c_string]& names, const CSchema& schema | ||
): | ||
) noexcept: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we sure this function cannot raise a Python exception? Its code is definitely non-trivial, so the "solution" here looks more like a bandaid. In the interest of moving this forward, can you open a bug for this and we'll revisit later? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function executes user-defined python code, which absolutely can raise an exception. In fact, the test cases specifically do raise an exception. The problem is that the C++ functionality explicitly chooses to ignore it and return its own error. The pyarrow binding is designed with this in mind, so adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed #37235 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll see if I can fix this in a separate PR first. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It would certainly be nice to fix that, but FWIW I don't think that should hold up this PR even longer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI I haven't attempted #37235. I hope its okay to merge as-is given how long this PR has taken. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you just add a comment pointing to GH-37235? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do! |
||
cdef: | ||
c_string c_name | ||
shared_ptr[CTable] c_in_table | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... classmethod doesn't work correctly on Cython 3 anymore? Is it a known issue? Or is this change actually not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
classmethod does work, I can change it back. I tried switching to staticmethod when numpydocs were failing, but it turns out it was the comment that was causing numpydoc parsing errors. I thought staticmethod was a slight improvement since
cls
wasn't actually used in the classmethod.