wuchong commented on code in PR #3558:
URL: https://github.com/apache/fluss/pull/3558#discussion_r3566565482
##########
fluss-server/src/main/java/org/apache/fluss/server/utils/TableDescriptorValidation.java:
##########
@@ -518,6 +554,25 @@ private static void checkPartition(
+ "partition is enabled, please
set table property '%s'.",
ConfigOptions.TABLE_AUTO_PARTITION_TIME_UNIT.key()));
}
+
+ if (enforceDateDayFormatCompatibility
+ && autoPartition.timeUnit() ==
AutoPartitionTimeUnit.DAY) {
+ String autoPartitionKey =
+
StringUtils.isNullOrWhitespaceOnly(autoPartition.key())
+ ? partitionKeys.get(0)
+ : autoPartition.key();
+ DataType autoPartitionDataType =
+
rowType.getTypeAt(rowType.getFieldIndex(autoPartitionKey));
+ if (autoPartitionDataType.getTypeRoot() ==
DataTypeRoot.DATE
+ && autoPartition.dayFormat() !=
AutoPartitionDayFormat.YYYY_MM_DD) {
+ throw new InvalidTableException(
+ String.format(
+ "Table property '%s' must be '%s' when
auto partition key '%s' has DATE type.",
+
ConfigOptions.TABLE_AUTO_PARTITION_DAY_FORMAT.key(),
+
AutoPartitionDayFormat.YYYY_MM_DD.pattern(),
+ autoPartitionKey));
Review Comment:
Why don't we allow other date formats for date-type partitions?
##########
fluss-common/src/main/java/org/apache/fluss/config/ConfigOptions.java:
##########
@@ -1604,14 +1583,24 @@ 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")
Review Comment:
Would it make sense to generalize this option to
`table.auto-partition.time-format` instead of introducing a DAY-specific public
key? The formatting path already handles `HOUR`, `DAY`, `MONTH`, `QUARTER`, and
`YEAR`. Once `day-format` becomes public, supporting formats such as `yyyy-MM`
or `yyyy-MM-dd-HH` later would require additional per-unit options or
deprecating this key.
If we generalize it, however, I don't think we should accept arbitrary
`DateTimeFormatter` patterns without validation. Partition retention currently
relies on the lexicographical ordering of formatted partition values through
`compareTo()` and `TreeMap.headMap()`, and the format must preserve the
configured granularity. For example, an HOUR format without `HH` would cause
multiple hours to map to the same partition, while `MM-yyyy` or non-zero-padded
fields would not sort chronologically.
Could we introduce `table.auto-partition.time-format`, use the existing
unit-specific formats when the option is absent for backward compatibility, and
validate supported formats per time unit so that they are fixed-width, ordered
from the most significant field to the least significant field, and include the
required time-unit field?
##########
fluss-server/src/main/java/org/apache/fluss/server/utils/TableDescriptorValidation.java:
##########
@@ -458,9 +472,31 @@ private static void checkTieredLog(Configuration
tableConf) {
}
private static void checkPartition(
- Configuration tableConf, List<String> partitionKeys, RowType
rowType) {
+ Configuration tableConf,
+ List<String> partitionKeys,
+ RowType rowType,
+ boolean enforceDateDayFormatCompatibility) {
boolean isPartitioned = !partitionKeys.isEmpty();
AutoPartitionStrategy autoPartition =
AutoPartitionStrategy.from(tableConf);
+ boolean hasExplicitDayFormat =
+
tableConf.contains(ConfigOptions.TABLE_AUTO_PARTITION_DAY_FORMAT);
+
+ if (hasExplicitDayFormat && !autoPartition.isAutoPartitionEnabled()) {
Review Comment:
**Allow the day format to be retained when auto partitioning is disabled**
For a table configured with `table.auto-partition.day-format=yyyy-MM-dd`,
executing `ALTER TABLE ... SET table.auto-partition.enabled=false` keeps the
persisted day-format property, so this check throws `InvalidTableException`.
Since the day-format option itself cannot be altered, users have no valid way
to disable auto partitioning.
Please allow the format to be retained when auto partitioning is disabled,
so it can still be used to automatically validate the partition-name format
when partitions are created.
##########
fluss-server/src/main/java/org/apache/fluss/server/utils/TableDescriptorValidation.java:
##########
@@ -88,6 +90,14 @@ public static void validateTableDescriptor(
TableDescriptor tableDescriptor,
int maxBucketNum,
@Nullable DataLakeFormat clusterDataLakeFormat) {
+ validateTableDescriptor(tableDescriptor, maxBucketNum,
clusterDataLakeFormat, true);
+ }
+
+ public static void validateTableDescriptor(
+ TableDescriptor tableDescriptor,
+ int maxBucketNum,
+ @Nullable DataLakeFormat clusterDataLakeFormat,
+ boolean enforceDateDayFormatCompatibility) {
Review Comment:
Why introducing this flag? It seems this variable is always `true`?
--
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]
