Skip to content

feat(bindings/C): opendal_operator_ptr construction using kvs#2329

Merged
Xuanwo merged 4 commits intoapache:mainfrom
xyjixyjixyji:c-binding-via_map
May 26, 2023
Merged

feat(bindings/C): opendal_operator_ptr construction using kvs#2329
Xuanwo merged 4 commits intoapache:mainfrom
xyjixyjixyji:c-binding-via_map

Conversation

@xyjixyjixyji
Copy link
Copy Markdown
Contributor

@xyjixyjixyji xyjixyjixyji commented May 26, 2023

Introduce initializing opendal_operator by key-value pairs. With this PR, we can build operator on any scheme theoretically 😆 .

Some explainations

For map construction, I came up with essentially three ideas.

  1. Use a opendal_map struct, which is essentially a pointer to Rust Hashmap. I did not use this because this introduce complexity just like we construct a opendal_operator_ptr, users have to pay their extra attention on freeing the heap memory that is even used only once.
opendal_map m = opendal_map_new(kvs: opendal_kvs);
opendal_operator_ptr ptr = opendal_operator_from_map(scheme, m);

/* I don't want this since this is not necessary, we are not using this anymore */
opendal_map_free(m);

opendal_operator_ptr_free(ptr);

  1. Use a series of keys and values in the opendal_operator_ptr_from_kvs(), this api would be like
fn opendal_operator_ptr_from_kvs(scheme: *const c_char, keys: *const c_char, values: *const c_char) -> ....

The pro of this is that we do not need to introduce any new foreign, hard to manage data structures into Rust (like we actually do in this PR for opendal_kv).
The con of this is that users have we make sure that the ordering and the number of the provided keys and values are correct, making this API really bad.


  1. The approach used in this PR. We introduce a new data structure called opendal_kv, which is essentially a simple key-value pair. This helps us to easier align the keys and values provided by user, and provide a more friendly interface.
opendal_kv kvs[3];
kvs[0] = opendal_kv {.key = "k1", .value = "v1"};
kvs[1] = opendal_kv {.key = "k2", .value = "v2"};
kvs[2] = opendal_kv {.key = "k3", .value = "v3"};

opendal_operator_ptr ptr = opendal_operator_ptr_from_kvs("memory", kvs, 3);
...

We do not need to explicitly manage heap allocation of hashmap, manage the correctness of the ordering of key-values, and we can make use of this kv data structure in the future.

@xyjixyjixyji
Copy link
Copy Markdown
Contributor Author

Related #2321

@Xuanwo
Copy link
Copy Markdown
Member

Xuanwo commented May 26, 2023

opendal_kv is not a good name to me as it confused with existing features inside opendal, so does opendal_map.

How about API like the following:

struct opendal_operator_builder *options = opendal_operator_builder_new();
opendal_operator_builder_set(options, "a", "x");
opendal_operator_builder_set(options, "b", "y");
opendal_operator_builder_set(options, "c", "z");
opendal_operator_ptr ptr = opendal_operator_new(options);

Can we implement the move semantic in Rust for freeing opendal_operator_builder in opendal_operator_new?

@xyjixyjixyji
Copy link
Copy Markdown
Contributor Author

opendal_kv is not a good name to me as it confused with existing features inside opendal, so does opendal_map.

How about API like the following:

struct opendal_operator_builder *options = opendal_operator_builder_new();
opendal_operator_builder_set(options, "a", "x");
opendal_operator_builder_set(options, "b", "y");
opendal_operator_builder_set(options, "c", "z");
opendal_operator_ptr ptr = opendal_operator_new(options);

Can we implement the move semantic in Rust for freeing opendal_operator_builder in opendal_operator_new?

IMO the builder is different from this map approach, I think opendal_operator_options may be a good name.

For the move semantic, it is not a common approach in C, usually if user manually allocates a heap memory, the user has to free it himself. (If we free the heap memory in our function, a programmer who often programs in C would probably double free the memory).

@Xuanwo
Copy link
Copy Markdown
Member

Xuanwo commented May 26, 2023

I think opendal_operator_options may be a good name.

LGTM!

For the move semantic, it is not a common approach in C, usually if user manually allocates a heap memory, the user has to free it himself.

Ok, let's add opendal_operator_options_free.

Comment thread bindings/c/include/opendal.h Outdated
Comment thread bindings/c/include/opendal.h
Comment thread bindings/c/src/types.rs Outdated
Copy link
Copy Markdown
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.

Thanks a lot!

@Xuanwo Xuanwo merged commit 98bc820 into apache:main May 26, 2023
@Xuanwo Xuanwo mentioned this pull request May 30, 2023
Comment on lines +356 to +363
```C
opendal_operator_options options = opendal_operator_options_new();
opendal_operator_options_set(&options, "root", "/myroot");

// .. use your opendal_operator_options

opendal_operator_options_free(options);
```
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In this line, it passes wrong type to opendal_operator_options_free. It should be opendal_operator_options_free(&options);

Moreover, opendal_operator_options_new returns a value, but set and free requires a pointer. It seems easy to make mistakes here. Maybe it's better to unify the types.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I originally wanted to unify them into pointers, by #[repr(transparent)], but this will cause the doxygen not recognizing the header cbindgen generated.

I agree that this is a problem and we should fix this, hence I have created an issue #2462 here.

FYI I am sort of being covered by a hell lot of mess these days 😢 , so it may take a while but not too long.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants