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

Raise exception when reading non-UTC adjusted timestamps in parquet #4421

Merged
merged 13 commits into from
Sep 19, 2023

Conversation

malhotrashivam
Copy link
Contributor

@malhotrashivam malhotrashivam commented Aug 31, 2023

Closes #3588
Related to #976

Also moved all parquet files which were stored in the core repo to Git LFS.
Interesting links about Git LFS

@malhotrashivam malhotrashivam self-assigned this Aug 31, 2023
@malhotrashivam malhotrashivam added bug Something isn't working parquet Related to the Parquet integration NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. labels Aug 31, 2023
@malhotrashivam malhotrashivam added this to the August 2023 milestone Aug 31, 2023
chipkent
chipkent previously approved these changes Aug 31, 2023
rcaudy
rcaudy previously approved these changes Sep 15, 2023
Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

Workflow updates look fine to me. I'm happy to take an lfs: true approach for all workflow files as you have done here - the advantage is it's consistent across all workflows. The other approach is to just use lfs: true for the tests - the advantage is that non-test workflows won't need to download the LFS files. I'm indifferent, at least until a point in time where we have a significant LFS download cost. I'll defer to other reviewers.

@rcaudy
Copy link
Member

rcaudy commented Sep 18, 2023

Workflow updates look fine to me. I'm happy to take an lfs: true approach for all workflow files as you have done here - the advantage is it's consistent across all workflows. The other approach is to just use lfs: true for the tests - the advantage is that non-test workflows won't need to download the LFS files. I'm indifferent, at least until a point in time where we have a significant LFS download cost. I'll defer to other reviewers.

Which approach do you think is best, @devinrsmith ? I wonder if we should start from a principal of "good hygiene" and only fetch lfs files for test workflows. Or do we expect to need some in, say, publishing or documentation workflows?

@devinrsmith
Copy link
Member

I'm fine proceeding w/ a principal of "good hygiene"

@malhotrashivam
Copy link
Contributor Author

I'm fine proceeding w/ a principal of "good hygiene"

Cool, I have reverted back the change to just Check CI and Nightly Check CI jobs.

@malhotrashivam malhotrashivam merged commit 59921bc into deephaven:main Sep 19, 2023
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. parquet Related to the Parquet integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to read parquet TimestampLogicalTypeAnnotation that is not adjusted to UTC
4 participants