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

@iemejia iemejia commented Jun 2, 2026

Copy link
Copy Markdown
Member

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.

@etseidl etseidl left a comment

Copy link
Copy Markdown
Contributor

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

@etseidl etseidl left a comment

Copy link
Copy Markdown
Contributor

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

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

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

alamb commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

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

@iemejia

iemejia commented Jun 5, 2026

Copy link
Copy Markdown
Member Author

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

@alamb

alamb commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

No worries-- I just thought I would mention it

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

wgtmac commented Jun 8, 2026

Copy link
Copy Markdown
Member

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