Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-2385: [Rust] implement to_json for DataType and Field #1829

Closed
wants to merge 5 commits into from
Closed

ARROW-2385: [Rust] implement to_json for DataType and Field #1829

wants to merge 5 commits into from

Conversation

andygrove
Copy link
Member

@andygrove andygrove commented Apr 3, 2018

Note that this PR also moves some tests for comparing arrays from Array to Buffer and removes some redundant code that was implemented before it was possible to get a type-safe Iterator from Buffer.

This change was made in this PR because the serde_json crate's macros pretty much forced me to address this now.

@andygrove andygrove changed the title implement to_json for DataType and Field ARROW-2385: [Rust] implement to_json for DataType and Field Apr 3, 2018
@andygrove andygrove changed the title ARROW-2385: [Rust] implement to_json for DataType and Field ARROW-2385: [Rust] implement to_json for DataType and Field [** DO NOT MERGE YET **] Apr 3, 2018
@andygrove andygrove changed the title ARROW-2385: [Rust] implement to_json for DataType and Field [** DO NOT MERGE YET **] ARROW-2385: [Rust] implement to_json for DataType and Field Apr 3, 2018
use serde_json::Value;

#[derive(Debug, Clone, PartialEq)]
pub enum ArrowError {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is one of these in errors.rs FYI

Copy link
Member

@xhochy xhochy left a comment

Choose a reason for hiding this comment

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

+1, LGTM

@xhochy xhochy closed this in 7081752 Apr 4, 2018
fn test_define_schema() {
let _person = Schema::new(vec![
fn create_struct_type() {
let _person = DataType::Struct(vec![
Copy link
Contributor

Choose a reason for hiding this comment

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

@andygrove are we still using Schema? The tests now all reference Vecs of fields in a single DataType.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think Schema is becoming redundant. I'm still using it in my project to define a RecordBatch but I could switch to using a Vec<Field> instead. Do you want to file a JIRA for removing Schema?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, though to what extent should we mirror https://arrow.apache.org/docs/metadata.html?

That would make a Schema of Fields.
Struct(Vec<Field>) would go from a variant of DataType to defined as children on a Field, whose type was only Struct?

(tbc these are low confidence and I'm only recently learning, so shoot me down if I'm off)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants