You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "emilk (via GitHub)" <gi...@apache.org> on 2026/08/06 12:55:16 UTC
[PR] MINOR: [Format] Use `///` for doc comments so they reach generated code [arrow]
emilk opened a new pull request, #50826:
URL: https://github.com/apache/arrow/pull/50826
### Rationale for this change
`flatc` only propagates `///` comments into generated bindings. A few descriptions in `Message.fbs` and `Schema.fbs` use plain `//`, so they are silently dropped and never reach downstream users.
For example `CompressionType::LZ4_FRAME` has a helpful note about frame vs. raw/block format that does not appear in the generated Rust in arrow-rs. The `custom_metadata` field is documented on `Field` and `Footer` (both `///`) but not on `Schema` (`//`).
### What changes are included in this PR?
Changed `//` to `///` for five comment blocks that describe a declaration:
| File | Declaration |
|-------------|--------------------------------|
| Message.fbs | `CompressionType.LZ4_FRAME` |
| Message.fbs | `CompressionType.ZSTD` |
| Schema.fbs | `IntervalUnit` |
| Schema.fbs | `Duration` |
| Schema.fbs | `Schema.custom_metadata` |
The `IntervalUnit` block sat *after* the enum, so `///` in place would have attached it to `table Interval` instead. It is moved above `enum IntervalUnit`, which is what it describes.
Comments only. No schema, wire format, or field numbering changes.
### Are these changes tested?
Generated Rust with `flatc 23.5.26` before and after and diffed. The only differences are the five added doc comment blocks.
### Are there any user-facing changes?
Yes, in the good sense: these descriptions now show up in generated bindings for all languages.
---
Disclosure per the [AI-generated code guidance](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code): I used Claude Code to find the affected comments and verify the generated-output diff. I reviewed every line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: [Format] Fix docstrings in .fbs [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #50826:
URL: https://github.com/apache/arrow/pull/50826
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: [Format] Fix docstrinngs in .fbs [arrow]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #50826:
URL: https://github.com/apache/arrow/pull/50826#issuecomment-5244419005
LGTM, thank you @emilk!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: [Format] Fix docstrinngs in .fbs [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #50826:
URL: https://github.com/apache/arrow/pull/50826#discussion_r3729234853
##########
format/Schema.fbs:
##########
@@ -395,42 +395,50 @@ table Timestamp {
timezone: string;
}
-enum IntervalUnit: short { YEAR_MONTH, DAY_TIME, MONTH_DAY_NANO}
-// A "calendar" interval which models types that don't necessarily
-// have a precise duration without the context of a base timestamp (e.g.
-// days can differ in length during day light savings time transitions).
-// All integers in the types below are stored in the endianness indicated
-// by the schema.
-//
-// YEAR_MONTH - Indicates the number of elapsed whole months, stored as
-// 4-byte signed integers.
-// DAY_TIME - Indicates the number of elapsed days and milliseconds (no leap seconds),
-// stored as 2 contiguous 32-bit signed integers (8-bytes in total). Support
-// of this IntervalUnit is not required for full arrow compatibility.
-// MONTH_DAY_NANO - A triple of the number of elapsed months, days, and nanoseconds.
-// The values are stored contiguously in 16-byte blocks. Months and days are
-// encoded as 32-bit signed integers and nanoseconds is encoded as a 64-bit
-// signed integer. Nanoseconds does not allow for leap seconds. Each field is
-// independent (e.g. there is no constraint that nanoseconds have the same
-// sign as days or that the quantity of nanoseconds represents less than a
-// day's worth of time).
+/// The unit of an Interval.
+///
+/// All integers in the units below are stored in the endianness indicated
+/// by the schema.
+enum IntervalUnit: short {
+ /// Indicates the number of elapsed whole months, stored as
+ /// 4-byte signed integers.
+ YEAR_MONTH,
+
+ /// Indicates the number of elapsed days and milliseconds (no leap seconds),
+ /// stored as 2 contiguous 32-bit signed integers (8-bytes in total). Support
+ /// of this IntervalUnit is not required for full arrow compatibility.
Review Comment:
In generated docs this will now read "full arrow compatibility"; Arrow is a proper noun and is capitalized elsewhere in the format docs.
This issue also appears in the following locations of the same file:
- line 422
- line 439
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: [Format] Fix docstrings in .fbs [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50826:
URL: https://github.com/apache/arrow/pull/50826#discussion_r3850688785
##########
format/Schema.fbs:
##########
@@ -395,42 +395,50 @@ table Timestamp {
timezone: string;
}
-enum IntervalUnit: short { YEAR_MONTH, DAY_TIME, MONTH_DAY_NANO}
-// A "calendar" interval which models types that don't necessarily
-// have a precise duration without the context of a base timestamp (e.g.
-// days can differ in length during day light savings time transitions).
-// All integers in the types below are stored in the endianness indicated
-// by the schema.
-//
-// YEAR_MONTH - Indicates the number of elapsed whole months, stored as
-// 4-byte signed integers.
-// DAY_TIME - Indicates the number of elapsed days and milliseconds (no leap seconds),
-// stored as 2 contiguous 32-bit signed integers (8-bytes in total). Support
-// of this IntervalUnit is not required for full arrow compatibility.
-// MONTH_DAY_NANO - A triple of the number of elapsed months, days, and nanoseconds.
-// The values are stored contiguously in 16-byte blocks. Months and days are
-// encoded as 32-bit signed integers and nanoseconds is encoded as a 64-bit
-// signed integer. Nanoseconds does not allow for leap seconds. Each field is
-// independent (e.g. there is no constraint that nanoseconds have the same
-// sign as days or that the quantity of nanoseconds represents less than a
-// day's worth of time).
+/// The unit of an Interval.
+///
+/// All integers in the units below are stored in the endianness indicated
+/// by the schema.
+enum IntervalUnit: short {
+ /// Indicates the number of elapsed whole months, stored as
+ /// 4-byte signed integers.
+ YEAR_MONTH,
+
+ /// Indicates the number of elapsed days and milliseconds (no leap seconds),
+ /// stored as 2 contiguous 32-bit signed integers (8-bytes in total). Support
+ /// of this IntervalUnit is not required for full Arrow compatibility.
+ DAY_TIME,
+
+ /// A triple of the number of elapsed months, days, and nanoseconds.
+ /// The values are stored contiguously in 16-byte blocks. Months and days are
+ /// encoded as 32-bit signed integers and nanoseconds is encoded as a 64-bit
+ /// signed integer. Nanoseconds does not allow for leap seconds. Each field is
+ /// independent (e.g. there is no constraint that nanoseconds have the same
+ /// sign as days or that the quantity of nanoseconds represents less than a
+ /// day's worth of time).
+ MONTH_DAY_NANO
+}
+
+/// A "calendar" interval which models types that don't necessarily
+/// have a precise duration without the context of a base timestamp (e.g.
+/// days can differ in length during day light savings time transitions).
table Interval {
unit: IntervalUnit;
}
-// An absolute length of time unrelated to any calendar artifacts.
-//
-// For the purposes of Arrow Implementations, adding this value to a Timestamp
-// ("t1") naively (i.e. simply summing the two numbers) is acceptable even
-// though in some cases the resulting Timestamp (t2) would not account for
-// leap-seconds during the elapsed time between "t1" and "t2". Similarly,
-// representing the difference between two Unix timestamps is acceptable, but
-// would yield a value that is possibly a few seconds off from the true elapsed
-// time.
-//
-// The resolution defaults to millisecond, but can be any of the other
-// supported TimeUnit values as with Timestamp and Time types. This type is
-// always represented as an 8-byte integer.
+/// An absolute length of time unrelated to any calendar artifacts.
+///
+/// For the purposes of Arrow Implementations, adding this value to a Timestamp
+/// ("t1") naively (i.e. simply summing the two numbers) is acceptable even
+/// though in some cases the resulting Timestamp (t2) would not account for
+/// leap-seconds during the elapsed time between "t1" and "t2". Similarly,
+/// representing the difference between two Unix timestamps is acceptable, but
+/// would yield a value that is possibly a few seconds off from the true elapsed
+/// time.
+///
+/// The resolution defaults to millisecond, but can be any of the other
+/// supported TimeUnit values as with Timestamp and Time types. This type is
+/// always represented as an 8-byte integer.
Review Comment:
Let's remove stray spaces.
```suggestion
/// The resolution defaults to millisecond, but can be any of the other
/// supported TimeUnit values as with Timestamp and Time types. This type is
/// always represented as an 8-byte integer.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: [Format] Fix docstrings in .fbs [arrow]
Posted by "emilk (via GitHub)" <gi...@apache.org>.
emilk commented on PR #50826:
URL: https://github.com/apache/arrow/pull/50826#issuecomment-5393558449
need approval to run ci
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org