You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fesod.apache.org by "Mikkey-f (via GitHub)" <gi...@apache.org> on 2026/10/14 11:23:22 UTC
[PR] feat: add java.sql.Timestamp converters (#1017) [fesod]
Mikkey-f opened a new pull request, #1220:
URL: https://github.com/apache/fesod/pull/1220
## Purpose of the pull request
Related: #1017 (java.sql.Timestamp slice).
Adds `java.sql.Timestamp` converter support, carried forward from #1019 by @SyedIshmumAhnaf, who handed the PR over — the original commits are preserved with their authorship and one additional commit brings the code up to date with `main`.
## What's changed?
- `TimestampNumberConverter` and `TimestampStringConverter` (read + write) and `TimestampDateConverter` (write), registered in `DefaultConverterLoader` next to the `java.sql.Time` family.
- Values keep local date-time semantics on both sides: reads use `DateUtils.getLocalDateTime` and writes convert through `Timestamp#toLocalDateTime()`, so no implicit timezone shifts are introduced.
### Carry-forward adaptation
- The 1904 windowing flag now resolves through the shared `DateUtils.isDate1904` helper. Reading `DateTimeFormatProperty#getUse1904windowing()` directly throws a `NullPointerException` since #1042 keeps the annotation DEFAULT as `null` — any `@DateTimeFormat` without an explicit `use1904windowing` value would hit it.
- Dropped the EasyExcel derived license header from the new files (plain ASF header, written from scratch).
- Tests moved to `converters.timestamp`, with qualified assertions and the same thread-local cache teardown as sibling suites, plus a `loadConvertersRegistersTimestampFamily` case and an XLSX/XLS/CSV round trip.
### Notes carried over from #1019
- @nkuprins's review note: the locale test now uses a month name (`dd MMMM yyyy HH:mm:ss`), so it fails when the locale is not honored.
- The two Copilot notes (timezone-sensitive write side in the number and date converters) were addressed on the original branch and are preserved here.
## Verification
- Red/green on the adaptation: the new windowing test fails on the original code with `NullPointerException: ... because getUse1904windowing() is null`, passes after the change.
- Full suite: 1059 tests, 0 failures (`fesod-common`, `fesod-shaded`, `fesod-sheet`); `spotless:check` and `javadoc:javadoc` pass.
Supersedes #1019 — many thanks to @SyedIshmumAhnaf for the original implementation.
## Checklist
- [x] I have read the [Contributor Guide](https://fesod.apache.org/community/contribution/).
- [x] I have added the necessary unit tests and all cases have passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add java.sql.Timestamp converters (#1017) [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng merged PR #1220:
URL: https://github.com/apache/fesod/pull/1220
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add java.sql.Timestamp converters (#1017) [fesod]
Posted by "nkuprins (via GitHub)" <gi...@apache.org>.
nkuprins commented on PR #1220:
URL: https://github.com/apache/fesod/pull/1220#issuecomment-6193622023
Please resolve merge conflicts :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add java.sql.Timestamp converters (#1017) [fesod]
Posted by "Mikkey-f (via GitHub)" <gi...@apache.org>.
Mikkey-f commented on code in PR #1220:
URL: https://github.com/apache/fesod/pull/1220#discussion_r4283148730
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/timestamp/TimestampDateConverter.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fesod.sheet.converters.timestamp;
+
+import java.sql.Timestamp;
+import org.apache.fesod.sheet.converters.Converter;
+import org.apache.fesod.sheet.metadata.GlobalConfiguration;
+import org.apache.fesod.sheet.metadata.data.WriteCellData;
+import org.apache.fesod.sheet.metadata.property.ExcelContentProperty;
+import org.apache.fesod.sheet.util.DateUtils;
+import org.apache.fesod.sheet.util.WorkBookUtil;
+
+/** Timestamp and date converter. */
+public class TimestampDateConverter implements Converter<Timestamp> {
+ @Override
+ public Class<?> supportJavaTypeKey() {
+ return Timestamp.class;
+ }
+
+ @Override
+ public WriteCellData<?> convertToExcelData(
+ Timestamp value, ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration)
+ throws Exception {
+ WriteCellData<?> cellData = new WriteCellData<>(value.toLocalDateTime());
+ String format = null;
+ if (contentProperty != null && contentProperty.getDateTimeFormatProperty() != null) {
+ format = contentProperty.getDateTimeFormatProperty().getFormat();
+ }
+ WorkBookUtil.fillDataFormat(cellData, format, DateUtils.defaultDateFormat);
Review Comment:
Good catch — fixed in 41ad950d: an empty format is now normalised to `null` before `WorkBookUtil.fillDataFormat`, matching `SqlTimeDateConverter`, so it falls back to the default.
Regression test: `dateConverterFallsBackToDefaultFormatForEmptyDateTimeFormat` — on the old code it fails with `expected: <yyyy-MM-dd HH:mm:ss> but was: <>`, passes after the change.
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/timestamp/TimestampNumberConverter.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fesod.sheet.converters.timestamp;
+
+import java.math.BigDecimal;
+import java.sql.Timestamp;
+import org.apache.fesod.sheet.converters.Converter;
+import org.apache.fesod.sheet.enums.CellDataTypeEnum;
+import org.apache.fesod.sheet.metadata.GlobalConfiguration;
+import org.apache.fesod.sheet.metadata.data.ReadCellData;
+import org.apache.fesod.sheet.metadata.data.WriteCellData;
+import org.apache.fesod.sheet.metadata.property.ExcelContentProperty;
+import org.apache.fesod.sheet.util.DateUtils;
+import org.apache.poi.ss.usermodel.DateUtil;
+
+/** Timestamp and number converter. */
+public class TimestampNumberConverter implements Converter<Timestamp> {
+
+ @Override
+ public Class<?> supportJavaTypeKey() {
+ return Timestamp.class;
+ }
+
+ @Override
+ public CellDataTypeEnum supportExcelTypeKey() {
+ return CellDataTypeEnum.NUMBER;
+ }
+
+ @Override
+ public Timestamp convertToJavaData(
+ ReadCellData<?> cellData, ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration) {
+ return Timestamp.valueOf(DateUtils.getLocalDateTime(
Review Comment:
Right — fixed in 41ad950d: `DateUtils.getLocalDateTime` returning `null` (a number that is not a valid Excel date, e.g. a negative value) no longer reaches `Timestamp.valueOf(null)`; the converter now returns `null`, mirroring `SqlTimeNumberConverter`.
Regression test: `numberConverterReturnsNullForInvalidExcelDate` — red on the old code with `NullPointerException: Cannot invoke "java.time.LocalDateTime.getYear()" because "dateTime" is null`, green now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add java.sql.Timestamp converters (#1017) [fesod]
Posted by "Mikkey-f (via GitHub)" <gi...@apache.org>.
Mikkey-f commented on PR #1220:
URL: https://github.com/apache/fesod/pull/1220#issuecomment-6395277359
Hi @nkuprins — a quick heads-up that I rebased the branch onto current main on 22 Oct (new head 59bf7551) to resolve the conflict with the newly merged Instant converters in `DefaultConverterLoader` and its test. The rebase only replayed our existing commits and kept both converter families registered — no functional change from the version you approved.
All CI checks are green on the new head (9/9). As the push invalidated your earlier approval, could you take another look and re-approve when you have a moment? Happy to address anything that still looks off.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add java.sql.Timestamp converters (#1017) [fesod]
Posted by "nkuprins (via GitHub)" <gi...@apache.org>.
nkuprins commented on code in PR #1220:
URL: https://github.com/apache/fesod/pull/1220#discussion_r4272550385
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/timestamp/TimestampDateConverter.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fesod.sheet.converters.timestamp;
+
+import java.sql.Timestamp;
+import org.apache.fesod.sheet.converters.Converter;
+import org.apache.fesod.sheet.metadata.GlobalConfiguration;
+import org.apache.fesod.sheet.metadata.data.WriteCellData;
+import org.apache.fesod.sheet.metadata.property.ExcelContentProperty;
+import org.apache.fesod.sheet.util.DateUtils;
+import org.apache.fesod.sheet.util.WorkBookUtil;
+
+/** Timestamp and date converter. */
+public class TimestampDateConverter implements Converter<Timestamp> {
+ @Override
+ public Class<?> supportJavaTypeKey() {
+ return Timestamp.class;
+ }
+
+ @Override
+ public WriteCellData<?> convertToExcelData(
+ Timestamp value, ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration)
+ throws Exception {
+ WriteCellData<?> cellData = new WriteCellData<>(value.toLocalDateTime());
+ String format = null;
+ if (contentProperty != null && contentProperty.getDateTimeFormatProperty() != null) {
+ format = contentProperty.getDateTimeFormatProperty().getFormat();
+ }
+ WorkBookUtil.fillDataFormat(cellData, format, DateUtils.defaultDateFormat);
Review Comment:
If `format` is empty, it is still passed instead of falling back to the default format. Could we normalise an empty format to `null`?
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/timestamp/TimestampNumberConverter.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fesod.sheet.converters.timestamp;
+
+import java.math.BigDecimal;
+import java.sql.Timestamp;
+import org.apache.fesod.sheet.converters.Converter;
+import org.apache.fesod.sheet.enums.CellDataTypeEnum;
+import org.apache.fesod.sheet.metadata.GlobalConfiguration;
+import org.apache.fesod.sheet.metadata.data.ReadCellData;
+import org.apache.fesod.sheet.metadata.data.WriteCellData;
+import org.apache.fesod.sheet.metadata.property.ExcelContentProperty;
+import org.apache.fesod.sheet.util.DateUtils;
+import org.apache.poi.ss.usermodel.DateUtil;
+
+/** Timestamp and number converter. */
+public class TimestampNumberConverter implements Converter<Timestamp> {
+
+ @Override
+ public Class<?> supportJavaTypeKey() {
+ return Timestamp.class;
+ }
+
+ @Override
+ public CellDataTypeEnum supportExcelTypeKey() {
+ return CellDataTypeEnum.NUMBER;
+ }
+
+ @Override
+ public Timestamp convertToJavaData(
+ ReadCellData<?> cellData, ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration) {
+ return Timestamp.valueOf(DateUtils.getLocalDateTime(
Review Comment:
If `DateUtils.getLocalDateTime` returns null, then it throws. Could we return null in that case for consistency with other converters?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org