ARROW-10259: [Rust] Add custom metadata to Field by mqy · Pull Request #9025 · apache/arrow

mqy · 2020-12-28T07:31:27Z

This PR adds the missing custom metadata to data type Field -- the requirement is specified by ARROW-10259.

To adapt existing tests for custom metadata, I updated Field's display: print the struct with debug, this will be improved in later PRs.

github-actions · 2020-12-28T07:44:30Z

https://issues.apache.org/jira/browse/ARROW-10259

nevi-me

Thanks for working on this @mqy. Please enable the relevant integration tests (see 18b9281#diff-16170ebfd75c15cd0ef92a3e4f35dba353edcb11352101c363fc35cf3729267d). They should be marked with a TODO :)

nevi-me · 2020-12-29T07:09:20Z

We should serialise the metadata to a JSON struct in the form of:

{ "metadata": [ {"key": "k", "value": "v"} ] }

There are integration tests that we need to enable, I'll find the relevant file in the repository.

Fixed in impl From<&Field> for ArrowJsonField, thanks!

@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

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?

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.

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 #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?

Sorry, forgot to reply earlier. Yeah, there's no harm in keeping the behaviour as is in this PR

mqy · 2020-12-29T12:37:29Z

Thanks for working on this @mqy. Please enable the relevant integration tests (see 18b9281#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!.

codecov-io · 2021-01-01T09:02:12Z

Codecov Report

Merging #9025 (d70f6bd) into master (5f2495d) will decrease coverage by 0.01%.
The diff coverage is 82.30%.

@@            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	Coverage Δ
rust/arrow/src/array/builder.rs	`85.89% <ø> (ø)`
rust/datafusion/src/physical_plan/planner.rs	`77.89% <ø> (ø)`
rust/datafusion/src/sql/planner.rs	`84.05% <33.33%> (+0.08%)`	⬆️
rust/arrow/src/ipc/convert.rs	`89.77% <47.61%> (-2.14%)`	⬇️
rust/arrow/src/util/integration_util.rs	`66.59% <81.96%> (-0.89%)`	⬇️
rust/arrow/src/datatypes.rs	`78.69% <86.99%> (+1.02%)`	⬆️
rust/datafusion/src/logical_plan/dfschema.rs	`85.52% <100.00%> (+0.06%)`	⬆️
rust/datafusion/src/logical_plan/display.rs	`94.93% <100.00%> (ø)`
rust/datafusion/src/logical_plan/plan.rs	`82.48% <100.00%> (ø)`
rust/datafusion/src/physical_plan/expressions.rs	`84.48% <100.00%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a911e67...d70f6bd. Read the comment docs.

nevi-me · 2021-01-04T19:31:57Z

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.

mqy · 2021-01-05T06:08:53Z

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!

nevi-me · 2021-01-05T08:09:10Z

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")

mqy · 2021-01-06T11:40:55Z

@nevi-me the integration test passed at commit "ipc: build field metadata", please take some time to review, thanks!

nevi-me

Thanks for patiently working on this @mqy. I'm happy with the implementation.

I have one change on with_metadata(&mut self, ...) -> Self;, where we should change the signature to not return the cloned Self.

I'll think about whether we should keep the {"k": "v"} format in the long run, or whether to standardise with {"key": "k", "value": "v"}. I might enounter more examples of this in the coming weeks as I work on testing the Rust Parquet work with other languages.

@alamb @jorgecarleitao may I please have a concurring review, more to check code style.

@carols10cents this might also be of interest to you, not sure if we have Flight tests that require Field to have custom metadata.

nevi-me · 2021-01-06T11:44:25Z

    }

    /// 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.


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?

I think this "good now, better later" strategy is a good one. I agree with @nevi-me 's suggestion

yeah, i agree as well. would be better to have an atomic merge implementation done as a separate PR in the future.

nevi-me · 2021-01-06T11:45:59Z

Sorry, forgot to reply earlier. Yeah, there's no harm in keeping the behaviour as is in this PR

alamb

This looks like a nice change -- thank you @mqy and @nevi-me

github-actions Bot added Component: Rust - DataFusion Component: Rust labels Dec 28, 2020

mqy marked this pull request as draft December 28, 2020 08:06

mqy changed the title ~~ARROW-10259: [Rust] Add custom_metadata to Field~~ ARROW-10259: [Rust] Add custom metadata to Field Dec 28, 2020

mqy force-pushed the field-metadata branch 2 times, most recently from 3623c7e to 0ec54e3 Compare December 28, 2020 10:12

mqy marked this pull request as ready for review December 28, 2020 10:14

mqy marked this pull request as draft December 28, 2020 11:44

nevi-me self-requested a review December 29, 2020 07:02

nevi-me reviewed Dec 29, 2020

View reviewed changes

mqy mentioned this pull request Dec 30, 2020

ARROW-11063: [Rust] [Breaking] Validate null counts when building arrays #9041

Closed

mqy force-pushed the field-metadata branch from aaacd55 to f0030e0 Compare December 30, 2020 10:48

mqy marked this pull request as ready for review December 30, 2020 10:49

mqy mentioned this pull request Dec 30, 2020

ARROW-9777: [Rust] [IPC] write custom metadata [WIP] #9011

Closed

mqy force-pushed the field-metadata branch from f0030e0 to 2417122 Compare December 30, 2020 13:47

mqy marked this pull request as draft December 30, 2020 16:28

mqy force-pushed the field-metadata branch from f012668 to 4274dc8 Compare January 1, 2021 08:48

mqy requested a review from nevi-me January 4, 2021 09:17

mqy marked this pull request as ready for review January 4, 2021 09:17

mqy force-pushed the field-metadata branch from 601297b to 68020cf Compare January 4, 2021 09:21

mqy added 4 commits January 6, 2021 13:04

Add custom metadata to Field

3456eac

Test Field from/to JSON

6582133

ArrowJsonField::from(): convert metadata

e2e2548

Update for json integration test

9ae2f68

mqy added 7 commits January 6, 2021 13:04

Support read field metadata from JSON oject(map)

06f5f2b

try merge field metadata

4750df9

the test schema_equality(): add field metadata

6dc7b3f

set_metadata -> with_metadata

c775873

clippy and doc

11c8357

Enable generate_custom_metadata_case for Rust

4b10cb9

Fix field metadata

ec5a7af

mqy force-pushed the field-metadata branch from 17ee2a3 to ec5a7af Compare January 6, 2021 06:56

ipc: build field metadata

d70f6bd

mqy marked this pull request as draft January 6, 2021 10:15

mqy marked this pull request as ready for review January 6, 2021 11:02

Add test data to convert_schema_round_trip

5d170a7

nevi-me approved these changes Jan 6, 2021

View reviewed changes

Change signature of with_metadata

a578fd6

nevi-me closed this in 98159f1 Jan 7, 2021

alamb reviewed Jan 7, 2021

View reviewed changes

rdettai mentioned this pull request Mar 1, 2022

Support extension arrays apache/arrow-rs#218

Closed

asfimport mentioned this pull request Jan 7, 2021

[Rust] Support field metadata #26255

Closed

Uh oh!

Conversation

mqy commented Dec 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Dec 28, 2020

Uh oh!

nevi-me left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mqy Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mqy Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mqy commented Dec 29, 2020

Uh oh!

codecov-io commented Jan 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nevi-me commented Jan 4, 2021

Uh oh!

mqy commented Jan 5, 2021

Uh oh!

nevi-me commented Jan 5, 2021

Uh oh!

mqy commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nevi-me left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mqy commented Dec 28, 2020 •

edited

Loading

mqy Dec 30, 2020 •

edited

Loading

mqy Dec 30, 2020 •

edited

Loading

codecov-io commented Jan 1, 2021 •

edited

Loading

mqy commented Jan 6, 2021 •

edited

Loading