Copilot commented on code in PR #3558:
URL: https://github.com/apache/fluss/pull/3558#discussion_r3534594573
##########
fluss-common/src/main/java/org/apache/fluss/config/ConfigOptions.java:
##########
@@ -1581,14 +1581,23 @@ public class ConfigOptions {
+ "If the value is `HOUR`, the partition
format for "
+ "auto created is yyyyMMddHH. "
+ "If the value is `DAY`, the partition
format for "
- + "auto created is yyyyMMdd. "
+ + "auto created is yyyyMMdd by default. "
+ "If the value is `MONTH`, the partition
format for "
+ "auto created is yyyyMM. "
+ "If the value is `QUARTER`, the
partition format for "
+ "auto created is yyyyQ. "
+ "If the value is `YEAR`, the partition
format for "
+ "auto created is yyyy.");
+ public static final ConfigOption<String> TABLE_AUTO_PARTITION_DAY_FORMAT =
+ key("table.auto-partition.day-format")
+ .stringType()
+ .defaultValue("yyyyMMdd")
+ .withDescription(
+ "The format for DAY auto created partitions. "
+ + "Supported values are `yyyyMMdd` and
`yyyy-MM-dd`. "
+ + "This option only applies when
`table.auto-partition.time-unit` is `DAY`.");
Review Comment:
The description says this option only applies when
`table.auto-partition.time-unit` is `DAY`, but validation also requires
auto-partitioning to be enabled. Updating the description to reflect that
avoids surprising users who try to set it while auto partitioning is disabled.
##########
fluss-common/src/test/java/org/apache/fluss/utils/PartitionUtilsTest.java:
##########
@@ -158,17 +190,102 @@ void testGenerateAutoPartitionName(
AutoPartitionTimeUnit autoPartitionTimeUnit,
int[] offsets,
String[] expected) {
+ testGenerateAutoPartitionName(
+ zonedDateTime,
+ AutoPartitionStrategy.from(new Configuration()),
+ autoPartitionTimeUnit,
+ offsets,
+ expected);
+ }
+
+ void testGenerateAutoPartitionName(
+ ZonedDateTime zonedDateTime,
+ AutoPartitionStrategy autoPartitionStrategy,
+ AutoPartitionTimeUnit autoPartitionTimeUnit,
+ int[] offsets,
+ String[] expected) {
for (int i = 0; i < offsets.length; i++) {
ResolvedPartitionSpec resolvedPartitionSpec =
generateAutoPartition(
Collections.singletonList("dt"),
zonedDateTime,
offsets[i],
- autoPartitionTimeUnit);
+ autoPartitionTimeUnit,
+ autoPartitionStrategy);
assertThat(resolvedPartitionSpec.getPartitionName()).isEqualTo(expected[i]);
}
}
+ @Test
+ void testValidateAutoPartitionTimeWithConfiguredDayFormat() {
+ Configuration dashedDayConf = new Configuration();
+ dashedDayConf.setBoolean(ConfigOptions.TABLE_AUTO_PARTITION_ENABLED,
true);
+ dashedDayConf.set(ConfigOptions.TABLE_AUTO_PARTITION_TIME_UNIT,
AutoPartitionTimeUnit.DAY);
+ dashedDayConf.setString(ConfigOptions.TABLE_AUTO_PARTITION_DAY_FORMAT,
"yyyy-MM-dd");
+ AutoPartitionStrategy dashedDayStrategy =
AutoPartitionStrategy.from(dashedDayConf);
+
+ LocalDate today = LocalDate.now();
Review Comment:
Using `LocalDate.now()` here can make the test timezone- or
midnight-sensitive because `validateAutoPartitionTime(...)` computes "now" from
`Instant.now()` in the strategy timezone. Derive `today` from the same timezone
to avoid flaky boundary behavior.
##########
fluss-server/src/test/java/org/apache/fluss/server/coordinator/AutoPartitionManagerTest.java:
##########
@@ -1018,6 +1116,9 @@ private TableInfo createPartitionedTable(
.property(
ConfigOptions.TABLE_AUTO_PARTITION_NUM_RETENTION,
partitionRetentionNum)
+ .property(
+ ConfigOptions.TABLE_AUTO_PARTITION_DAY_FORMAT,
+ dayFormat == null ? "yyyyMMdd" : dayFormat)
Review Comment:
This helper always sets `table.auto-partition.day-format` even when
`dayFormat` is null (default). That makes the option look explicitly
configured, which is now invalid when the auto-partition time unit is not DAY,
and also diverges from how real `TableDescriptor` properties usually omit
defaulted options.
##########
fluss-common/src/test/java/org/apache/fluss/utils/PartitionUtilsTest.java:
##########
@@ -158,17 +190,102 @@ void testGenerateAutoPartitionName(
AutoPartitionTimeUnit autoPartitionTimeUnit,
int[] offsets,
String[] expected) {
+ testGenerateAutoPartitionName(
+ zonedDateTime,
+ AutoPartitionStrategy.from(new Configuration()),
+ autoPartitionTimeUnit,
+ offsets,
+ expected);
+ }
+
+ void testGenerateAutoPartitionName(
+ ZonedDateTime zonedDateTime,
+ AutoPartitionStrategy autoPartitionStrategy,
+ AutoPartitionTimeUnit autoPartitionTimeUnit,
+ int[] offsets,
+ String[] expected) {
for (int i = 0; i < offsets.length; i++) {
ResolvedPartitionSpec resolvedPartitionSpec =
generateAutoPartition(
Collections.singletonList("dt"),
zonedDateTime,
offsets[i],
- autoPartitionTimeUnit);
+ autoPartitionTimeUnit,
+ autoPartitionStrategy);
assertThat(resolvedPartitionSpec.getPartitionName()).isEqualTo(expected[i]);
}
}
+ @Test
+ void testValidateAutoPartitionTimeWithConfiguredDayFormat() {
+ Configuration dashedDayConf = new Configuration();
+ dashedDayConf.setBoolean(ConfigOptions.TABLE_AUTO_PARTITION_ENABLED,
true);
+ dashedDayConf.set(ConfigOptions.TABLE_AUTO_PARTITION_TIME_UNIT,
AutoPartitionTimeUnit.DAY);
+ dashedDayConf.setString(ConfigOptions.TABLE_AUTO_PARTITION_DAY_FORMAT,
"yyyy-MM-dd");
+ AutoPartitionStrategy dashedDayStrategy =
AutoPartitionStrategy.from(dashedDayConf);
+
+ LocalDate today = LocalDate.now();
+
+ assertThatNoException()
+ .isThrownBy(
+ () ->
+ validateAutoPartitionTime(
+ new PartitionSpec(
+ Collections.singletonMap("dt",
today.toString())),
+ Collections.singletonList("dt"),
+ dashedDayStrategy));
+
+ assertThatThrownBy(
+ () ->
+ validateAutoPartitionTime(
+ new PartitionSpec(
+ Collections.singletonMap(
+ "dt",
+ today.format(
+
java.time.format.DateTimeFormatter
+
.ofPattern("yyyyMMdd")))),
+ Collections.singletonList("dt"),
+ dashedDayStrategy))
+ .isInstanceOf(InvalidPartitionException.class)
+ .hasMessageContaining("yyyy-MM-dd")
+ .hasMessageContaining("DAY");
+ }
+
+ @Test
+ void
testValidateAutoPartitionTimeRetentionBoundaryWithConfiguredDayFormat() {
+ Configuration dashedDayConf = new Configuration();
+ dashedDayConf.setBoolean(ConfigOptions.TABLE_AUTO_PARTITION_ENABLED,
true);
+ dashedDayConf.set(ConfigOptions.TABLE_AUTO_PARTITION_TIME_UNIT,
AutoPartitionTimeUnit.DAY);
+ dashedDayConf.setString(ConfigOptions.TABLE_AUTO_PARTITION_DAY_FORMAT,
"yyyy-MM-dd");
+ AutoPartitionStrategy dashedDayStrategy =
AutoPartitionStrategy.from(dashedDayConf);
+
+ LocalDate today = LocalDate.now();
+ LocalDate earliestRetained = today.minusDays(7);
+ LocalDate outOfDate = today.minusDays(8);
+
Review Comment:
Same issue as above: `LocalDate.now()` can diverge from the strategy
timezone used by `validateAutoPartitionTime(...)`, potentially making this
retention-boundary assertion flaky around date boundaries. Compute dates from
the strategy timezone instead.
--
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]
