feat: add java.sql.Timestamp converters by SyedIshmumAhnaf · Pull Request #1019 · apache/fesod

SyedIshmumAhnaf · 2026-08-17T19:12:15Z

Purpose of the pull request

Related: #1017

Add first-class converter support for java.sql.Timestamp, following the existing temporal converter patterns in Apache Fesod.

What's changed?

Added TimestampDateConverter as the default write converter for Timestamp.
Added TimestampNumberConverter for Excel numeric date conversion, including use1904windowing handling.
Added TimestampStringConverter using the existing DateUtils parsing/formatting behavior and @DateTimeFormat support.
Registered the new converters in DefaultConverterLoader using the same read/write structure as LocalDateTime.
Added TimestampConverterTest covering:
- converter support keys
- number read/write behavior
- 1904 date windowing
- string parsing/formatting
- custom date-time format handling
- date write behavior
- default converter registration

The change is intentionally scoped to java.sql.Timestamp and does not modify the converter lookup mechanism or other converter types.

Verification:

./mvnw spotless:check
./mvnw -pl fesod-sheet -Dmaven.test.skip=false -Dtest=TimestampConverterTest,ConverterTest,ConverterDataTest,CustomConverterTest test
- 68 tests passed
CI-equivalent module build with tests enabled passed successfully.

Checklist

I have read the Contributor Guide.
I have written the necessary doc or comment.
I have added the necessary unit tests and all cases have passed.

Copilot

Pull request overview

Adds first-class converter support for java.sql.Timestamp in fesod-sheet, aligning with the existing temporal converter family and registering the converters in the default loader alongside unit coverage.

Changes:

Introduces TimestampDateConverter, TimestampNumberConverter, and TimestampStringConverter.
Registers the new converters in DefaultConverterLoader for read + default write behavior.
Adds TimestampConverterTest to validate key registration and basic read/write conversions (number/string/date) including 1904 windowing.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/TimestampConverterTest.java	Adds unit coverage for timestamp converters and default loader registration.
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/timestamp/TimestampStringConverter.java	Implements `Timestamp` ↔ `STRING` conversion using `DateUtils` formatting/parsing.
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/timestamp/TimestampNumberConverter.java	Implements `Timestamp` ↔ Excel numeric date conversion with 1904 windowing support.
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/timestamp/TimestampDateConverter.java	Implements default write conversion to `DATE` with appropriate cell format.
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/DefaultConverterLoader.java	Wires the new timestamp converters into the default converter registry.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+    @Override
+    public WriteCellData<?> convertToExcelData(
+            Timestamp value, ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration) {
+        if (contentProperty == null || contentProperty.getDateTimeFormatProperty() == null) {
+            return new WriteCellData<>(
+                    BigDecimal.valueOf(DateUtil.getExcelDate(value, globalConfiguration.getUse1904windowing())));
+        } else {
+            return new WriteCellData<>(BigDecimal.valueOf(DateUtil.getExcelDate(
+                    value, contentProperty.getDateTimeFormatProperty().getUse1904windowing())));
+        }
+    }


+    public WriteCellData<?> convertToExcelData(
+            Timestamp value, ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration)
+            throws Exception {
+        WriteCellData<?> cellData = new WriteCellData<>(value);


nkuprins · 2026-08-19T20:47:15Z

+        contentProperty.setDateTimeFormatProperty(dateTimeFormatProperty);
+
+        GlobalConfiguration globalConfiguration = new GlobalConfiguration();
+        globalConfiguration.setLocale(Locale.US);


The test still passes even if you remove the 129 line or replace it with Locale.JAPANESE or new Locale("th", "TH").

To actually check the locale, consider using a different format.
For example, "dd MMMM yyyy HH:mm:ss"="01 January 2020 01:01:01",
which now fails for Locale.GERMAN etc.

SyedIshmumAhnaf · 2026-08-21T09:01:28Z

Updated the test to use a locale-sensitive MMMM format with Locale.US, including January for both parsing and formatting. Thanks for catching that.

bengbengbalabalabeng · 2026-08-23T05:31:47Z

The newly added files in this PR are implemented from scratch and are not derived from Alibaba's EasyExcel. Therefore, no EasyExcel-related license header is required for these files.

Please refer to: https://github.com/apache/fesod/blob/main/fesod-sheet/src/main/java/org/apache/fesod/sheet/FesodSheet.java

nkuprins · 2026-09-16T12:20:46Z

Please resolve the conflicts from main

Mikkey-f · 2026-10-13T08:29:56Z

Hi @SyedIshmumAhnaf — checking in on this one. nkuprins asked for a rebase a month ago and it has been quiet since, so I wanted to ask before assuming anything.

Are you still planning to carry it forward? If you don't have the bandwidth for it right now, I'd be happy to pick it up — I'd rebase the three converters on current main, keep the implementation and tests, adjust to the current testkit conventions where needed, and reference this PR and #1017 in the description so your work stays credited. Either way, just let me know.

SyedIshmumAhnaf · 2026-10-13T20:00:27Z

Hi @Mikkey-f, sorry for being awol. I've been dealing with some things lately which has kept me off from GitHub and contributing. I hope to get back soon to the picture however I would be more than happy for you to continue working on this. Primarily because I am still working some things out and not sure of the timeline by when I might get my hands on this. If you have any questions regarding the existing work, please feel free to tag me and I'll try my very best to respond at the earliest.

Mikkey-f · 2026-10-14T10:46:36Z

Hi @SyedIshmumAhnaf — thanks for the kind words and for handing this over, and no worries at all; these things take the time they take. I'll carry this forward from here.

So you know what to expect: your commits will come along with your authorship preserved, brought up to date with main. As part of the carry-forward I'll address @nkuprins's locale-test point and the two Copilot notes, then open a fresh PR linked to #1017 that credits you as the original author and supersedes #1019. If a question comes up about the existing implementation I'll tag you here, and you're welcome to take a look at the carried-forward PR whenever you're back.

Nothing needed from you in the meantime — take care of things on your side first.

Mikkey-f · 2026-10-14T11:23:37Z

Carried forward as #1220 — your commits are preserved with your authorship, plus one adaptation commit to bring them up to date with main (details in the PR description). Thanks again for the hand-off!

Carry-forward of apache#1019 (original implementation by Syed Ishmum Ahnaf): - Resolve the 1904 windowing flag through the shared DateUtils.isDate1904 helper, like the rest of the date converter family. Reading DateTimeFormatProperty#getUse1904windowing() directly broke on the null DEFAULT preserved by apache#1042 and threw a NullPointerException whenever @DateTimeFormat was present without an explicit use1904windowing value. - Drop the EasyExcel derived license header from the four new files; they are written from scratch, so the plain ASF header applies. - Align with current conventions: Class<?> supportJavaTypeKey, single-line javadoc, and tests moved to the mirrored converters.timestamp package with qualified assertions, the thread-local cache teardown used by the sibling suites, a loader family test and an XLSX/XLS/CSV round trip.

feat: add java.sql.Timestamp converters

8132272

delei added the PR: first-time contributor first-time contributor label Aug 18, 2026

delei requested a lite review from Copilot August 18, 2026 00:47

Copilot started reviewing on behalf of delei August 18, 2026 00:48

View session

Copilot AI reviewed Aug 18, 2026

View reviewed changes

fix: preserve local timestamp conversion semantics

b407e8c

nkuprins suggested changes Aug 19, 2026

View reviewed changes

test: verify timestamp locale formatting

f42d147

bengbengbalabalabeng added the waiting for feedback Waiting for a reply or feedback label Sep 18, 2026

Mikkey-f mentioned this pull request Oct 14, 2026

feat: add java.sql.Timestamp converters (#1017) #1220

Open

2 tasks done

Navigation Menu

Repository navigation

feat: add java.sql.Timestamp converters - #1019

feat: add java.sql.Timestamp converters#1019
SyedIshmumAhnaf wants to merge 3 commits into
apache:mainfrom
SyedIshmumAhnaf:feat/timestamp-converters

SyedIshmumAhnaf commented Aug 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

nkuprins Aug 19, 2026 •

edited

Loading

Uh oh!

SyedIshmumAhnaf commented Aug 21, 2026

Uh oh!

bengbengbalabalabeng commented Aug 23, 2026 •

edited

Loading

Uh oh!

nkuprins commented Sep 16, 2026

Uh oh!

Mikkey-f commented Oct 13, 2026

Uh oh!

SyedIshmumAhnaf commented Oct 13, 2026

Uh oh!

Mikkey-f commented Oct 14, 2026

Uh oh!

Mikkey-f commented Oct 14, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

SyedIshmumAhnaf commented Aug 17, 2026

Purpose of the pull request

What's changed?

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

nkuprins Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SyedIshmumAhnaf commented Aug 21, 2026

Uh oh!

bengbengbalabalabeng commented Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkuprins commented Sep 16, 2026

Uh oh!

Mikkey-f commented Oct 13, 2026

Uh oh!

SyedIshmumAhnaf commented Oct 13, 2026

Uh oh!

Mikkey-f commented Oct 14, 2026

Uh oh!

Mikkey-f commented Oct 14, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nkuprins Aug 19, 2026 •

edited

Loading

bengbengbalabalabeng commented Aug 23, 2026 •

edited

Loading