Skip to content

Conversation

@shivendra-dev54
Copy link
Contributor

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 DATE and TIMESTAMP entries to the DataType enum
Updated 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.

@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2025

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.09%. Comparing base (cf728bf) to head (21a8c7f).

Files with missing lines Patch % Lines
...in/java/org/apache/graphar/info/type/DataType.java 60.00% 2 Missing ⚠️
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              
Flag Coverage Δ
java-info 84.71% <60.00%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yangxk1
Copy link
Contributor

yangxk1 commented Dec 18, 2025

@yangxk1
Copy link
Contributor

yangxk1 commented Dec 22, 2025

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 info part of this PR

@shivendra-dev54
Copy link
Contributor Author

I have added new tests for the new data types

Copy link

Copilot AI left a 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.

@yangxk1
Copy link
Contributor

yangxk1 commented Dec 25, 2025

In order to be rigorous enough, I think we also need to verify whether yaml text can be parsed through testdata.

@shivendra-dev54
Copy link
Contributor Author

Thanks for pointing out the test data.
I cloned incubator-graphar-testing, set GAR_TEST_DATA accordingly, and ran the info module tests locally.
All tests passed successfully.
@yangxk1

Copy link
Contributor

@yangxk1 yangxk1 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you~

@yangxk1 yangxk1 merged commit bac3630 into apache:main Dec 29, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(java,info): support timestamp and date in dataType

3 participants