Skip to content

feat: add java.sql.Time converter support - #1092

Open
SummerC0zyR0ck wants to merge 5 commits into
apache:mainfrom
SummerC0zyR0ck:feature-sql-time-converter
Open

SummerC0zyR0ck wants to merge 5 commits into
apache:mainfrom
SummerC0zyR0ck:feature-sql-time-converter

Conversation

@SummerC0zyR0ck

Copy link
Copy Markdown

Purpose of the pull request

Related: #1017

Add first-class converter support for java.sql.Time, following the existing LocalTime converter pattern.

What's changed?

java.sql.Time represents a time-of-day value, but converter lookup uses the declared Java class. Therefore, the existing java.util.Date and LocalTime converters do not apply to fields declared as java.sql.Time.

This PR adds a converter family under org.apache.fesod.sheet.converters.sqltime:

  • SqlTimeDateConverter - default write path using an Excel DATE cell. It attaches DateUtils.EPOCH (1970-01-01) and applies HH:mm:ss by default.
  • SqlTimeNumberConverter - bidirectional Excel numeric serial conversion, including use1904windowing. Reading extracts the time component and discards any date component.
  • SqlTimeStringConverter - bidirectional STRING conversion. It defaults to HH:mm:ss, auto-detects HH:mm input, and respects @DateTimeFormat and the configured Locale.

The converters reuse the existing LocalTime parsing, formatting, and Excel serial handling through Time.toLocalTime() and Time.valueOf(LocalTime). No time-zone conversion is introduced.

Registration in DefaultConverterLoader follows the LocalTime pattern:

  • Number and String converters in initAllConverter()
  • Date converter as the default write converter
  • String converter for string-formatted writes

Tests

Tests cover converter keys, DATE/NUMBER/STRING conversion, custom date-time formats, 1904 date windowing, discarding the date component from numeric values, registry immutability, and round-trip behavior for XLSX, XLS, and CSV.

The complete fesod-sheet test suite passes with 923 tests, and both spotless:check and Apache RAT checks are green.

Scope is limited to java.sql.Time and remains JDK 8 compatible.

@SummerC0zyR0ck
SummerC0zyR0ck force-pushed the feature-sql-time-converter branch 2 times, most recently from 57b95ff to d07e5be Compare September 11, 2026 08:51
@SummerC0zyR0ck
SummerC0zyR0ck force-pushed the feature-sql-time-converter branch from 9440e55 to 5dee8dc Compare September 14, 2026 05:53
@bengbengbalabalabeng

Copy link
Copy Markdown
Contributor

Since the converter respects the configured Locale, it would be great to include tests that verify locale‑specific formatting.

For example, "HH:mm:ss a" produces "AM/PM" under Locale.US, but "上午/下午" under Locale.CHINA.

@SummerC0zyR0ck

Copy link
Copy Markdown
Author

Since the converter respects the configured Locale, it would be great to include tests that verify locale‑specific formatting.

For example, "HH:mm:ss a" produces "AM/PM" under Locale.US, but "上午/下午" under Locale.CHINA.

Thanks for the suggestion! I added a locale-specific formatting test covering Locale.US (PM) and Locale.CHINA (下午) with the HH:mm:ss a pattern. The test passes.

import org.apache.poi.ss.usermodel.DateUtil;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add @Tag(Tags.UNIT).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the review! Added @tag(Tags.UNIT) to SqlTimeConverterTest along with the required imports.

@Override
public Time convertToJavaData(
ReadCellData<?> cellData, ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration) {
return Time.valueOf(DateUtils.getLocalTime(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

An invalid argument here causes NPE: DateUtils.getLocalTime returns null for it, and Time.valueOf(null) fails. However, the other converters like LocalTimeNumberConverter return null in this case, matching POI's DateUtil.

Should we keep that behaviour?

LocalTime localTime = DateUtils.getLocalTime(
        cellData.getNumberValue().doubleValue(), DateUtils.isDate1904(contentProperty, globalConfiguration));
return localTime == null ? null : Time.valueOf(localTime);

@SummerC0zyR0ck SummerC0zyR0ck Sep 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good catch — you're right. DateUtils.getLocalTime returns null for an invalid Excel serial, and Time.valueOf(null) throws an NPE. I've updated the converter to mirror LocalTimeNumberConverter:

LocalTime localTime = DateUtils.getLocalTime(
        cellData.getNumberValue().doubleValue(), DateUtils.isDate1904(contentProperty, globalConfiguration));
return localTime == null ? null : Time.valueOf(localTime);

I also added a regression test (invalidNumberReturnsNull) to lock in the behavior. Thanks for pointing it out!

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.

3 participants