You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "Ji-Xinyou (via GitHub)" <gi...@apache.org> on 2023/05/26 07:36:09 UTC
[GitHub] [incubator-opendal] Ji-Xinyou opened a new pull request, #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Ji-Xinyou opened a new pull request, #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329
Introduce initializing opendal_operator by key-value pairs
# 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 free the heap memory used only once.
```Rust
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);
```
2. Use a series of keys and values in the `opendal_operator_ptr_from_kvs()`, this api would be like
```Rust
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 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 number of the provided keys and values are correct, making this API really bad.
3. 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 interfaces.
```Rust
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1206462565
##########
bindings/c/src/types.rs:
##########
@@ -178,3 +180,63 @@ impl opendal_metadata {
}
}
}
+
+/// [`opendal_operator_options`] represents a series of string type key-value pairs, it may be used for initialization
+#[repr(transparent)]
+pub struct opendal_operator_options {
+ pub inner: *mut HashMap<String, String>,
Review Comment:
> This is not required, I have changed it to pub(crate) now.
My point is can we make it priviate? And can we remove `HashMap_String__String` from our C API?
##########
bindings/c/src/types.rs:
##########
@@ -178,3 +180,63 @@ impl opendal_metadata {
}
}
}
+
+/// [`opendal_operator_options`] represents a series of string type key-value pairs, it may be used for initialization
+#[repr(transparent)]
+pub struct opendal_operator_options {
+ pub inner: *mut HashMap<String, String>,
Review Comment:
> This is not required, I have changed it to pub(crate) now.
My point is can we make it private? And can we remove `HashMap_String__String` from our C API?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1206469552
##########
bindings/c/src/types.rs:
##########
@@ -178,3 +180,63 @@ impl opendal_metadata {
}
}
}
+
+/// [`opendal_operator_options`] represents a series of string type key-value pairs, it may be used for initialization
+#[repr(transparent)]
+pub struct opendal_operator_options {
+ pub inner: *mut HashMap<String, String>,
Review Comment:
I'm bit confused about that. It's ok to use `HashMap` in `opendal_operator_options`. But we expose a public struct call `opendal_operator_options` in C with its own API. Why we need to expose `HashMap_String__String` too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Ji-Xinyou commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Ji-Xinyou (via GitHub)" <gi...@apache.org>.
Ji-Xinyou commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1206458037
##########
bindings/c/src/types.rs:
##########
@@ -178,3 +180,63 @@ impl opendal_metadata {
}
}
}
+
+/// [`opendal_operator_options`] represents a series of string type key-value pairs, it may be used for initialization
+#[repr(transparent)]
+pub struct opendal_operator_options {
+ pub inner: *mut HashMap<String, String>,
Review Comment:
We could use pub(crate)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#issuecomment-1563972022
> 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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo merged pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Ji-Xinyou commented on pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Ji-Xinyou (via GitHub)" <gi...@apache.org>.
Ji-Xinyou commented on PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#issuecomment-1563970177
> `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:
>
> ```c
> 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Ji-Xinyou commented on pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Ji-Xinyou (via GitHub)" <gi...@apache.org>.
Ji-Xinyou commented on PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#issuecomment-1563943380
Related #2321
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] silver-ymz commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "silver-ymz (via GitHub)" <gi...@apache.org>.
silver-ymz commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1229717700
##########
bindings/c/include/opendal.h:
##########
@@ -311,6 +338,39 @@ bool opendal_metadata_is_file(const opendal_metadata *self);
*/
bool opendal_metadata_is_dir(const opendal_metadata *self);
+/*
+ Construct a heap-allocated opendal_operator_options
+ */
+opendal_operator_options opendal_operator_options_new(void);
+
+/*
+ Set a Key-Value pair inside opendal_operator_options
+
+ # Safety
+
+ This function is unsafe because it dereferences and casts the raw pointers
+ Make sure the pointer of `key` and `value` point to a valid string.
+
+ # Example
+
+ ```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);
+ ```
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Ji-Xinyou commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Ji-Xinyou (via GitHub)" <gi...@apache.org>.
Ji-Xinyou commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1206460857
##########
bindings/c/src/types.rs:
##########
@@ -178,3 +180,63 @@ impl opendal_metadata {
}
}
}
+
+/// [`opendal_operator_options`] represents a series of string type key-value pairs, it may be used for initialization
+#[repr(transparent)]
+pub struct opendal_operator_options {
+ pub inner: *mut HashMap<String, String>,
Review Comment:
This is not required, I have changed it to pub(crate) now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1206459329
##########
bindings/c/src/types.rs:
##########
@@ -178,3 +180,63 @@ impl opendal_metadata {
}
}
}
+
+/// [`opendal_operator_options`] represents a series of string type key-value pairs, it may be used for initialization
+#[repr(transparent)]
+pub struct opendal_operator_options {
+ pub inner: *mut HashMap<String, String>,
Review Comment:
It looks good to me. I just want to know is this required by FFI?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1206471924
##########
bindings/c/src/types.rs:
##########
@@ -178,3 +180,63 @@ impl opendal_metadata {
}
}
}
+
+/// [`opendal_operator_options`] represents a series of string type key-value pairs, it may be used for initialization
+#[repr(transparent)]
+pub struct opendal_operator_options {
+ pub inner: *mut HashMap<String, String>,
Review Comment:
> I access the keys and values in the `opendal_operator_new()`, so a pub(crate) will make it easier.
~~Oh, I got it. It's better to add a API `into_inner` (in Rust only) for `opendal_operator_options` instead access inner directly.~~
We can't move the value from `opendal_operator_options`, let's keep the code AS-IS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#issuecomment-1563963426
`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:
```c
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`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Ji-Xinyou commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Ji-Xinyou (via GitHub)" <gi...@apache.org>.
Ji-Xinyou commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1206473557
##########
bindings/c/src/types.rs:
##########
@@ -178,3 +180,63 @@ impl opendal_metadata {
}
}
}
+
+/// [`opendal_operator_options`] represents a series of string type key-value pairs, it may be used for initialization
+#[repr(transparent)]
+pub struct opendal_operator_options {
+ pub inner: *mut HashMap<String, String>,
Review Comment:
> I'm bit confused about that. It's ok to use `HashMap` in `opendal_operator_options`. But we expose a public struct call `opendal_operator_options` in C with its own API. Why we need to expose `HashMap_String__String` too?
Inside the opendal_operator_options, it is a pointer to a HashMap<String, String>.
When we compile the opendal library, we actually compile it along with the Ruststd (since it is a Rust code).
Therefore, the compiled library will contains a type called `HashMap_String__String`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Ji-Xinyou commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Ji-Xinyou (via GitHub)" <gi...@apache.org>.
Ji-Xinyou commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1229986200
##########
bindings/c/include/opendal.h:
##########
@@ -311,6 +338,39 @@ bool opendal_metadata_is_file(const opendal_metadata *self);
*/
bool opendal_metadata_is_dir(const opendal_metadata *self);
+/*
+ Construct a heap-allocated opendal_operator_options
+ */
+opendal_operator_options opendal_operator_options_new(void);
+
+/*
+ Set a Key-Value pair inside opendal_operator_options
+
+ # Safety
+
+ This function is unsafe because it dereferences and casts the raw pointers
+ Make sure the pointer of `key` and `value` point to a valid string.
+
+ # Example
+
+ ```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);
+ ```
Review Comment:
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 https://github.com/apache/incubator-opendal/issues/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Ji-Xinyou commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Ji-Xinyou (via GitHub)" <gi...@apache.org>.
Ji-Xinyou commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1206466709
##########
bindings/c/src/types.rs:
##########
@@ -178,3 +180,63 @@ impl opendal_metadata {
}
}
}
+
+/// [`opendal_operator_options`] represents a series of string type key-value pairs, it may be used for initialization
+#[repr(transparent)]
+pub struct opendal_operator_options {
+ pub inner: *mut HashMap<String, String>,
Review Comment:
> My point is can we make it private?
I access the keys and values in the `opendal_operator_new()`, so a pub(crate) will make it easier.
> And can we remove `HashMap_String__String` from our C API?
Then we have to implement our own hashmap or include third-party dependencies, which is not what we want I think.
If we really really hate the `HashMap_String__String`, we should use the original solution of `opendal_kv` (maybe another name).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1206471924
##########
bindings/c/src/types.rs:
##########
@@ -178,3 +180,63 @@ impl opendal_metadata {
}
}
}
+
+/// [`opendal_operator_options`] represents a series of string type key-value pairs, it may be used for initialization
+#[repr(transparent)]
+pub struct opendal_operator_options {
+ pub inner: *mut HashMap<String, String>,
Review Comment:
> I access the keys and values in the `opendal_operator_new()`, so a pub(crate) will make it easier.
Oh, I got it. It's better to add a API `inner(&self) -> &HashMap` (in Rust only) for `opendal_operator_options` instead access inner directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1206471924
##########
bindings/c/src/types.rs:
##########
@@ -178,3 +180,63 @@ impl opendal_metadata {
}
}
}
+
+/// [`opendal_operator_options`] represents a series of string type key-value pairs, it may be used for initialization
+#[repr(transparent)]
+pub struct opendal_operator_options {
+ pub inner: *mut HashMap<String, String>,
Review Comment:
> I access the keys and values in the `opendal_operator_new()`, so a pub(crate) will make it easier.
Oh, I got it. It's better to add a API `into_inner` (in Rust only) for `opendal_operator_options` instead access inner directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Ji-Xinyou commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Ji-Xinyou (via GitHub)" <gi...@apache.org>.
Ji-Xinyou commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1206456254
##########
bindings/c/include/opendal.h:
##########
@@ -192,17 +199,37 @@ extern "C" {
#endif // __cplusplus
/*
- Returns a result type [`opendal_result_op`], with operator_ptr. If the construction succeeds
- the error is nullptr, otherwise it contains the error information.
+ Uses an array of key-value pairs to initialize the operator based on provided scheme.
+
+ # Example
+
+ Following is a C example.
Review Comment:
The comment in the header is auto generated from the Rust code.
I will check cbindgen doc to see if there is a way of avoiding this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Ji-Xinyou commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Ji-Xinyou (via GitHub)" <gi...@apache.org>.
Ji-Xinyou commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1206459126
##########
bindings/c/include/opendal.h:
##########
@@ -192,17 +199,37 @@ extern "C" {
#endif // __cplusplus
/*
- Returns a result type [`opendal_result_op`], with operator_ptr. If the construction succeeds
- the error is nullptr, otherwise it contains the error information.
+ Uses an array of key-value pairs to initialize the operator based on provided scheme.
+
+ # Example
+
+ Following is a C example.
+ ```no_run
+ opendal_operator_options options = opendal_operator_options_new();
+ opendal_operator_options_set(&options, "root", "/myroot");
+
+ opendal_operator_ptr ptr = opendal_operator_from_kvs("memory", options);
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1206452889
##########
bindings/c/include/opendal.h:
##########
@@ -192,17 +199,37 @@ extern "C" {
#endif // __cplusplus
/*
- Returns a result type [`opendal_result_op`], with operator_ptr. If the construction succeeds
- the error is nullptr, otherwise it contains the error information.
+ Uses an array of key-value pairs to initialize the operator based on provided scheme.
+
+ # Example
+
+ Following is a C example.
Review Comment:
I think we don't need this in a `.h` file?
##########
bindings/c/include/opendal.h:
##########
@@ -192,17 +199,37 @@ extern "C" {
#endif // __cplusplus
/*
- Returns a result type [`opendal_result_op`], with operator_ptr. If the construction succeeds
- the error is nullptr, otherwise it contains the error information.
+ Uses an array of key-value pairs to initialize the operator based on provided scheme.
+
+ # Example
+
+ Following is a C example.
+ ```no_run
+ opendal_operator_options options = opendal_operator_options_new();
+ opendal_operator_options_set(&options, "root", "/myroot");
+
+ opendal_operator_ptr ptr = opendal_operator_from_kvs("memory", options);
Review Comment:
Should be `opendal_operator_new`?
##########
bindings/c/src/types.rs:
##########
@@ -178,3 +180,63 @@ impl opendal_metadata {
}
}
}
+
+/// [`opendal_operator_options`] represents a series of string type key-value pairs, it may be used for initialization
+#[repr(transparent)]
+pub struct opendal_operator_options {
+ pub inner: *mut HashMap<String, String>,
Review Comment:
Is this `pub` required?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1206457218
##########
bindings/c/include/opendal.h:
##########
@@ -192,17 +199,37 @@ extern "C" {
#endif // __cplusplus
/*
- Returns a result type [`opendal_result_op`], with operator_ptr. If the construction succeeds
- the error is nullptr, otherwise it contains the error information.
+ Uses an array of key-value pairs to initialize the operator based on provided scheme.
+
+ # Example
+
+ Following is a C example.
Review Comment:
Oh, that's fine. Let's keep AS-IS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Ji-Xinyou commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Ji-Xinyou (via GitHub)" <gi...@apache.org>.
Ji-Xinyou commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1206474946
##########
bindings/c/src/types.rs:
##########
@@ -178,3 +180,63 @@ impl opendal_metadata {
}
}
}
+
+/// [`opendal_operator_options`] represents a series of string type key-value pairs, it may be used for initialization
+#[repr(transparent)]
+pub struct opendal_operator_options {
+ pub inner: *mut HashMap<String, String>,
Review Comment:
> > I access the keys and values in the `opendal_operator_new()`, so a pub(crate) will make it easier.
>
> Oh, I got it. It's better to add a API `into_inner` (in Rust only) for `opendal_operator_options` instead access inner directly.
Maybe not `into_inner`, since it will consume the HashMap. We could just have a `as_ref`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #2329: feat(bindings/C): opendal_operator_ptr construction using kvs
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2329:
URL: https://github.com/apache/incubator-opendal/pull/2329#discussion_r1206476418
##########
bindings/c/src/types.rs:
##########
@@ -178,3 +180,63 @@ impl opendal_metadata {
}
}
}
+
+/// [`opendal_operator_options`] represents a series of string type key-value pairs, it may be used for initialization
+#[repr(transparent)]
+pub struct opendal_operator_options {
+ pub inner: *mut HashMap<String, String>,
Review Comment:
> When we compile the opendal library, we actually compile it along with the Ruststd (since it is a Rust code).
Thanks a lot! Lesson learnt.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org