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

GH-36760: [Go] Adding avro ocf reader - schema converter #36796

Closed

Conversation

loicalleyne
Copy link
Contributor

@loicalleyne loicalleyne commented Jul 20, 2023

Rationale for this change

First step in implementing an Avro reader.

What changes are included in this PR?

Added Avro schema to Arrow schema converter.

Are these changes tested?

yes

Are there any user-facing changes?

One exported functions:
ArrowSchemaFromAvro returns a new Arrow schema from an Avro schema JSON.
ArrowSchemaFromAvro(avroSchema []byte, includeTopLevel bool) (*arrow.Schema, error)

@loicalleyne loicalleyne requested a review from zeroshade as a code owner July 20, 2023 22:45
@github-actions
Copy link

⚠️ GitHub issue #36760 has been automatically assigned in GitHub to PR creator.

go/arrow/avro/schema.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Jul 24, 2023
go/arrow/avro/common.go Outdated Show resolved Hide resolved
go/arrow/avro/common.go Outdated Show resolved Hide resolved
go/arrow/avro/common.go Outdated Show resolved Hide resolved
go/arrow/avro/common.go Outdated Show resolved Hide resolved
go/arrow/avro/common.go Outdated Show resolved Hide resolved
go/arrow/avro/schema.go Outdated Show resolved Hide resolved
go/arrow/avro/schema.go Outdated Show resolved Hide resolved
go/arrow/avro/schema.go Outdated Show resolved Hide resolved
go/arrow/avro/schema.go Outdated Show resolved Hide resolved
go/arrow/avro/schema.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Aug 1, 2023
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 10, 2023
@loicalleyne loicalleyne requested a review from zeroshade August 11, 2023 18:26
go/arrow/avro/reader.go Outdated Show resolved Hide resolved
go/arrow/avro/reader.go Outdated Show resolved Hide resolved
go/arrow/avro/reader.go Outdated Show resolved Hide resolved
go/arrow/avro/schema.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Aug 14, 2023
Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

Just a few nitpick questions, otherwise this looks good to me

go/arrow/avro/schema.go Outdated Show resolved Hide resolved
go/arrow/avro/schema.go Outdated Show resolved Hide resolved
go/arrow/avro/schema.go Outdated Show resolved Hide resolved
go/arrow/avro/schema.go Outdated Show resolved Hide resolved
go/arrow/avro/schema.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Sep 5, 2023
loicalleyne added a commit to loicalleyne/arrow that referenced this pull request Sep 6, 2023
to match apacheGH-36760: [Go] Adding avro ocf reader - schema converter apache#36796
Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

LGTM i hope rebasing this into the reader PR isn't gonna be a nightmare for you, haha

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Sep 6, 2023
@zeroshade
Copy link
Member

@loicalleyne looks like you've got some build/test failures. Other than fixing those issues, everything else looks good to me

@loicalleyne
Copy link
Contributor Author

loicalleyne commented Sep 7, 2023

github.com/hamba/avro/v2 requires go 1.19 so the builds for go 1.17 and go 1.18 will always fail. How many go releases worth of backwards compatibility does arrow need to maintain?
Also, schema.Equal() has a bug which is demonstrated in schema_test.go, the string representation is identical but the schema.Equal() comparison fails for some reason.

loicalleyne added a commit to loicalleyne/arrow that referenced this pull request Sep 7, 2023
to match apacheGH-36760: [Go] Adding avro ocf reader - schema converter apache#36796
@zeroshade
Copy link
Member

@loicalleyne Hmmm, i didn't realize that. I think i'm fine with us bumping the minimum to go1.19 and dropping the go1.17/go1.18 builds. I'll put together a PR for that. Let's see if we can dig in and figure out the bug in the schema.Equals method.

@zeroshade
Copy link
Member

@loicalleyne should this be dropped/closed in favor of #37115 ? It seems like this hasn't been updated or rebased

@loicalleyne
Copy link
Contributor Author

loicalleyne commented Oct 6, 2023

@zeroshade it's up to you, I submitted the schema converter and the reader in separate PRs to keep the scope small.
To the best of my recollection #37115 contains the exactly the same code in schema.go and schema_test.go with the reader code in the rest of the files. Let me know if you want me to rebase one or both of them.

@zeroshade
Copy link
Member

let's close this one and just use #37115 to track the whole thing. Please rebase that one and have a look at my comments there and close this one. It's likely easier to just keep it all in there than go back and forth between these and have you having to maintain both.

@loicalleyne loicalleyne closed this Oct 6, 2023
@loicalleyne loicalleyne force-pushed the Adding-Avro-OCF-reader-#36760 branch from ae1e08a to 3697bcd Compare October 6, 2023 22:20
loicalleyne added a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
to match apacheGH-36760: [Go] Adding avro ocf reader - schema converter apache#36796
loicalleyne added a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
to match apacheGH-36760: [Go] Adding avro ocf reader - schema converter apache#36796
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants