github-actions[bot] commented on code in PR #65219:
URL: https://github.com/apache/doris/pull/65219#discussion_r3614189953
##########
fe/fe-core/src/main/java/org/apache/doris/clone/DynamicPartitionScheduler.java:
##########
@@ -201,23 +199,48 @@ private static long getNextPartitionSize(List<Long>
historyPartitionsSize) {
}
private static Pair<Integer, Integer>
getBucketsNum(DynamicPartitionProperty property, OlapTable table,
- String partitionName, String nowPartitionName, boolean
executeFirstTime) {
+ String partitionName, String nowPartitionName, boolean
executeFirstTime, String currentUtcBorder) {
AutoBucketCalculator.AutoBucketContext context = new
AutoBucketCalculator.AutoBucketContext(
- table, partitionName, nowPartitionName, executeFirstTime,
property.getBuckets());
+ table, partitionName, nowPartitionName, executeFirstTime,
property.getBuckets(),
+ currentUtcBorder);
AutoBucketCalculator.AutoBucketResult result =
AutoBucketCalculator.calculateAutoBuckets(context);
return Pair.of(result.getBuckets(), result.getPreviousBuckets());
}
- public static List<Partition> getHistoricalPartitions(OlapTable table,
String nowPartitionName) {
+ /**
+ * Get all partitions except the current one. When currentUtcBorder is
non-null,
+ * the current partition is identified by matching its range lower bound
against
+ * currentUtcBorder rather than by name. For TIMESTAMPTZ tables both names
and
+ * ranges are UTC-based, so the name-based fallback is also correct; the
range
+ * check provides an additional safety layer.
+ */
+ public static List<Partition> getHistoricalPartitions(OlapTable table,
String nowPartitionName,
+ String currentUtcBorder) {
table.readLock();
try {
RangePartitionInfo info = (RangePartitionInfo)
(table.getPartitionInfo());
List<Map.Entry<Long, PartitionItem>> idToItems = new
ArrayList<>(info.getIdToItem(false).entrySet());
idToItems.sort(Comparator.comparing(o -> ((RangePartitionItem)
o.getValue()).getItems().upperEndpoint()));
return idToItems.stream()
.map(entry -> table.getPartition(entry.getKey()))
- .filter(partition -> partition != null &&
!partition.getName().equals(nowPartitionName))
+ .filter(partition -> {
+ if (partition == null) {
+ return false;
+ }
+ // When currentUtcBorder is available, exclude the
current
+ // partition by comparing range lower bounds, not
names.
+ if (currentUtcBorder != null) {
+ RangePartitionItem item = (RangePartitionItem)
info.getItem(partition.getId());
+ if (item != null) {
+ PartitionKey lower =
item.getItems().lowerEndpoint();
+ if
(lower.getKeys().get(0).getStringValue().equals(currentUtcBorder)) {
Review Comment:
`currentUtcBorder` is formatted only to whole seconds, but a scaled temporal
lower key preserves its declared scale in `getStringValue()` (for example,
`2026-07-20 00:00:00.000000+00:00` for `TIMESTAMPTZ(6)` versus `2026-07-20
00:00:00+00:00` here), so equal partition keys fail this string comparison. The
name fallback is not guaranteed: changing the supported
`dynamic_partition.prefix` updates the property without renaming existing
partitions. On the next pass that creates a missing future partition, both
checks then miss the active partition and `AutoBucketCalculator` can size from
its partial data. Please compare a parsed/normalized `PartitionKey` (or pass
the current key directly) and cover a scaled type with an existing old-prefix
current partition.
##########
fe/fe-core/src/main/java/org/apache/doris/clone/DynamicPartitionScheduler.java:
##########
@@ -655,19 +679,14 @@ private ArrayList<DropPartitionOp>
getDropPartitionOpForAutoPartition(Database d
idToItems.sort(Comparator.comparing(o -> ((RangePartitionItem)
o.getValue()).getItems().upperEndpoint()));
// Get current time as PartitionKey for comparison
- ZonedDateTime now = ZonedDateTime.now(DateUtils.getTimeZone());
Column partitionColumn = info.getPartitionColumns().get(0);
+ boolean isTimestampTz = partitionColumn.getDataType() ==
PrimitiveType.TIMESTAMPTZ;
+ ZonedDateTime nowTz = ZonedDateTime.now(DateUtils.getTimeZone());
+ ZonedDateTime now = isTimestampTz ?
nowTz.withZoneSameInstant(ZoneOffset.UTC) : nowTz;
Review Comment:
For TIMESTAMPTZ, this converts the clock to UTC, but `currentTimeStr` below
is formatted without an offset and `PartitionKey.createPartitionKey()` reparses
it through `TimestampTzLiteral.fromSessionTimeZone()`. The daemon's
null-context path happens to fall back to UTC, but
`InternalCatalog.createOlapTable()` also invokes first-time scheduling inline
with the creator's `ConnectContext`, after legal predefined AUTO RANGE
partitions have been installed. In an `America/Chicago` session, actual
`12:30Z` becomes suffix-less `12:30` and reparses as `17:30Z`, so
current/future initial partitions can be classified as history and one can be
dropped by `partition.retention_count`. Please keep this cutoff instant-bearing
(for example `+00:00` or a directly constructed UTC `PartitionKey`) and test
the predefined-partition, non-UTC-session path.
##########
fe/fe-core/src/test/java/org/apache/doris/catalog/DynamicPartitionTableTest.java:
##########
@@ -2219,17 +2311,147 @@ public void testTimeStampTzDynamicPartitionHourUnit()
throws Exception {
Assert.assertEquals("Hour partition name length: " +
partitionName,
11, partitionName.length());
- // Verify range validity and UTC boundaries
+ // Verify range validity
Range<PartitionKey> range = item.getItems();
Assert.assertTrue("lower must be < upper",
range.lowerEndpoint().compareTo(range.upperEndpoint())
< 0);
- // With time_zone = "+00:00", partition boundaries should be
UTC timestamps
List<LiteralExpr> lowerKeys = range.lowerEndpoint().getKeys();
Assert.assertEquals(1, lowerKeys.size());
String lowerStr = lowerKeys.get(0).getStringValue();
Assert.assertTrue("Lower key must have +00:00 suffix: " +
lowerStr,
lowerStr.contains("+00:00"));
+
+ List<LiteralExpr> upperKeys = range.upperEndpoint().getKeys();
+ Assert.assertEquals(1, upperKeys.size());
+ String upperStr = upperKeys.get(0).getStringValue();
+ Assert.assertTrue("Upper key must have +00:00 suffix: " +
upperStr,
+ upperStr.contains("+00:00"));
+
+ // Partition boundaries must be at whole UTC hours (0-23).
+ int lowerHour = Integer.parseInt(lowerStr.substring(11, 13));
Review Comment:
This only proves that a parsed hour is in its valid `0..23` range; the
adjacency checks below also pass for boundaries at `00:15, 01:15, ...`. Because
`Asia/Shanghai` has an integral-hour offset, the previous configured-zone hour
flooring also produces UTC `:00` boundaries, so this test does not prove the
HOUR-specific UTC-first change. Please use a supported fractional offset such
as `+05:45`/`Asia/Kathmandu` and assert that every lower and upper bound has
minute and second `00`, while keeping the full-timestamp adjacency check.
##########
fe/fe-core/src/test/java/org/apache/doris/common/PropertyAnalyzerTest.java:
##########
@@ -524,4 +526,139 @@ public void testAnalyzeSequenceMap() throws
AnalysisException {
Assert.fail();
}
}
+
+ @Test
+ public void testStorageCooldownTimeWithTzOffset() throws AnalysisException
{
+ // A known UTC instant: 2027-06-15 12:30:00 UTC
+ ZonedDateTime zdt = ZonedDateTime.of(2027, 6, 15, 12, 30, 0, 0,
ZoneOffset.UTC);
+ long expectedMillis = zdt.toInstant().toEpochMilli();
+
+ // +00:00 suffix — must be parsed as an instant via TZ_FORMATTER
+ String cooldownStr = "2027-06-15 12:30:00+00:00";
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(PropertyAnalyzer.PROPERTIES_STORAGE_MEDIUM, "SSD");
+ properties.put(PropertyAnalyzer.PROPERTIES_STORAGE_COOLDOWN_TIME,
cooldownStr);
+ DataProperty dp = PropertyAnalyzer.analyzeDataProperty(properties,
+ new DataProperty(TStorageMedium.SSD));
+ Assert.assertEquals("TZ offset +00:00 should parse as exact UTC
instant",
+ expectedMillis, dp.getCooldownTimeMs());
+
+ // -05:00 offset — same instant, different representation
+ String cooldownStr2 = "2027-06-15 07:30:00-05:00";
+ Map<String, String> properties2 = Maps.newHashMap();
+ properties2.put(PropertyAnalyzer.PROPERTIES_STORAGE_MEDIUM, "SSD");
+ properties2.put(PropertyAnalyzer.PROPERTIES_STORAGE_COOLDOWN_TIME,
cooldownStr2);
+ DataProperty dp2 = PropertyAnalyzer.analyzeDataProperty(properties2,
+ new DataProperty(TStorageMedium.SSD));
+ Assert.assertEquals("TZ offset -05:00 should parse as same UTC
instant",
+ expectedMillis, dp2.getCooldownTimeMs());
+ }
+
+ @Test
+ public void testStorageCooldownTimeWithTzOffsetFractionalSeconds() throws
AnalysisException {
+ // A known UTC instant with fractional seconds: 2027-06-15
12:30:00.123456 UTC
+ ZonedDateTime zdt = ZonedDateTime.of(2027, 6, 15, 12, 30, 0,
123456000, ZoneOffset.UTC);
+ long expectedMillis = zdt.toInstant().toEpochMilli();
+
+ // TIMESTAMPTZ(6) style string with +00:00 suffix
+ String cooldownStr = "2027-06-15 12:30:00.123456+00:00";
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(PropertyAnalyzer.PROPERTIES_STORAGE_MEDIUM, "SSD");
+ properties.put(PropertyAnalyzer.PROPERTIES_STORAGE_COOLDOWN_TIME,
cooldownStr);
+ DataProperty dp = PropertyAnalyzer.analyzeDataProperty(properties,
+ new DataProperty(TStorageMedium.SSD));
+ Assert.assertEquals("Fractional seconds with TZ offset should parse
correctly",
+ expectedMillis, dp.getCooldownTimeMs());
+
+ // TIMESTAMPTZ(0) — no fractional seconds, with +00:00
+ String cooldownStr2 = "2027-06-15 12:30:00+00:00";
+ Map<String, String> properties2 = Maps.newHashMap();
+ properties2.put(PropertyAnalyzer.PROPERTIES_STORAGE_MEDIUM, "SSD");
+ properties2.put(PropertyAnalyzer.PROPERTIES_STORAGE_COOLDOWN_TIME,
cooldownStr2);
+ DataProperty dp2 = PropertyAnalyzer.analyzeDataProperty(properties2,
+ new DataProperty(TStorageMedium.SSD));
+ Assert.assertEquals("No fractional seconds (precision 0) should still
parse",
+ ZonedDateTime.of(2027, 6, 15, 12, 30, 0, 0, ZoneOffset.UTC)
+ .toInstant().toEpochMilli(),
+ dp2.getCooldownTimeMs());
+ }
+
+ @Test
+ public void testStorageCooldownTimeInvalidTzFormat() throws
AnalysisException {
+ // A value that looks like it has a TZ offset but is malformed
+ // (missing minutes in offset) — should fall back to
MAX_COOLDOWN_TIME_MS
+ // instead of throwing an exception.
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(PropertyAnalyzer.PROPERTIES_STORAGE_MEDIUM, "SSD");
+ properties.put(PropertyAnalyzer.PROPERTIES_STORAGE_COOLDOWN_TIME,
"2027-06-15 12:30:00+00");
+ DataProperty dp = PropertyAnalyzer.analyzeDataProperty(properties,
+ new DataProperty(TStorageMedium.SSD));
+ Assert.assertEquals("Malformed TZ offset should fall back to MAX",
+ DataProperty.MAX_COOLDOWN_TIME_MS, dp.getCooldownTimeMs());
+ }
+
+ @Test
+ public void testStorageCooldownTimeStrictDateValidation() throws
AnalysisException {
+ // An invalid calendar date with a valid TZ offset (Feb 29 in a
+ // non-leap year) must be rejected by the STRICT resolver rather
+ // than silently normalized to Feb 28, and should fall back to
+ // MAX_COOLDOWN_TIME_MS.
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(PropertyAnalyzer.PROPERTIES_STORAGE_MEDIUM, "SSD");
+ properties.put(PropertyAnalyzer.PROPERTIES_STORAGE_COOLDOWN_TIME,
+ "2027-02-29 00:00:00+00:00");
+ DataProperty dp = PropertyAnalyzer.analyzeDataProperty(properties,
+ new DataProperty(TStorageMedium.SSD));
+ Assert.assertEquals("Invalid date (Feb 29 in non-leap year) should
fall back to MAX",
+ DataProperty.MAX_COOLDOWN_TIME_MS, dp.getCooldownTimeMs());
+
+ // Also test an impossible month (month=13)
+ Map<String, String> properties2 = Maps.newHashMap();
+ properties2.put(PropertyAnalyzer.PROPERTIES_STORAGE_MEDIUM, "SSD");
+ properties2.put(PropertyAnalyzer.PROPERTIES_STORAGE_COOLDOWN_TIME,
+ "2027-13-01 00:00:00+00:00");
+ DataProperty dp2 = PropertyAnalyzer.analyzeDataProperty(properties2,
+ new DataProperty(TStorageMedium.SSD));
+ Assert.assertEquals("Invalid month (13) should fall back to MAX",
+ DataProperty.MAX_COOLDOWN_TIME_MS, dp2.getCooldownTimeMs());
+ }
+
+ @Test
+ public void testStorageCooldownTimeDstFallbackDistinctInstants() throws
AnalysisException {
+ // On 2026-11-01 at 2:00 AM CDT, America/Chicago falls back to
+ // 1:00 AM CST. The wall-clock hour 01:00–02:00 occurs twice:
+ // 01:00 CDT = 06:00 UTC (earlier)
+ // 01:00 CST = 07:00 UTC (later)
+ // A bare DATETIME string like "2026-11-01 01:30:00" is ambiguous
+ // without an offset. With an explicit +00:00 suffix the instant
+ // parser correctly distinguishes the two.
+
+ // Earlier occurrence: 01:30 CDT = 06:30 UTC
+ String earlierStr = "2026-11-01 06:30:00+00:00";
Review Comment:
This fixed `2026-11-01` cooldown is passed through the public
`analyzeDataProperty()` path, which rejects every finite SSD cooldown at or
before `System.currentTimeMillis()`. The test therefore starts throwing after
this date even when offset parsing is correct; the positive tests above have
the same expiry with `2027-06-15`. Please isolate the parser from the
future-time policy, inject/control the clock, or derive test instants that
cannot age into the past (and derive an actual future overlap if DST semantics
are part of the assertion).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
