Make format version pages data driven#189
Conversation
| * [polars](https://github.com/pola-rs/polars) (Rust) | ||
|
|
||
| <!-- Status source in data/implementations --> | ||
| <!-- Data driven table, see /layouts/shortcodes --> |
There was a problem hiding this comment.
I added a readme and new link in layouts/shortcodes
|
|
||
| Forward incompatible features and the format version each became available in: | ||
|
|
||
| | Feature | Released in | Source | Notes | |
There was a problem hiding this comment.
The point of the PR is to automatically derive this table from the .yml files
| - id: compression-uncompressed | ||
| display_name: UNCOMPRESSED | ||
|
|
||
| compatibility: forward_incompatible |
There was a problem hiding this comment.
I added the various fields needed for the version page to the existing data files. I could probably make this less verbose by automatically generating the urls, but I think this is clearer and easier to understand
| - id: encoding-byte-stream-split-extended | ||
| display_name: BYTE_STREAM_SPLIT (Additional Types) | ||
| spec_url: https://github.com/apache/parquet-format/commit/e517ac4dbe08d518eb5c2e58576d4c711973db94 | ||
| format_version: |
There was a problem hiding this comment.
some features already had a "format_version" which was redundant with the information from the versions page, so I consolidated them into the same fields
From
format_version:
version: "2.11.0"
date: "2024-03-21"To
released_in: 2.11.0
release_date: '2024-03-21'There was a problem hiding this comment.
my one concern is 2.11.0, interpretted properly as a string without quote marks?
There was a problem hiding this comment.
Good idea -- I quoted them all in commit 9f8d58a
| @@ -0,0 +1,15 @@ | |||
| # Hugo shortcodes | |||
There was a problem hiding this comment.
I found the whole hugo rendering thing new, so I also left some small documentation
|
Thanks @alamb, this is neat! One thing occurred to me while verifying external links...it would be nice for the features to link to their specific PR. The current "Source" column links to all changes between versions, so there's a lot of housekeeping changes to wade through. Not for now, but as a follow-on. |
| display_name: 8, 16, 32, 64 bit signed and unsigned INT | ||
|
|
||
| version_display_name: Signed and unsigned integer logical types (INT32, INT64) | ||
| compatibility: forward_compatible |
There was a problem hiding this comment.
I think this is incompatible?
There was a problem hiding this comment.
negative physical values are interpetted incorrectly?
There was a problem hiding this comment.
I think we've agreed that new logical types are forward compatible.
New logical types are considered forward compatible despite the loss of semantic meaning.
There was a problem hiding this comment.
Yeah, this is broader discussion, I think we should clarify the wording here. we can leave it as is, but I worry about the difference between different semantics:
- Losing information that a int64 represents a timestamp is fine in my mind. It still looks like an integer, and following arithmetic properties.
- Losing information that an int64 is actually a uint64 feels like a data corruption case.
But I'd have to clarify my thinking here a bit more. We don't need to make the change here.
| - id: logical-decimal-int64 | ||
| display_name: DECIMAL (INT64) | ||
|
|
||
| compatibility: forward_compatible |
There was a problem hiding this comment.
technically incompatible. numbers cannot be understood properly without scale?
There was a problem hiding this comment.
Same comment as #189 (comment) applies, not a blocker.
| {{- $featureUrl = $feature.spec_url -}} | ||
| {{- end -}} | ||
| {{- $releasedIn := printf "%v" $feature.released_in -}} | ||
| {{- $legacyReleaseTags := slice "1.0.0" "2.0.0" "2.1.0" -}} |
There was a problem hiding this comment.
what does the slice do here?
There was a problem hiding this comment.
Apparently in hugo makes an array -- this is part of some overly complicated logic to translate release names like 1.0.0 into tag names (whose format changed over time)
I simplified the whole thing by changing this to a lookup table -- which simplifies this logic and I think makes it clearer what is going on
| <tbody> | ||
| {{- range $features -}} | ||
| {{- /* Include rows by default; skip rows explicitly marked implementation_status: false. */ -}} | ||
| {{- if ne .implementation_status false -}} |
There was a problem hiding this comment.
Thank you for checking this.
emkornfield
left a comment
There was a problem hiding this comment.
Overall, looks good to me, I think some of the logical changes are actually forward incompatible but that might be a broader conversation on what exactly is compatible/incompatible with them.
Thank you for doing this.
alamb
left a comment
There was a problem hiding this comment.
Thank you @etseidl and @emkornfield
I reworked the rendering to
- use a table for each parquet format release to avoid fancy rendering logic
- restore a Notes column
- quoted the version numbers
I have also re-generated the rendered preview https://alamb.github.io/parquet-site/docs/file-format/versions/
I think there are two follow ons here:
- Figure out what to do with the features that are in parquet-format but don't have entry on implementation status: https://github.com/apache/parquet-site/pull/189/changes#r3470611445
- @etseidl 's suggestion to research the approving PRs #189 (comment) (I'll do this as a follow on PR)
Stuff I am not really sure how to proceed:
- @emkornfield 's comment in https://github.com/apache/parquet-site/pull/189/changes#r3470700287 about what actually constitutes "forward compatible" -- I think this is a larger consensus building exercise so I would like to not address it in this PR
| {{- $featureUrl = $feature.spec_url -}} | ||
| {{- end -}} | ||
| {{- $releasedIn := printf "%v" $feature.released_in -}} | ||
| {{- $legacyReleaseTags := slice "1.0.0" "2.0.0" "2.1.0" -}} |
There was a problem hiding this comment.
Apparently in hugo makes an array -- this is part of some overly complicated logic to translate release names like 1.0.0 into tag names (whose format changed over time)
I simplified the whole thing by changing this to a lookup table -- which simplifies this logic and I think makes it clearer what is going on
| - id: encoding-byte-stream-split-extended | ||
| display_name: BYTE_STREAM_SPLIT (Additional Types) | ||
| spec_url: https://github.com/apache/parquet-format/commit/e517ac4dbe08d518eb5c2e58576d4c711973db94 | ||
| format_version: |
There was a problem hiding this comment.
Good idea -- I quoted them all in commit 9f8d58a
Honestly I think most of these are so old, that it doesn't necessary add value, other then maybe providing a note on that page? Is there anything worth surfacing? Looking more closely, these seem to be the modern ones:
Yeah, I have a feeling this might come up in the context of versioning conversations no need to block. |
I will try and address it in a follow on PR |
I certainly didn't mean to task you 😉. I'd be happy to compile the data...I'd just need rendering help. |
Oh no problem -- I'll send an agent to do it and then verify. Your help will be most useful in helping to review it as well |
|
I think I have addressed all feedback in this PR -- so I will plan to merge it in tomorrow unless people would like more time or have other comments. I'll also file tickets to track follow on work |
|
Tracking in |
|
@etseidl here is a PR to add the PR research |
Follow on to
Rendered Preview:
Rationale
Implements @emkornfield 's request in #186 (comment)
Among other things, rendering the page using data files forces it to remain consistent with the implementation status page.
Changes
Updates the Parquet format version and implementation status pages to generate their feature tables from structured data under data/implementations.
Planned follow on
Making this change already shows value -- some features only on the versions page but not the implementation status page
This PR simply ignores rows that have
implementation_status: falseon the implementation status pageAs a follow on PR, I will propose adding the new features to the implementation status page, but this PR is already quite large
Examples: