Skip to content

Fix typos, grammar, and comment inconsistencies in parquet.thrift#573

Merged
wgtmac merged 3 commits into
apache:masterfrom
iemejia:fix/thrift-comments
Jun 8, 2026
Merged

Fix typos, grammar, and comment inconsistencies in parquet.thrift#573
wgtmac merged 3 commits into
apache:masterfrom
iemejia:fix/thrift-comments

Conversation

@iemejia
Copy link
Copy Markdown
Member

@iemejia iemejia commented Jun 2, 2026

Summary

Fix typos, grammar, and comment inconsistencies in the canonical Thrift schema definition.

Changes

  • Fix typos: "to be be", "documention", "not necessary"
  • Remove off-by-one in DataPageHeaderV2 is_compressed comment
  • Fix article agreement ("a element" -> "an element", "a OffsetIndex" -> "an OffsetIndex")
  • Disambiguate compressed_page_size comment in PageLocation (it includes the header; the field of the same name on PageHeader does not)
  • Fix "edges interpolation" -> "edge interpolation" in Geospatial comments
  • Capitalize proper nouns: Hive, Pig; normalize GZIP casing
  • Add terminal periods for consistency
  • Clarify BIT_PACKED is superseded by RLE (cross-reference Encodings.md)
  • Missing space before parenthesis in frameworks list

Validation

Thrift definition compiles cleanly after all changes. No semantic/behavioral changes to the format specification.

Split from #572 for easier review.

Copy link
Copy Markdown
Contributor

@etseidl etseidl left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on. Just a few minor nits.

Comment thread src/main/thrift/parquet.thrift Outdated
Comment thread src/main/thrift/parquet.thrift Outdated
Comment thread src/main/thrift/parquet.thrift Outdated
Copy link
Copy Markdown
Contributor

@etseidl etseidl left a comment

Choose a reason for hiding this comment

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

Thanks. Just a few more "edges" to clean up.

Comment thread src/main/thrift/parquet.thrift Outdated
Comment thread src/main/thrift/parquet.thrift Outdated
Copy link
Copy Markdown
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @iemejia and @etseidl

iemejia added 3 commits June 5, 2026 15:41
- Fix typos: "to be be", "documention", "not necessary"
- Remove off-by-one in DataPageHeaderV2 is_compressed comment
- Fix article agreement ("a element" -> "an element", "a OffsetIndex" -> "an OffsetIndex")
- Disambiguate compressed_page_size comment in PageLocation
- Fix "edges interpolation" -> "edge interpolation" in Geospatial comments
- Capitalize proper nouns: Hive, Pig; normalize GZIP casing
- Add terminal periods for consistency
- Clarify BIT_PACKED is superseded by RLE (cross-reference Encodings.md)
- Missing space before parenthesis in frameworks list

Thrift validation passes after these edits.
@iemejia iemejia force-pushed the fix/thrift-comments branch from f46fd06 to 11b1649 Compare June 5, 2026 13:42
@alamb
Copy link
Copy Markdown
Contributor

alamb commented Jun 5, 2026

(BTW force pushing makes it harder to review what has changed since this was last reviewed / approved)

@iemejia
Copy link
Copy Markdown
Member Author

iemejia commented Jun 5, 2026

@alamb My excuses for the rebase. I had an odd conflict locally so I ended up 'rebasing' (in this and the other PR)

@alamb
Copy link
Copy Markdown
Contributor

alamb commented Jun 5, 2026

No worries-- I just thought I would mention it

@wgtmac wgtmac merged commit c4b3ef2 into apache:master Jun 8, 2026
4 checks passed
@wgtmac
Copy link
Copy Markdown
Member

wgtmac commented Jun 8, 2026

Thanks @iemejia for improving this and @alamb @etseidl @Fokko for the review!

@iemejia iemejia deleted the fix/thrift-comments branch June 8, 2026 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants