-
Notifications
You must be signed in to change notification settings - Fork 458
fix: Handle java.sql date types compatibility in Excel read and write operations
#719
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: main
Are you sure you want to change the base?
fix: Handle java.sql date types compatibility in Excel read and write operations
#719
Conversation
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 fixes a bug where java.sql.Date instances would throw an UnsupportedOperationException when passed to the WriteCellData constructor. The fix changes the date conversion from using toInstant() (which is not supported by java.sql.Date) to using getTime() (which works for both java.util.Date and java.sql.Date).
- Changed the
WriteCellDataDate constructor to useInstant.ofEpochMilli(dateValue.getTime())instead ofdateValue.toInstant() - Added comprehensive test coverage for
java.sql.Datecompatibility - Updated date format in test data classes from Chinese format to standard ISO format for consistency
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
fesod/src/main/java/org/apache/fesod/sheet/metadata/data/WriteCellData.java |
Fixed the Date constructor to handle both java.util.Date and java.sql.Date by using getTime() method instead of toInstant() |
fesod/src/test/java/org/apache/fesod/sheet/celldata/CellDataWriteData.java |
Added sqlDate field and updated date format to ISO format for testing |
fesod/src/test/java/org/apache/fesod/sheet/celldata/CellDataReadData.java |
Added corresponding sqlDate field for reading test data |
fesod/src/test/java/org/apache/fesod/sheet/celldata/CellDataDataTest.java |
Added test case that creates WriteCellData with java.sql.Date to verify the fix |
fesod/src/test/java/org/apache/fesod/sheet/celldata/CellDataDataListener.java |
Added assertion to verify java.sql.Date handling works correctly |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setType(CellDataTypeEnum.DATE); | ||
| this.dateValue = LocalDateTime.ofInstant(dateValue.toInstant(), ZoneId.systemDefault()); | ||
| // Use getTime() which works for both java.util.Date and java.sql.Date | ||
| this.dateValue = LocalDateTime.ofInstant(Instant.ofEpochMilli(dateValue.getTime()), ZoneId.systemDefault()); |
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.
Using the Date#getTime() incurs a loss of precision, as it only provides millisecond accuracy. Furthermore, might we consider supporting java.sql.Timestamp? I believe the unit test examples could be improved.
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.
Thank you for the feedback! I'll look into this and explore supporting java.sql.Timestamp as well. Will improve the test examples too.
java.sql.Date compatibility in WriteCellDatajava.sql date types compatibility in Excel read and write operations
…heng/fesod into fix/handle-sql-date-compatibility
Closed: #714
Purpose of the pull request
This PR fixes a bug where using
java.sql.DatewithWriteCellDataconstructor throws anUnsupportedOperationException. The issue occurs because the original implementation callstoInstant()method which is not supported byjava.sql.Date.What's changed?
The
WriteCellDataDate constructor now usesInstant.ofEpochMilli(dateValue.getTime())instead ofdateValue.toInstant()as thegetTime()method works for bothjava.util.Date and java.sql.Date.Checklist