GH-36240: [Python] Refactor CumulativeSumOptions to a separate class for independent deprecation #36977
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
Hi, reviewers,here are two choices to handle the problem the PR can't pass
|
danepitkin
reviewed
Aug 1, 2023
Member
danepitkin
left a comment
danepitkin
left a comment
There was a problem hiding this comment.
For the failing test, we can just add an explicit check for CumulativeSumOptions == CumulativeOptions. We can add a TODO comment to remove the check when the deprecated class is removed.
github-actions
bot
added
awaiting committer review
danepitkin
reviewed
Aug 11, 2023
Member
|
One suggestion, otherwise LGTM! Nice work! I'll add an Arrow committer to the reviewer's list. |
Co-authored-by: Dane Pitkin <48041712+danepitkin@users.noreply.github.com>
AlenkaF
approved these changes
Aug 22, 2023
Member
AlenkaF
left a comment
AlenkaF
left a comment
There was a problem hiding this comment.
LGTM +1
Just a small nitpick suggestion for clearer understanding of the TODO, then I will merge 👍
Co-authored-by: Alenka Frim <AlenkaF@users.noreply.github.com>
Member
|
Thanks! |
This was referenced Aug 22, 2023
|
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit fe750ed. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them. |
jorisvandenbossche
pushed a commit
that referenced
this pull request
Aug 23, 2023
…eSumOptions refactor (#37305) ### Rationale for this change Merging of #36977 caused a CI failure due to a test giving a warning. ### What changes are included in this PR? Add a `filterwarnings` mark to the failing tests. This also tests the deprecation message. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: #37303 Authored-by: AlenkaF <frim.alenka@gmail.com> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
loicalleyne
pushed a commit
to loicalleyne/arrow
that referenced
this pull request
Nov 13, 2023
…class for independent deprecation (apache#36977) **Rationale for this change** As apache#36240 says, we refactor CumulativeSumOptions to a separate class. **What changes are included in this PR?** - independent CumulativeSumOptions - the original simple test before apache#32190 - fix a typo in CumulativeOptions **Are these changes tested?** No. Actually, the PR can't pass the `test_option_class_equality` in test_compute.py ([Error example](https://github.com/apache/arrow/actions/runs/5728571658/job/15523443371?pr=36977)). Cause CumulativeSumOptions's C++ part is also CumulativeOptions.  **Are there any user-facing changes?** No. Closes: apache#36240 * Closes: apache#36240 Lead-authored-by: Junming Chen <junming.chen.r@outlook.com> Co-authored-by: Dane Pitkin <48041712+danepitkin@users.noreply.github.com> Co-authored-by: Alenka Frim <AlenkaF@users.noreply.github.com> Signed-off-by: AlenkaF <frim.alenka@gmail.com>
loicalleyne
pushed a commit
to loicalleyne/arrow
that referenced
this pull request
Nov 13, 2023
…ulativeSumOptions refactor (apache#37305) ### Rationale for this change Merging of apache#36977 caused a CI failure due to a test giving a warning. ### What changes are included in this PR? Add a `filterwarnings` mark to the failing tests. This also tests the deprecation message. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#37303 Authored-by: AlenkaF <frim.alenka@gmail.com> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale for this change
As #36240 says, we refactor CumulativeSumOptions to a separate class.
What changes are included in this PR?
Are these changes tested?
No.
Actually, the PR can't pass the
test_option_class_equalityin test_compute.py (Error example). Cause CumulativeSumOptions's C++ part is also CumulativeOptions.Are there any user-facing changes?
No.
Closes: #36240