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

Update docs for datatypes #5260

Merged
merged 4 commits into from
Dec 31, 2023
Merged

Conversation

Jefffrey
Copy link
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

Update docs for datatypes

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Dec 30, 2023
@@ -23,14 +23,18 @@ use crate::{Field, FieldRef, Fields, UnionFields};
/// The set of datatypes that are supported by this implementation of Apache Arrow.
///
/// The Arrow specification on data types includes some more types.
/// See also [`Schema.fbs`](https://github.com/apache/arrow/blob/master/format/Schema.fbs)
/// See also [`Schema.fbs`](https://github.com/apache/arrow/blob/main/format/Schema.fbs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current link works but shows popup everytime:

image

So just change to main

Comment on lines 154 to 158
/// Must be either seconds or millieseconds.
Time32(TimeUnit),
/// A 64-bit time representing the elapsed time since midnight in the unit of `TimeUnit`.
/// Must be either microseconds or nanoseconds.
Time64(TimeUnit),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Want to be explicit here as in isolation it can imply Time64 could take on seconds/milliseconds and Time32 could take on microseconds/nanoseconds based on TimeUnit enum alone

Copy link
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 @Jefffrey ❤️

There is one typo but otherwise this PR looks good to go to me

/// for Arrow's specification.
///
/// The variants of this enum include primitive fixed size types as well as parametric or
/// nested types.
/// Currently the Rust implementation supports the following nested types:
/// Currently the Rust implementation supports the following nested types:
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

arrow-schema/src/datatype.rs Outdated Show resolved Hide resolved
@tustvold tustvold merged commit b00f4e0 into apache:master Dec 31, 2023
25 checks passed
@Jefffrey Jefffrey deleted the datatype_doc_update branch December 31, 2023 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants