-
Notifications
You must be signed in to change notification settings - Fork 86
feat(java): Add DATE and TIMESTAMP support to DataType enum #812
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
feat(java): Add DATE and TIMESTAMP support to DataType enum #812
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #812 +/- ##
============================================
- Coverage 77.10% 77.09% -0.02%
Complexity 607 607
============================================
Files 84 84
Lines 8912 8916 +4
Branches 1045 1045
============================================
+ Hits 6872 6874 +2
- Misses 1800 1802 +2
Partials 240 240
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
please add more unit test, testing dataset: https://github.com/apache/incubator-graphar-testing/blob/8286724cd898047de833f0304dbddd2831afdf9f/ldbc_sample/parquet/person_knows-timestamp_person.edge.yml |
|
I think what we need to do is add unit tests with reference to the test data instead of modifying the test data. ref:#754, Just pay attention to the |
|
I have added new tests for the new data types |
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.
Pull request overview
This PR adds support for DATE and TIMESTAMP data types to the Java implementation of GraphAr by extending the DataType enum. This enables users to define schemas that reference these logical types, addressing issue #771.
- Added DATE and TIMESTAMP enum values to the DataType enum
- Updated the
fromString()method to parse "date" and "timestamp" strings - Added comprehensive test coverage for the new data types
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| maven-projects/info/src/main/java/org/apache/graphar/info/type/DataType.java | Added DATE and TIMESTAMP enum values with documentation and updated fromString() to parse these types |
| maven-projects/info/src/test/java/org/apache/graphar/info/PropertyTest.java | Extended testPropertyWithAllDataTypes() to verify DATE and TIMESTAMP properties work correctly |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
In order to be rigorous enough, I think we also need to verify whether yaml text can be parsed through testdata. |
|
Thanks for pointing out the test data. |
yangxk1
left a comment
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.
LGTM! Thank you~
Fixes #771
Reason for this PR
This change adds support for date and timestamp data types in the DataType enum.
This is required to support schema definitions that already reference these logical types and is discussed in issue #771.
What changes are included in this PR?
Added
DATEandTIMESTAMPentries to theDataTypeenumUpdated string parsing logic to recognize date and timestamp
Are these changes tested?
No new tests are added in this PR.
This PR is intentionally scoped to the enum change only.
Test coverage for loading and saving date and timestamp types will be added in a follow-up change, as mentioned in the issue.
Are there any user-facing changes?
This enables users to define date and timestamp types in schema YAML files, but does not change existing behavior.