GH-39440: [Python] Calling pyarrow.dataset.ParquetFileFormat.make_write_options as a class method results in a segfault#40976
Conversation
|
I don't think it makes sense to add the check here, because we would have to do that on many existing methods. Example: >>> pa.Array.get_total_buffer_size(0)
Erreur de segmentation |
|
Right, but I would also agree with Joris here: #39440 (comment)
|
|
I opened cython/cython#6127 on the Cython side. Remember that it's fine to suggest additions to third-party libraries or utilities when it's general enough that it could serve other people. |
Ah! It makes sense, then. Can you add the rationale as a comment in the source code? |
Oh cool! Broadening my horizons =) |
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
|
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 074d45f. There were 4 benchmark results indicating a performance regression:
The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them. |
… for Cython 2 (#41059) ### Rationale for this change `test_make_write_options_error` has been failing on Cython 2 crossbow build because in older versions of Cython the methods were "regular" C extension method had type check automatically built in. In Cython 3 that is not the case, see cython/cython#6127 and so the check for `ParquetFileFormat` was added in #40976. ### What changes are included in this PR? Checking the error raised for both messages, type check and the check for `ParquetFileFormat` added in #40976. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: #41043 Authored-by: AlenkaF <frim.alenka@gmail.com> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
… for Cython 2 (#41059) ### Rationale for this change `test_make_write_options_error` has been failing on Cython 2 crossbow build because in older versions of Cython the methods were "regular" C extension method had type check automatically built in. In Cython 3 that is not the case, see cython/cython#6127 and so the check for `ParquetFileFormat` was added in #40976. ### What changes are included in this PR? Checking the error raised for both messages, type check and the check for `ParquetFileFormat` added in #40976. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: #41043 Authored-by: AlenkaF <frim.alenka@gmail.com> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
…ke_write_options as a class method results in a segfault (apache#40976) ### Rationale for this change Calling `make_write_options()` method as class instead of instance method results in segfault. ### What changes are included in this PR? Adds a type check on `self` and raises an error if not `ParquetFileFormat`. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#39440 Lead-authored-by: AlenkaF <frim.alenka@gmail.com> Co-authored-by: Alenka Frim <AlenkaF@users.noreply.github.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr> Signed-off-by: AlenkaF <frim.alenka@gmail.com>
…_error for Cython 2 (apache#41059) ### Rationale for this change `test_make_write_options_error` has been failing on Cython 2 crossbow build because in older versions of Cython the methods were "regular" C extension method had type check automatically built in. In Cython 3 that is not the case, see cython/cython#6127 and so the check for `ParquetFileFormat` was added in apache#40976. ### What changes are included in this PR? Checking the error raised for both messages, type check and the check for `ParquetFileFormat` added in apache#40976. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#41043 Authored-by: AlenkaF <frim.alenka@gmail.com> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
Rationale for this change
Calling
make_write_options()method as class instead of instance method results in segfault.What changes are included in this PR?
Adds a type check on
selfand raises an error if notParquetFileFormat.Are these changes tested?
Yes.
Are there any user-facing changes?
No.
pyarrow.dataset.ParquetFileFormat.make_write_optionsas a class method results in a segfault #39440