-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Document Calendar ORC metadata limitation #26874
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
base: master
Are you sure you want to change the base?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR enhances the ORC file-formats documentation by adding a known limitation about Trino’s ORC reader treating dates and timestamps using the Proleptic Gregorian Calendar due to missing calendar metadata support. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `docs/src/main/sphinx/object-storage/file-formats.md:35` </location>
<code_context>
configured.
+Known limitations:
+Currently, Trino does not support Calendar entry in Orc file metadata. As a results Trino always treats dates and timestamps as values written using Proleptic Gregorian Calendar. This causes incorrect reads of legacy dates and timestamps that were written using Hybrid Calendar.
+
(parquet-format-configuration)=
</code_context>
<issue_to_address>
**issue (typo):** Typo: 'As a results' should be 'As a result'.
Please update the phrase for correct grammar.
```suggestion
Currently, Trino does not support Calendar entry in Orc file metadata. As a result Trino always treats dates and timestamps as values written using Proleptic Gregorian Calendar. This causes incorrect reads of legacy dates and timestamps that were written using Hybrid Calendar.
```
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
014b2dd
to
d04570d
Compare
d04570d
to
0b4f585
Compare
4f4b7d1
to
0d7cfd3
Compare
@mosabua can you take a look? |
Limitations: | ||
[Trino ignores Calendar entry in ORC file metadata.](https://github.com/trinodb/trino/issues/26865) | ||
As a result Trino always treats dates and timestamps as values written using | ||
proleptic Gregorian calendar. This causes incorrect values read when reading |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proleptic Gregorian calendar. This causes incorrect values read when reading | |
Gregorian calendar. This causes incorrect values when reading |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
proleptic is an important word, we should keep it
-
"This causes incorrect values when reading" reads as missing something.
Incorrect values where? does the file gets broken, changed or malformed when we try to read it? or only the values reconstructed in computer memory are wrong?
[Trino ignores Calendar entry in ORC file metadata.](https://github.com/trinodb/trino/issues/26865) | ||
As a result Trino always treats dates and timestamps as values written using | ||
proleptic Gregorian calendar. This causes incorrect values read when reading | ||
date/time values before Oct 15, 1582 that were written using hybrid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
date/time values before Oct 15, 1582 that were written using hybrid | |
values before Oct 15, 1582 that were written using hybrid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove "date/time", but then we need to indicate which types does this apply to.
@mosabua how would you want to indicate this?
Description
The follow up of the #26507
It's related to the #26865
Additional context and related issues
Release notes
(x ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:
Summary by Sourcery
Documentation: