-
Notifications
You must be signed in to change notification settings - Fork 868
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
Print Parquet BasicTypeInfo id when present #7094
Conversation
devinrsmith
commented
Feb 7, 2025
•
edited by alamb
Loading
edited by alamb
- Closes [parquet] Print Parquet BasicTypeInfo id when present #7081
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.
Thanks @devinrsmith -- looks good to me
Any chance you can also improve documentation of print_schema
with an example of the output too? 🙏 (not needed to merge this PR, just a request)
So for example, update https://arrow.apache.org/rust/parquet/schema/printer/fn.print_schema.html with the output of your test:
message schema [2] {
OPTIONAL group field [1] {
REQUIRED INT32 f1 [0] (INT_32);
OPTIONAL BYTE_ARRAY f2 [1] (UTF8);
OPTIONAL BYTE_ARRAY f3 [1] (STRING);
}
REPEATED FIXED_LEN_BYTE_ARRAY (12) f4 [2] (INTERVAL);
}";
.build(); | ||
let f3 = Type::primitive_type_builder("f3", PhysicalType::BYTE_ARRAY) | ||
.with_logical_type(Some(LogicalType::String)) | ||
.with_id(Some(1)) |
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.
is it a problem that this is the same id as f2?
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.
Parquet itself treats the id
s as metadata, it does not care about them. Higher level systems that map their own schemas into parquet schemas (such as Iceberg) may have stricter requirements and care.
.unwrap(); | ||
p.print(&message); | ||
} | ||
let expected = "message schema [2] { |
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.
Thanks @alamb for the quick review!
I can add that into this PR, no problem; I'll have that out to you late today. |
/// | ||
/// outputs | ||
/// | ||
/// ```text |
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.
❤️
Thanks again @devinrsmith ❤️ |
* Print Parquet BasicTypeInfo id when present * Improve print_schema documentation * tiny cleanup