Skip to content

GH-39579: [Python] fix raising ValueError on _ensure_partitioning#39593

Merged
AlenkaF merged 3 commits intoapache:mainfrom
idailylife:fix_39579
Jan 22, 2024
Merged

GH-39579: [Python] fix raising ValueError on _ensure_partitioning#39593
AlenkaF merged 3 commits intoapache:mainfrom
idailylife:fix_39579

Conversation

@idailylife
Copy link
Copy Markdown
Contributor

@idailylife idailylife commented Jan 14, 2024 •
edited by github-actions bot
Loading

Rationale for this change

The _ensure_partitioning method in dataset.py is missing a "raise" which currently ignores bad scheme silently.

What changes are included in this PR?

Fixed the typo.

Are these changes tested?

Tried with new code that the exception is properly raised.

Are there any user-facing changes?

No.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 14, 2024

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

@AlenkaF
Copy link
Copy Markdown
Member

AlenkaF commented Jan 15, 2024

Thank you for catching this!
We should add a test for it though, preferably in pyarrow/tests/dataset.py::test_partitioning.

@pitrou pitrou requested a review from AlenkaF January 17, 2024 15:30
@AlenkaF
Copy link
Copy Markdown
Member

AlenkaF commented Jan 17, 2024

Can we add a test to pyarrow/tests/dataset.py::test_partitioning?

@idailylife
Copy link
Copy Markdown
Contributor Author

idailylife commented Jan 18, 2024

Can we add a test to pyarrow/tests/dataset.py::test_partitioning?

Yup working on it

@idailylife
Copy link
Copy Markdown
Contributor Author

idailylife commented Jan 22, 2024

Not sure why one of the CIs failed, seemingly unrelated.

AlenkaF
AlenkaF reviewed Jan 22, 2024
View reviewed changes
@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 22, 2024
@AlenkaF
Copy link
Copy Markdown
Member

AlenkaF commented Jan 22, 2024

The failing build did look unrelated, triggered it again to see if it will help.
Also added one nit in the test, looks good otherwise!

@AlenkaF
Copy link
Copy Markdown
Member

AlenkaF commented Jan 22, 2024

The failure is not connected, I can see it elsewhere also: https://github.com/ursacomputing/crossbow/actions/runs/7601318025/job/20700571533

@idailylife @AlenkaF
Update python/pyarrow/tests/test_dataset.py
fe752bf 
Co-authored-by: Alenka Frim <AlenkaF@users.noreply.github.com>
AlenkaF
AlenkaF approved these changes Jan 22, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@AlenkaF AlenkaF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failures are not connected and have an issue opened: #39732

Thanks for the fix!

@AlenkaF AlenkaF merged commit a7f8140 into apache:main Jan 22, 2024
@AlenkaF AlenkaF removed the awaiting committer review Awaiting committer review label Jan 22, 2024
@conbench-apache-arrow
Copy link
Copy Markdown

conbench-apache-arrow bot commented Jan 22, 2024

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit a7f8140.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
@idailylife @AlenkaF @dgreiss
apacheGH-39579: [Python] fix raising ValueError on _ensure_partitioni…
b4d165e 
…ng (apache#39593)

### Rationale for this change
The `_ensure_partitioning` method in dataset.py is missing a "raise" which currently ignores bad scheme silently.

### What changes are included in this PR?

Fixed the typo.

### Are these changes tested?
Tried with new code that the exception is properly raised.

### Are there any user-facing changes?
No.

* Closes: apache#39579

Lead-authored-by: idailylife <iorange@126.com>
Co-authored-by: 0x0000ffff <idailylife@users.noreply.github.com>
Co-authored-by: Alenka Frim <AlenkaF@users.noreply.github.com>
Signed-off-by: AlenkaF <frim.alenka@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlenkaF AlenkaF AlenkaF approved these changes

Assignees

No one assigned

Labels

Component: Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Python] exception not being raised if passed an invalid partitiong on _ensure_partitioning

2 participants

@idailylife @AlenkaF