Copilot commented on code in PR #9526:
URL: https://github.com/apache/seatunnel/pull/9526#discussion_r2178853151
##########
seatunnel-connectors-v2/connector-file/connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/source/reader/AbstractReadStrategy.java:
##########
@@ -45,8 +45,13 @@
import java.io.IOException;
import java.io.InputStream;
import java.math.BigDecimal;
+import java.time.Instant;
import java.time.LocalDate;
import java.time.LocalDateTime;
Review Comment:
The imports for `LocalDate` and `LocalDateTime` are unused and can be
removed to clean up the code.
```suggestion
```
##########
seatunnel-connectors-v2/connector-file/connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/source/reader/AbstractReadStrategy.java:
##########
@@ -145,6 +155,50 @@ public List<String> getFileNamesByPath(String path) throws
IOException {
return fileNames;
}
+ private boolean filterFileByModificationDate(FileStatus fileStatus) {
+
+ Instant fileModifiedDate =
Instant.ofEpochMilli(fileStatus.getModificationTime());
+
+ DateTimeFormatter formatter =
+
DateTimeFormatter.ofPattern(modifiedDateFormat).withZone(ZoneId.of("GMT+8"));
+
Review Comment:
If `modifiedDateFormat` is null or unset, `ofPattern` will throw a
NullPointerException. Either default `modifiedDateFormat` or guard against null
before creating the formatter.
```suggestion
if (StringUtils.isEmpty(modifiedDateFormat)) {
modifiedDateFormat = "yyyy-MM-dd"; // Default date format
}
DateTimeFormatter formatter =
DateTimeFormatter.ofPattern(modifiedDateFormat).withZone(ZoneId.of("GMT+8"));
```
##########
seatunnel-connectors-v2/connector-file/connector-file-base/src/test/java/org/apache/seatunnel/connectors/seatunnel/file/source/reader/AbstractReadStrategyTest.java:
##########
@@ -131,4 +144,97 @@ public static void deleteFile(String file) {
}
}
}
+
+ @BeforeEach
+ void setUp() throws Exception {
+ strategy = new CsvReadStrategy();
+ filterMethod =
+ AbstractReadStrategy.class.getDeclaredMethod(
+ "filterFileByModificationDate", FileStatus.class);
+ filterMethod.setAccessible(true);
+ }
+
+ @AfterEach
+ void tearDown() {
+ strategy = null;
+ filterMethod = null;
+ }
+
+ @Test
+ void testBothStartAndEndWithinRange() throws Exception {
+ try (MockedStatic<LocalDate> localDateMockedStatic =
mockStatic(LocalDate.class)) {
+
+ String startDateStr = "2024-01-01";
+ String endDateStr = "2024-12-31";
+ String format = "yyyy-MM-dd";
+
+ strategy.fileModifiedStartDate = startDateStr;
+ strategy.fileModifiedEndDate = endDateStr;
+ strategy.modifiedDateFormat = format;
+
+ long modificationTime =
Instant.parse("2024-06-01T00:00:00Z").toEpochMilli();
+
+ FileStatus mockFileStatus = mock(FileStatus.class);
+
when(mockFileStatus.getModificationTime()).thenReturn(modificationTime);
+
+ Boolean result = (Boolean) filterMethod.invoke(strategy,
mockFileStatus);
+
+ Assertions.assertTrue(result);
+ }
+ }
+
+ @Test
+ void testOnlyEndDateOutOfRange() throws Exception {
+ try (MockedStatic<LocalDate> localDateMockedStatic =
mockStatic(LocalDate.class)) {
+ String endDateStr = "2024-07-01";
+ String format = "yyyy-MM-dd";
+
+ strategy.fileModifiedStartDate = null;
+ strategy.fileModifiedEndDate = endDateStr;
+ strategy.modifiedDateFormat = format;
+
+ long modificationTime =
Instant.parse("2024-06-01T00:00:00Z").toEpochMilli();
+
+ FileStatus mockFileStatus = mock(FileStatus.class);
+
when(mockFileStatus.getModificationTime()).thenReturn(modificationTime);
+
+ Boolean result = (Boolean) filterMethod.invoke(strategy,
mockFileStatus);
+
+ Assertions.assertTrue(result);
+ }
Review Comment:
[nitpick] Mocking `LocalDate` is unnecessary since the code under test
doesn't use it; remove this static mock to simplify tests.
```suggestion
String startDateStr = "2024-01-01";
String endDateStr = "2024-12-31";
String format = "yyyy-MM-dd";
strategy.fileModifiedStartDate = startDateStr;
strategy.fileModifiedEndDate = endDateStr;
strategy.modifiedDateFormat = format;
long modificationTime =
Instant.parse("2024-06-01T00:00:00Z").toEpochMilli();
FileStatus mockFileStatus = mock(FileStatus.class);
when(mockFileStatus.getModificationTime()).thenReturn(modificationTime);
Boolean result = (Boolean) filterMethod.invoke(strategy,
mockFileStatus);
Assertions.assertTrue(result);
}
@Test
void testOnlyEndDateOutOfRange() throws Exception {
String endDateStr = "2024-07-01";
String format = "yyyy-MM-dd";
strategy.fileModifiedStartDate = null;
strategy.fileModifiedEndDate = endDateStr;
strategy.modifiedDateFormat = format;
long modificationTime =
Instant.parse("2024-06-01T00:00:00Z").toEpochMilli();
FileStatus mockFileStatus = mock(FileStatus.class);
when(mockFileStatus.getModificationTime()).thenReturn(modificationTime);
Boolean result = (Boolean) filterMethod.invoke(strategy,
mockFileStatus);
Assertions.assertTrue(result);
```
##########
seatunnel-connectors-v2/connector-file/connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/source/reader/AbstractReadStrategy.java:
##########
@@ -145,6 +155,50 @@ public List<String> getFileNamesByPath(String path) throws
IOException {
return fileNames;
}
+ private boolean filterFileByModificationDate(FileStatus fileStatus) {
+
+ Instant fileModifiedDate =
Instant.ofEpochMilli(fileStatus.getModificationTime());
+
+ DateTimeFormatter formatter =
+
DateTimeFormatter.ofPattern(modifiedDateFormat).withZone(ZoneId.of("GMT+8"));
+
+ Instant startDate = parseDateToInstant(fileModifiedStartDate,
formatter);
+ Instant endDate = parseDateToInstant(fileModifiedEndDate, formatter);
+
+ // Both start and end date are set
+ if (startDate != null && endDate != null) {
+ return !fileModifiedDate.isBefore(startDate) &&
!fileModifiedDate.isAfter(endDate);
+ }
+
+ // Only start date is set
+ if (startDate != null) {
+ return !fileModifiedDate.isBefore(startDate);
+ }
+
+ // Only end date is set
+ if (endDate != null) {
+ return !fileModifiedDate.isAfter(endDate);
+ }
+
+ // Neither start nor end date is set
+ return true;
+ }
+
+ private Instant parseDateToInstant(String dateTime, DateTimeFormatter
dateTimeFormatter) {
+ if (dateTime == null || dateTime.isEmpty()) {
+ return null;
+ }
+ try {
+
+ TemporalAccessor temporal = dateTimeFormatter.parse(dateTime);
+
+ return Instant.from(temporal);
+ } catch (DateTimeParseException e) {
+ // no throw exception
Review Comment:
[nitpick] Silently catching parse exceptions may mask configuration errors.
Consider logging a warning or surfacing an error when a date fails to parse.
```suggestion
log.warn("Failed to parse date '{}' with format '{}'. Returning
null.", dateTime, dateTimeFormatter, e);
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
