-
Notifications
You must be signed in to change notification settings - Fork 177
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
feat: Define schema post order visitor. #25
Conversation
I'm working on more tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small comments, but looks great! Thanks @liurenjie1024 for working on this
@@ -28,12 +28,14 @@ keywords = ["iceberg"] | |||
|
|||
[dependencies] | |||
apache-avro = "0.15.0" | |||
serde = "^1.0" | |||
serde = {version = "^1.0", features = ["rc"]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also like to live dangerous :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't get your point here 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I'm sorry. It looks like you're pulling in Release Candidates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. feature flag in rust is a tool to enable condition compilation, and 'rc' here means generating see/de code for 'Arc', and the explanation is here: https://stackoverflow.com/questions/49312600/how-do-i-serialize-or-deserialize-an-arct-in-serde.
I don't know similar concepts in java/python.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't know that. Thanks!
} | ||
} | ||
|
||
impl Display for Schema { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although I can't say that much to the visitor pattern. Thanks for working on this!
1167203
to
6af0393
Compare
6af0393
to
cd2e320
Compare
cc @Fokko Any other comments? |
More tests are following.