Skip to content

Conversation

@chitralverma
Copy link
Contributor

@chitralverma chitralverma commented Apr 24, 2025

Which issue does this PR close?

Closes #5943

What changes are included in this PR?

  • Support all read and write options on sync/ async operator open(...)
  • Support all read options on sync/ async operator read(...)
  • Support all write options on sync/ async operator write(...)
  • Add missing capabilities to py bindings
  • Update documentation
  • Update all capabilities
  • Minor fixes linting, typing and typos

Are there any user-facing changes?

No breaking changes, but users can use the new features.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. releases-note/feat The PR implements a new feature or has a title that begins with "feat" size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Apr 24, 2025
@chitralverma chitralverma changed the title WIP: feat(bindings/python): Enhance Reader and Writer feat(bindings/python): Enhance Reader and Writer Apr 28, 2025
@chitralverma
Copy link
Contributor Author

chitralverma commented Apr 28, 2025

@Xuanwo @messense @Zheaoli please have a look whenever you get a chance

the 2 failing tests, seem to show a different service specific bug. the if_not_exist capability is there but the behaviour is not correct.

@chitralverma
Copy link
Contributor Author

@Xuanwo @messense @Zheaoli any suggestions you have for this PR?
I think addition of reader/ writer options to AsyncOperator and the lazy streaming copy of data from 1 file to another can be quite useful features.

@chitralverma
Copy link
Contributor Author

@messense @Xuanwo added some benchmarks and updated the scope of this PR.

See Comment

Please let me know if you have some review comments

@chitralverma
Copy link
Contributor Author

@Xuanwo Since #6215 and #6216 have been merged, I think most of the options for reader_with/ writer_with are already covered there. Some one also suggested looking into write_from at a later time. so do you recommend closing this PR or is some here still valid?

@Xuanwo
Copy link
Member

Xuanwo commented May 26, 2025

@Xuanwo Since #6215 and #6216 have been merged, I think most of the options for reader_with/ writer_with are already covered there. Some one also suggested looking into write_from at a later time. so do you recommend closing this PR or is some here still valid?

Hi, I think the additional options for WriteOptions and ReadOptions are still valid. We can go ahead and merge them. However, there are still some concerns regarding the new API related to write_from; perhaps we can leave them to another PR.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels May 26, 2025
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels May 30, 2025
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels May 30, 2025
Copy link
Member

@erickguan erickguan left a comment

Choose a reason for hiding this comment

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

Great feature and nice chore work!

I added a few suggestions.

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels May 30, 2025
@chitralverma
Copy link
Contributor Author

added a temp fix for the breaking tests.
See discussion #6235

@chitralverma chitralverma requested review from Xuanwo and erickguan May 31, 2025 22:20
Copy link
Member

@erickguan erickguan left a comment

Choose a reason for hiding this comment

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

Minor comments on test exceptions.

LGTM

Comment on lines 190 to 193
with pytest.raises(Exception)as excinfo:
async with await async_operator.open(filename, "wb", if_not_exists=True) as w:
w.write(content)
assert "ConditionNotMatch" in str(excinfo.value)
Copy link
Member

Choose a reason for hiding this comment

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

minor: blank around the keyword as.

Perhaps you can import ConditionNotMatch which aligns with other code. Then:

Suggested change
with pytest.raises(Exception)as excinfo:
async with await async_operator.open(filename, "wb", if_not_exists=True) as w:
w.write(content)
assert "ConditionNotMatch" in str(excinfo.value)
with pytest.raises(ConditionNotMatch):
async with await async_operator.open(filename, "wb", if_not_exists=True) as w:
w.write(content)

Copy link
Member

Choose a reason for hiding this comment

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

related discussion: #6235

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erickguan this is what i did before, but there was a different issue.
I'll address that in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

@chitralverma Thank you so much for bringing up the issue, investigation, and fix! You can add a TODO or a link to the GitHub issue. I will not make this comment in that case :)

Comment on lines 221 to 224
with pytest.raises(Exception)as excinfo:
with operator.open(filename, "wb", if_not_exists=True) as w:
w.write(content)
assert "ConditionNotMatch" in str(excinfo.value)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
with pytest.raises(Exception)as excinfo:
with operator.open(filename, "wb", if_not_exists=True) as w:
w.write(content)
assert "ConditionNotMatch" in str(excinfo.value)
with pytest.raises(ConditionNotMatch):
with operator.open(filename, "wb", if_not_exists=True) as w:
w.write(content)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erickguan this is what i did before, but there was a different issue.
I'll address that in a separate PR.

related discussion: #6235

@chitralverma
Copy link
Contributor Author

@Xuanwo @erickguan all your suggestions have been included now, and I've updated the description for this PR.
Please let me know if there's anything else pending before merging this.

@Xuanwo
Copy link
Member

Xuanwo commented Jun 3, 2025

Thank you @chitralverma for working on this. Although we haven't addressed #6235 yet, I think it's fine to use a workaround in the tests for now.

Will merge after CI passed.

@Xuanwo Xuanwo merged commit 86d1b40 into apache:main Jun 3, 2025
63 checks passed
@chitralverma chitralverma deleted the options-for-rw branch June 3, 2025 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer releases-note/feat The PR implements a new feature or has a title that begins with "feat" size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

new feature: Streaming Reader and Writer interfaces in python binding

4 participants