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

MINOR: Add FIXED_LEN_BYTE_ARRAY Type #190

Merged
merged 3 commits into from Mar 2, 2023
Merged

Conversation

XinyuZeng
Copy link
Contributor

Add missing FIXED_LEN_BYTE_ARRAY type under Types in README.md

@mapleFU
Copy link
Member

mapleFU commented Feb 8, 2023

@pitrou I think this is great, mind take a look?

README.md Outdated
@@ -132,6 +132,7 @@ readers and writers for the format. The types are:
- FLOAT: IEEE 32-bit floating point values
- DOUBLE: IEEE 64-bit floating point values
- BYTE_ARRAY: arbitrarily long byte arrays.
- FIXED_LEN_BYTE_ARRAY: fixed length byte arrays.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this!

It is weird that only BYTE_ARRAY and FIXED_LEN_BYTE_ARRAY end with a period. Better to remove them off for consistency.

Should we also mark INT96 as deprecated? cc @gszadovszky @shangxinli

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, sure, we can mark INT96 as deprecated.
It is unfortunate that we have documentation/specifications in multiple places and we usually forget about them. E.g. parquet.apache.org still not contain FIXED_LEN_BYTE_ARRAY. We might create a jira about clearing up these. Even referencing the parquet-format repo from the apache site or generating the content from it?

Copy link
Member

Choose a reason for hiding this comment

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

Sure. Give me some time to learn how the site works first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that a single source of truth would be much more beneficial to first-time readers :)

@wgtmac wgtmac changed the title Minor: add FIXED_LEN_BYTE_ARRAY under Types in doc MINOR: Add FIXED_LEN_BYTE_ARRAY Type Mar 2, 2023
Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @XinyuZeng

@wgtmac wgtmac merged commit 2e35a9f into apache:master Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants