Skip to content

Conversation

@geruh
Copy link
Contributor

@geruh geruh commented May 26, 2025

Which issue does this PR close?

Related to #6214.

Rationale for this change

This PR implements WriteOptions support in Java bindings as part of the migration to the new options API outlined in RFC-6213 (#6213). This is the first step in migrating Java bindings to use the new options APIs, starting with write operations. Although if you rather have all options migrated in one go we can close.

What changes are included in this PR?

Added a complete mapping and conversion of theopendal::options::WriteOptions and the Java WriteOptions class.

Are there any user-facing changes?

no, but I added the ability to handle chunking, and concurrency options to match opt api.

Let me know if this is the right direction!

@geruh geruh requested a review from tisonkun as a code owner May 26, 2025 02:21
@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" labels May 26, 2025
env: &mut JNIEnv<'a>,
options: &JObject,
) -> Result<opendal::options::WriteOptions> {
let concurrent = match convert::read_int_field(env, options, "concurrent")? {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added input validation to prevent negative values that would cause capacity overflow when casting to usize. negative i32/i64 values wrap to huge positive usize values

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Looks great to me, wait for @tisonkun for another look.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label May 29, 2025
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution!

@tisonkun tisonkun merged commit a4b9f78 into apache:main May 29, 2025
65 checks passed
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:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants