You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/12/28 07:31:38 UTC
[GitHub] [arrow] mqy opened a new pull request #9025: ARROW-10259: [Rust] Add custom_metadata to Field
mqy opened a new pull request #9025:
URL: https://github.com/apache/arrow/pull/9025
This PR adds the missing custom metadata to data type `Field`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r549939736
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -22,6 +22,7 @@
//! * [`Field`](crate::datatypes::Field) to describe one field within a schema.
//! * [`DataType`](crate::datatypes::DataType) to describe the type of a field.
+use std::collections::BTreeMap;
Review comment:
Copied from [rust doc std/collections](https://doc.rust-lang.org/std/collections/index.html):
**`Use a HashMap when:**
You want to associate arbitrary keys with an arbitrary value.
You want a cache.
You want a map, with no extra functionality.
**Use a BTreeMap when:**
You want a map sorted by its keys.
You want to be able to get a range of entries on-demand.
You're interested in what the smallest or largest key-value pair is.
You want to find the largest or smallest key that is smaller or larger than something.`
The only reason that use `BTreeMap` for `Field` is: don't break the `'Hash`, `PartialOrd`, `Ord`.
I almost can't image any situation that will sort schemas, unless compare/sort a schema by versions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r552601425
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1296,9 +1301,31 @@ impl Field {
nullable,
dict_id,
dict_is_ordered,
+ metadata: None,
}
}
+ /// Sets the `Field`'s optional custom metadata.
+ /// The metadata is set as `None` for empty map.
+ /// Return cloned `Self`.
+ #[inline]
+ pub fn with_metadata(&mut self, metadata: Option<BTreeMap<String, String>>) -> Self {
Review comment:
@nevi-me I totally agree that the cloned behavior is a bug instead of coding style problem.
I had fixed it per your suggestion and renamed the function from `with_metadata` to `set_metadata` without any return value.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#issuecomment-754477186
Please enable `generate_custom_metadata_case()`in `dev/archery/archery/integration/datagen.py`. You'll see the Rust case disabled.
That integration test fails with
```
Converting /tmp/arrow-integration-kj0llr2i/generated_custom_metadata.json to /tmp/tmpecqxfv9u/70af42de_generated_custom_metadata.json_as_file
Error: ParseError("Field \'metadata\' must have exact one json map for a key-value pair")
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] codecov-io edited a comment on pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#issuecomment-753289181
# [Codecov](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=h1) Report
> Merging [#9025](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=desc) (68020cf) into [master](https://codecov.io/gh/apache/arrow/commit/86cf246d161512923253baa8fe62e00de88db73a?el=desc) (86cf246) will **decrease** coverage by `0.01%`.
> The diff coverage is `77.85%`.
[](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9025 +/- ##
==========================================
- Coverage 82.60% 82.59% -0.02%
==========================================
Files 204 204
Lines 50175 50317 +142
==========================================
+ Hits 41447 41559 +112
- Misses 8728 8758 +30
```
| [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [rust/arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvYnVpbGRlci5ycw==) | `85.89% <ø> (ø)` | |
| [rust/datafusion/src/physical\_plan/planner.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3BsYW5uZXIucnM=) | `77.89% <ø> (ø)` | |
| [rust/arrow/src/util/integration\_util.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvdXRpbC9pbnRlZ3JhdGlvbl91dGlsLnJz) | `66.31% <35.29%> (-1.17%)` | :arrow_down: |
| [rust/arrow/src/ipc/convert.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2NvbnZlcnQucnM=) | `91.58% <77.77%> (-0.32%)` | :arrow_down: |
| [rust/arrow/src/datatypes.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvZGF0YXR5cGVzLnJz) | `75.95% <83.47%> (+0.92%)` | :arrow_up: |
| [rust/datafusion/src/logical\_plan/dfschema.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZGZzY2hlbWEucnM=) | `85.52% <100.00%> (+0.06%)` | :arrow_up: |
| [rust/arrow/src/ipc/gen/Schema.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9TY2hlbWEucnM=) | `38.99% <0.00%> (+0.23%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=footer). Last update [f49f293...68020cf](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] codecov-io commented on pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#issuecomment-753289181
# [Codecov](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=h1) Report
> Merging [#9025](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=desc) (4274dc8) into [master](https://codecov.io/gh/apache/arrow/commit/51672b28e97f19f70de0f0a8800c40ee9bb939d3?el=desc) (51672b2) will **decrease** coverage by `0.01%`.
> The diff coverage is `76.92%`.
[](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9025 +/- ##
==========================================
- Coverage 82.61% 82.59% -0.02%
==========================================
Files 202 202
Lines 50048 50196 +148
==========================================
+ Hits 41347 41461 +114
- Misses 8701 8735 +34
```
| [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [rust/arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvYnVpbGRlci5ycw==) | `84.00% <ø> (ø)` | |
| [rust/datafusion/src/physical\_plan/planner.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3BsYW5uZXIucnM=) | `77.89% <ø> (ø)` | |
| [rust/arrow/src/util/integration\_util.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvdXRpbC9pbnRlZ3JhdGlvbl91dGlsLnJz) | `66.38% <38.88%> (-1.10%)` | :arrow_down: |
| [rust/arrow/src/ipc/convert.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2NvbnZlcnQucnM=) | `91.14% <63.63%> (-0.77%)` | :arrow_down: |
| [rust/arrow/src/datatypes.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvZGF0YXR5cGVzLnJz) | `77.21% <83.20%> (+0.75%)` | :arrow_up: |
| [rust/datafusion/src/logical\_plan/dfschema.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZGZzY2hlbWEucnM=) | `85.52% <100.00%> (+0.06%)` | :arrow_up: |
| [rust/arrow/src/array/transform/fixed\_binary.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvdHJhbnNmb3JtL2ZpeGVkX2JpbmFyeS5ycw==) | `78.94% <0.00%> (-5.27%)` | :arrow_down: |
| [rust/arrow/src/ipc/gen/Schema.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9TY2hlbWEucnM=) | `38.99% <0.00%> (+0.23%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=footer). Last update [51672b2...4274dc8](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r550250838
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1903,9 +1929,20 @@ mod tests {
#[test]
fn serde_struct_type() {
+ let kv_array = [("k".to_string(), "v".to_string())];
Review comment:
> I can't remember where I encountered it, might be in the `0.14.1` test files. A quick way of seeing where the object example comes from, is to comment out that portion of the code, and run the integration tests.
@nevi-me After comment out, datatypes::tests::schema_json failed, but the test data are not from `testing` module, instead are manually constructed. Files with "metadata" in the file name can only be found in these directories: 1.0.0-bigendian and 1.0.0-littleendian/. The Object format for Schema metadata came from https://github.com/apache/arrow/pull/5907 by @grundprinzip
Anyway, I think it's no harm to support parsing `Field` metadata from JSON `Object(Map)`, just as what Schema did.
What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy edited a comment on pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy edited a comment on pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#issuecomment-755251712
@nevi-me the integration test passed at commit "ipc: build field metadata", please take some time to review, thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r550113231
##########
File path: rust/arrow/src/util/integration_util.rs
##########
@@ -60,13 +60,22 @@ pub struct ArrowJsonField {
impl From<&Field> for ArrowJsonField {
fn from(field: &Field) -> Self {
+ let mut metadata_value = None;
+ if let Some(kv_list) = field.metadata() {
+ let mut json_map = VMap::new();
+ for (k, v) in kv_list {
+ json_map.insert(k.clone(), Value::String(v.clone()));
+ }
+ metadata_value = Some(Value::Object(json_map));
+ }
+
Self {
name: field.name().to_string(),
field_type: field.data_type().to_json(),
nullable: field.is_nullable(),
children: vec![],
- dictionary: None, // TODO: not enough info
- metadata: None, // TODO(ARROW-10259)
+ dictionary: None, // TODO: not enough info
+ metadata: metadata_value, // TODO(ARROW-10259): metadata is not used.
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r550111143
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1594,7 +1620,7 @@ impl Field {
impl fmt::Display for Field {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
- write!(f, "{}: {:?}", self.name, self.data_type)
+ write!(f, "{:?}", self)
Review comment:
`#[derive(Display)]` does not work, are suggesting https://crates.io/crates/derive_more ?
Anyway, I tend to leave this change to a following PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r550200770
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1594,7 +1620,7 @@ impl Field {
impl fmt::Display for Field {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
- write!(f, "{}: {:?}", self.name, self.data_type)
+ write!(f, "{:?}", self)
Review comment:
Ah, yes, I was thinking of `#[derive(Debug)]`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#issuecomment-754171271
Hey @mqy, I'm back in the city, and have access to my desktop; so I'll be able to review this PR and help you enable integration tests during the week.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r549939736
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -22,6 +22,7 @@
//! * [`Field`](crate::datatypes::Field) to describe one field within a schema.
//! * [`DataType`](crate::datatypes::DataType) to describe the type of a field.
+use std::collections::BTreeMap;
Review comment:
Copied from [rust doc std/collections](https://doc.rust-lang.org/std/collections/index.html):
**Use a HashMap when:**
You want to associate arbitrary keys with an arbitrary value.
You want a cache.
You want a map, with no extra functionality.
**Use a BTreeMap when:**
You want a map sorted by its keys.
You want to be able to get a range of entries on-demand.
You're interested in what the smallest or largest key-value pair is.
You want to find the largest or smallest key that is smaller or larger than something.`
The only reason that use `BTreeMap` for `Field` is: don't break the `Hash`, `PartialOrd`, `Ord`.
I almost can't image any situation that will sort schemas, unless compare/sort a schema by versions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy edited a comment on pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy edited a comment on pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#issuecomment-755237551
@nevi-me the integration test passed, please take some time to review, thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r552525224
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1296,9 +1301,31 @@ impl Field {
nullable,
dict_id,
dict_is_ordered,
+ metadata: None,
}
}
+ /// Sets the `Field`'s optional custom metadata.
+ /// The metadata is set as `None` for empty map.
+ /// Return cloned `Self`.
+ #[inline]
+ pub fn with_metadata(&mut self, metadata: Option<BTreeMap<String, String>>) -> Self {
Review comment:
I think we shouldn't pass a mutable self, then return a new self, because this is going to mutate the incoming `Field` and return a clone of it, i.e.:
```rust
let mut field = Field::new("my_field", DataType::Int32, false);
let field2 = field.with_metadata(Some(metadata));
assert_eq!(field, field2); // this will be true, so we're creating 2 fields
```
It would be better to return nothing from the function, as we've already mutated `&mut self`.
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1903,9 +1929,20 @@ mod tests {
#[test]
fn serde_struct_type() {
+ let kv_array = [("k".to_string(), "v".to_string())];
Review comment:
Sorry, forgot to reply earlier. Yeah, there's no harm in keeping the behaviour as is in this PR
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1500,6 +1590,7 @@ impl Field {
}
/// Merge field into self if it is compatible. Struct will be merged recursively.
+ /// NOTE: `self` may be updated to unexpected state in case of merge failure.
Review comment:
I think this is fine, I'm happy with someone who encounters an issue here in future, providing an alternative implementation that doesn't partially mutate `&mut self` if there's a failure.
@houqp any opinion here, as you contributed this function?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy commented on pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy commented on pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#issuecomment-755237551
@nevi-me the integration test passed, please have a look at new changes in recent three commits, thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy removed a comment on pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy removed a comment on pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#issuecomment-755237551
@nevi-me the integration test passed, please take some time to review, thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] codecov-io edited a comment on pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#issuecomment-753289181
# [Codecov](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=h1) Report
> Merging [#9025](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=desc) (d70f6bd) into [master](https://codecov.io/gh/apache/arrow/commit/5f2495da0db1ba9ac98808a69c52d31a10effdf5?el=desc) (5f2495d) will **decrease** coverage by `0.01%`.
> The diff coverage is `82.30%`.
[](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9025 +/- ##
==========================================
- Coverage 82.57% 82.56% -0.02%
==========================================
Files 204 204
Lines 50330 50487 +157
==========================================
+ Hits 41561 41684 +123
- Misses 8769 8803 +34
```
| [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [rust/arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvYnVpbGRlci5ycw==) | `85.89% <ø> (ø)` | |
| [rust/datafusion/src/physical\_plan/planner.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3BsYW5uZXIucnM=) | `77.89% <ø> (ø)` | |
| [rust/datafusion/src/sql/planner.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9zcWwvcGxhbm5lci5ycw==) | `84.05% <33.33%> (+0.08%)` | :arrow_up: |
| [rust/arrow/src/ipc/convert.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2NvbnZlcnQucnM=) | `89.77% <47.61%> (-2.14%)` | :arrow_down: |
| [rust/arrow/src/util/integration\_util.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvdXRpbC9pbnRlZ3JhdGlvbl91dGlsLnJz) | `66.59% <81.96%> (-0.89%)` | :arrow_down: |
| [rust/arrow/src/datatypes.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvZGF0YXR5cGVzLnJz) | `78.69% <86.99%> (+1.02%)` | :arrow_up: |
| [rust/datafusion/src/logical\_plan/dfschema.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZGZzY2hlbWEucnM=) | `85.52% <100.00%> (+0.06%)` | :arrow_up: |
| [rust/datafusion/src/logical\_plan/display.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZGlzcGxheS5ycw==) | `94.93% <100.00%> (ø)` | |
| [rust/datafusion/src/logical\_plan/plan.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vcGxhbi5ycw==) | `82.48% <100.00%> (ø)` | |
| [rust/datafusion/src/physical\_plan/expressions.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL2V4cHJlc3Npb25zLnJz) | `84.48% <100.00%> (ø)` | |
| ... and [3 more](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=footer). Last update [a911e67...d70f6bd](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r550173817
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1903,9 +1929,20 @@ mod tests {
#[test]
fn serde_struct_type() {
+ let kv_array = [("k".to_string(), "v".to_string())];
Review comment:
@nevi-me I just saw the comment "The JSON can either be an Object or an Array of Objects" from [ARROW-8883: [Rust] [Integration] Enable more tests](https://github.com/apache/arrow/commit/18b92818e7e12c6516511abe33b9e9ff78573571#diff-16170ebfd75c15cd0ef92a3e4f35dba353edcb11352101c363fc35cf3729267d)
That's interesting! But I haven't seen the `Object` example from testing/data/arrow-ipc-stream/integration/1.0.0-littleendian/generated_custom_metadata.json.gz. So would you please give some pointers?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r553446362
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1500,6 +1590,7 @@ impl Field {
}
/// Merge field into self if it is compatible. Struct will be merged recursively.
+ /// NOTE: `self` may be updated to unexpected state in case of merge failure.
Review comment:
yeah, i agree as well. would be better to have an atomic merge implementation done as a separate PR 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy commented on pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy commented on pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#issuecomment-752060823
> Thanks for working on this @mqy. Please enable the relevant integration tests (see [18b9281#diff-16170ebfd75c15cd0ef92a3e4f35dba353edcb11352101c363fc35cf3729267d](https://github.com/apache/arrow/commit/18b92818e7e12c6516511abe33b9e9ff78573571#diff-16170ebfd75c15cd0ef92a3e4f35dba353edcb11352101c363fc35cf3729267d)). They should be marked with a TODO :)
@nevi-me thanks!
I had looked at recent PRs for reader/writer, tried to figure out what to do, finally got lost :( Your feed back helps a lot!.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r550173817
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1903,9 +1929,20 @@ mod tests {
#[test]
fn serde_struct_type() {
+ let kv_array = [("k".to_string(), "v".to_string())];
Review comment:
@nevi-me I just saw the comment "The JSON can either be an Object or an Array of Objects" in from [ARROW-8883: [Rust] [Integration] Enable more tests](https://github.com/apache/arrow/commit/18b92818e7e12c6516511abe33b9e9ff78573571#diff-16170ebfd75c15cd0ef92a3e4f35dba353edcb11352101c363fc35cf3729267d)
That's interesting! But I haven't seen the `Object` example from testing/data/arrow-ipc-stream/integration/1.0.0-littleendian/generated_custom_metadata.json.gz. So would you please give some pointers?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r550201222
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1903,9 +1929,20 @@ mod tests {
#[test]
fn serde_struct_type() {
+ let kv_array = [("k".to_string(), "v".to_string())];
Review comment:
I can't remember where I encountered it, might be in the `0.14.1` test files. A quick way of seeing where the object example comes from, is to comment out that portion of the code, and run the integration tests.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r549593402
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1594,7 +1620,7 @@ impl Field {
impl fmt::Display for Field {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
- write!(f, "{}: {:?}", self.name, self.data_type)
+ write!(f, "{:?}", self)
Review comment:
This defaults us to `Display` which we could `#[derive(Display)]` instead. I think we could remove this function in that case.
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1903,9 +1929,20 @@ mod tests {
#[test]
fn serde_struct_type() {
+ let kv_array = [("k".to_string(), "v".to_string())];
Review comment:
We should serialise the metadata to a JSON struct in the form of:
```json
{
"metadata": [
{"key": "k", "value": "v"}
]
}
```
There are integration tests that we need to enable, I'll find the relevant file in the repository.
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -22,6 +22,7 @@
//! * [`Field`](crate::datatypes::Field) to describe one field within a schema.
//! * [`DataType`](crate::datatypes::DataType) to describe the type of a field.
+use std::collections::BTreeMap;
Review comment:
Is there value in using `BTreeMap` for both metadata types?
##########
File path: rust/arrow/src/util/integration_util.rs
##########
@@ -60,13 +60,22 @@ pub struct ArrowJsonField {
impl From<&Field> for ArrowJsonField {
fn from(field: &Field) -> Self {
+ let mut metadata_value = None;
+ if let Some(kv_list) = field.metadata() {
+ let mut json_map = VMap::new();
+ for (k, v) in kv_list {
+ json_map.insert(k.clone(), Value::String(v.clone()));
+ }
+ metadata_value = Some(Value::Object(json_map));
+ }
+
Self {
name: field.name().to_string(),
field_type: field.data_type().to_json(),
nullable: field.is_nullable(),
children: vec![],
- dictionary: None, // TODO: not enough info
- metadata: None, // TODO(ARROW-10259)
+ dictionary: None, // TODO: not enough info
+ metadata: metadata_value, // TODO(ARROW-10259): metadata is not used.
Review comment:
This is now safe to remove, as you're addressing it in this PR
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy commented on pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy commented on pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#issuecomment-754421764
> Hey @mqy, I'm back in the city, and have access to my desktop; so I'll be able to review this PR and help you enable integration tests during the week.
Welcome back!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r550250838
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1903,9 +1929,20 @@ mod tests {
#[test]
fn serde_struct_type() {
+ let kv_array = [("k".to_string(), "v".to_string())];
Review comment:
> I can't remember where I encountered it, might be in the `0.14.1` test files. A quick way of seeing where the object example comes from, is to comment out that portion of the code, and run the integration tests.
@nevi-me After comment out, datatypes::tests::schema_json failed, but the test data are not from `testing` module, instead are manually constructed. Files with "metadata" in the file name can only be found in these directories: 1.0.0-bigendian and 1.0.0-littleendian/. The Object format for Schema metadata came from https://github.com/apache/arrow/pull/5907 by @grundprinzip
Anyway, I think it's no harm to support parse Field metadata from JSON Object(Map), just as what Schema did.
What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy commented on pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy commented on pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#issuecomment-755251712
@nevi-me the integration test passed, please take some time to review, thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] codecov-io edited a comment on pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#issuecomment-753289181
# [Codecov](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=h1) Report
> Merging [#9025](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=desc) (ec5a7af) into [master](https://codecov.io/gh/apache/arrow/commit/5f2495da0db1ba9ac98808a69c52d31a10effdf5?el=desc) (5f2495d) will **increase** coverage by `0.00%`.
> The diff coverage is `85.51%`.
[](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9025 +/- ##
========================================
Coverage 82.57% 82.57%
========================================
Files 204 204
Lines 50330 50475 +145
========================================
+ Hits 41561 41681 +120
- Misses 8769 8794 +25
```
| [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [rust/arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvYnVpbGRlci5ycw==) | `85.89% <ø> (ø)` | |
| [rust/datafusion/src/physical\_plan/planner.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3BsYW5uZXIucnM=) | `77.89% <ø> (ø)` | |
| [rust/datafusion/src/sql/planner.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9zcWwvcGxhbm5lci5ycw==) | `84.05% <33.33%> (+0.08%)` | :arrow_up: |
| [rust/arrow/src/ipc/convert.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2NvbnZlcnQucnM=) | `91.58% <77.77%> (-0.32%)` | :arrow_down: |
| [rust/arrow/src/util/integration\_util.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvdXRpbC9pbnRlZ3JhdGlvbl91dGlsLnJz) | `66.59% <81.96%> (-0.89%)` | :arrow_down: |
| [rust/arrow/src/datatypes.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvZGF0YXR5cGVzLnJz) | `78.69% <86.99%> (+1.02%)` | :arrow_up: |
| [rust/datafusion/src/logical\_plan/dfschema.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZGZzY2hlbWEucnM=) | `85.52% <100.00%> (+0.06%)` | :arrow_up: |
| [rust/datafusion/src/logical\_plan/display.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZGlzcGxheS5ycw==) | `94.93% <100.00%> (ø)` | |
| [rust/datafusion/src/logical\_plan/plan.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vcGxhbi5ycw==) | `82.48% <100.00%> (ø)` | |
| [rust/datafusion/src/physical\_plan/expressions.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL2V4cHJlc3Npb25zLnJz) | `84.48% <100.00%> (ø)` | |
| ... and [3 more](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=footer). Last update [a911e67...ec5a7af](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me closed pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
nevi-me closed pull request #9025:
URL: https://github.com/apache/arrow/pull/9025
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r549939736
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -22,6 +22,7 @@
//! * [`Field`](crate::datatypes::Field) to describe one field within a schema.
//! * [`DataType`](crate::datatypes::DataType) to describe the type of a field.
+use std::collections::BTreeMap;
Review comment:
Copied from [rust doc std/collections](https://doc.rust-lang.org/std/collections/index.html):
**`Use a HashMap when:**
You want to associate arbitrary keys with an arbitrary value.
You want a cache.
You want a map, with no extra functionality.
**Use a BTreeMap when:**
You want a map sorted by its keys.
You want to be able to get a range of entries on-demand.
You're interested in what the smallest or largest key-value pair is.
You want to find the largest or smallest key that is smaller or larger than something.`
The only reason that use `BTreeMap` for `Field` is: don't break the `Hash`, `PartialOrd`, `Ord`.
I almost can't image any situation that will sort schemas, unless compare/sort a schema by versions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] codecov-io edited a comment on pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#issuecomment-753289181
# [Codecov](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=h1) Report
> Merging [#9025](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=desc) (601297b) into [master](https://codecov.io/gh/apache/arrow/commit/86cf246d161512923253baa8fe62e00de88db73a?el=desc) (86cf246) will **decrease** coverage by `0.01%`.
> The diff coverage is `73.88%`.
[](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9025 +/- ##
==========================================
- Coverage 82.60% 82.59% -0.02%
==========================================
Files 204 204
Lines 50175 50317 +142
==========================================
+ Hits 41447 41559 +112
- Misses 8728 8758 +30
```
| [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [rust/arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvYnVpbGRlci5ycw==) | `85.89% <ø> (ø)` | |
| [rust/datafusion/src/physical\_plan/planner.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3BsYW5uZXIucnM=) | `77.89% <ø> (ø)` | |
| [rust/arrow/src/util/integration\_util.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvdXRpbC9pbnRlZ3JhdGlvbl91dGlsLnJz) | `66.31% <35.29%> (-1.17%)` | :arrow_down: |
| [rust/arrow/src/ipc/convert.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2NvbnZlcnQucnM=) | `91.58% <77.77%> (-0.32%)` | :arrow_down: |
| [rust/arrow/src/datatypes.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvZGF0YXR5cGVzLnJz) | `75.95% <79.24%> (+0.92%)` | :arrow_up: |
| [rust/datafusion/src/logical\_plan/dfschema.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZGZzY2hlbWEucnM=) | `85.52% <100.00%> (+0.06%)` | :arrow_up: |
| [rust/arrow/src/ipc/gen/Schema.rs](https://codecov.io/gh/apache/arrow/pull/9025/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvaXBjL2dlbi9TY2hlbWEucnM=) | `38.99% <0.00%> (+0.23%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=footer). Last update [f49f293...68020cf](https://codecov.io/gh/apache/arrow/pull/9025?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r553434218
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1500,6 +1590,7 @@ impl Field {
}
/// Merge field into self if it is compatible. Struct will be merged recursively.
+ /// NOTE: `self` may be updated to unexpected state in case of merge failure.
Review comment:
I think this "good now, better later" strategy is a good one. I agree with @nevi-me 's suggestion
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9025: ARROW-10259: [Rust] Add custom_metadata to Field
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#issuecomment-751618265
https://issues.apache.org/jira/browse/ARROW-10259
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r552602744
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1296,9 +1301,31 @@ impl Field {
nullable,
dict_id,
dict_is_ordered,
+ metadata: None,
}
}
+ /// Sets the `Field`'s optional custom metadata.
+ /// The metadata is set as `None` for empty map.
+ /// Return cloned `Self`.
+ #[inline]
+ pub fn with_metadata(&mut self, metadata: Option<BTreeMap<String, String>>) -> Self {
Review comment:
Thanks, I'm happy with the rename
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r550111143
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1594,7 +1620,7 @@ impl Field {
impl fmt::Display for Field {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
- write!(f, "{}: {:?}", self.name, self.data_type)
+ write!(f, "{:?}", self)
Review comment:
`#[derive(Display)]` does not work, are you suggesting https://crates.io/crates/derive_more ?
Anyway, I tend to leave this change to a following PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mqy commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field
Posted by GitBox <gi...@apache.org>.
mqy commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r550113422
##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1903,9 +1929,20 @@ mod tests {
#[test]
fn serde_struct_type() {
+ let kv_array = [("k".to_string(), "v".to_string())];
Review comment:
Fixed in `impl From<&Field> for ArrowJsonField`, thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org