morrySnow commented on code in PR #65219:
URL: https://github.com/apache/doris/pull/65219#discussion_r3575861778
##########
fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java:
##########
@@ -409,10 +422,25 @@ public static DataProperty
analyzeDataProperty(Map<String, String> properties, f
}
} else if (key.equalsIgnoreCase(PROPERTIES_STORAGE_COOLDOWN_TIME))
{
try {
- DateLiteral dateLiteral =
DateLiteralUtils.createDateLiteral(value,
- ScalarType.getDefaultDateType(Type.DATETIME));
- cooldownTimestamp =
dateLiteral.unixTimestamp(TimeUtils.getTimeZone());
- } catch (AnalysisException e) {
+ // A value with an explicit timezone offset (e.g.
+ // "2027-01-01 00:00:00+00:00" or, for TIMESTAMPTZ(6),
+ // "2027-01-01 00:00:00.000000+00:00") represents an
+ // unambiguous UTC instant. Parse it directly as a
+ // ZonedDateTime and convert to epoch millis, bypassing
+ // the DATETIME-based DateLiteralUtils.createDateLiteral()
+ // path which uses Instant.now() for the initial offset
+ // computation and produces incorrect results across DST
+ // boundaries.
+ if (value.length() >= 6
+ && value.substring(value.length() -
6).matches("[+-]\\d{2}:\\d{2}")) {
+ cooldownTimestamp = TZ_FORMATTER.parse(value,
ZonedDateTime::from)
+ .toInstant().toEpochMilli();
+ } else {
+ DateLiteral dateLiteral =
DateLiteralUtils.createDateLiteral(value,
+ ScalarType.getDefaultDateType(Type.DATETIME));
+ cooldownTimestamp =
dateLiteral.unixTimestamp(TimeUtils.getTimeZone());
+ }
+ } catch (Exception e) {
Review Comment:
**Issue (correctness):** `catch (AnalysisException e)` was broadened to
`catch (Exception e)`, which silently swallows ALL exceptions from both the new
`TZ_FORMATTER.parse()` path and the old `DateLiteralUtils.createDateLiteral()`
fallback.
If an unexpected `DateTimeParseException`, `NullPointerException`, or any
other runtime exception occurs in the new parse path, it will be silently
caught and cooldown will default to `MAX_COOLDOWN_TIME_MS` with only a
`LOG.warn` — the operator gets no indication of the actual error.
Suggestion: Either restore `catch (AnalysisException e)` and wrap
`TZ_FORMATTER.parse()` in its own try/catch that rethrows as
`AnalysisException`, or narrow the catch to `catch (AnalysisException |
DateTimeParseException e)`.
##########
fe/fe-core/src/main/java/org/apache/doris/clone/DynamicPartitionScheduler.java:
##########
@@ -282,7 +308,20 @@ private ArrayList<AddPartitionOp>
getAddPartitionOp(Database db, OlapTable olapT
ArrayList<AddPartitionOp> addPartitionOps = new ArrayList<>();
DynamicPartitionProperty dynamicPartitionProperty =
olapTable.getTableProperty().getDynamicPartitionProperty();
RangePartitionInfo rangePartitionInfo = (RangePartitionInfo)
olapTable.getPartitionInfo();
- ZonedDateTime now =
ZonedDateTime.now(dynamicPartitionProperty.getTimeZone().toZoneId());
+ // For TIMESTAMPTZ, both partition boundaries and names are UTC-based
+ // (00:00—24:00 in UTC). This keeps partition names and values
consistent.
+ boolean isTimestampTz = partitionColumn.getDataType() ==
PrimitiveType.TIMESTAMPTZ;
+ // nowTz: configured-timezone clock, used for storage properties
+ ZonedDateTime nowTz =
ZonedDateTime.now(dynamicPartitionProperty.getTimeZone().toZoneId());
+ // now: used for both border computation and naming. For TIMESTAMPTZ,
UTC-based.
+ // Derived from nowTz via withZoneSameInstant to guarantee both
+ // represent the exact same instant with a single system clock read.
+ ZonedDateTime now = isTimestampTz ?
nowTz.withZoneSameInstant(ZoneOffset.UTC) : nowTz;
+ // For TIMESTAMPTZ, borders and names are already UTC-based;
+ // convertToUtcTimestamp should not shift them further.
+ TimeZone borderTimeZone = isTimestampTz
Review Comment:
**Suggestion (reuse):** `TimeZone.getTimeZone("UTC")` used here (and
identically at line 628 in `getDropPartitionOpForDynamic`). The existing
utility `TimeUtils.getUTCTimeZone()` already provides a cached UTC `TimeZone`
reference, avoiding hardcoded `"UTC"` strings in two places.
##########
fe/fe-core/src/main/java/org/apache/doris/clone/DynamicPartitionScheduler.java:
##########
@@ -372,23 +417,44 @@ private ArrayList<AddPartitionOp>
getAddPartitionOp(Database db, OlapTable olapT
dynamicPartitionProperty.getReplicaAllocation().toCreateStmt());
}
- // set storage_medium and storage_cooldown_time based on
dynamic_partition.hot_partition_num
+ // set storage_medium and storage_cooldown_time based on
dynamic_partition.hot_partition_num.
+ // Use `now` (UTC-based for TIMESTAMPTZ) so the cooldown boundary
+ // aligns with the partition's UTC range, not the configured
timezone.
setStorageMediumProperty(partitionProperties,
dynamicPartitionProperty, now, hotPartitionNum, idx);
if (StringUtils.isNotEmpty(storagePolicyName)) {
setStoragePolicyProperty(partitionProperties,
dynamicPartitionProperty, now, idx, storagePolicyName);
}
+ // For TIMESTAMPTZ, cooldown / baseTime are UTC datetime strings
+ // (e.g. "2027-01-01 00:00:00") from the UTC-based `now` clock.
+ // Append an explicit +00:00 suffix so PropertyAnalyzer can detect
+ // this as an unambiguous UTC instant and parse it directly instead
+ // of going through the DATETIME path with Instant.now()-based
offsets.
+ if (isTimestampTz) {
Review Comment:
**Suggestion (fragile pattern):** After `setStorageMediumProperty()` and
`setStoragePolicyProperty()` write `PROPERTIES_STORAGE_COOLDOWN_TIME` /
`PROPERTIES_DATA_BASE_TIME` as bare datetime strings, this separate `if
(isTimestampTz)` block reads each property back out of the `HashMap`, calls
`convertToUtcTimestamp()`, and puts them back.
Since `convertToUtcTimestamp()` is a no-op for non-TIMESTAMPTZ columns, it
could be called unconditionally inside `setStorageMediumProperty()` and
`setStoragePolicyProperty()` before the `put()`. This eliminates the 13-line
post-processing block and the risk that a future property addition (or key
rename) is silently missed.
##########
fe/fe-core/src/main/java/org/apache/doris/clone/DynamicPartitionScheduler.java:
##########
@@ -282,7 +308,20 @@ private ArrayList<AddPartitionOp>
getAddPartitionOp(Database db, OlapTable olapT
ArrayList<AddPartitionOp> addPartitionOps = new ArrayList<>();
DynamicPartitionProperty dynamicPartitionProperty =
olapTable.getTableProperty().getDynamicPartitionProperty();
RangePartitionInfo rangePartitionInfo = (RangePartitionInfo)
olapTable.getPartitionInfo();
- ZonedDateTime now =
ZonedDateTime.now(dynamicPartitionProperty.getTimeZone().toZoneId());
+ // For TIMESTAMPTZ, both partition boundaries and names are UTC-based
+ // (00:00—24:00 in UTC). This keeps partition names and values
consistent.
+ boolean isTimestampTz = partitionColumn.getDataType() ==
PrimitiveType.TIMESTAMPTZ;
Review Comment:
**Suggestion (maintainability):** This 7-line block (`isTimestampTz`,
`nowTz`, `now`, `borderTimeZone` computation) is duplicated verbatim at line
624 in `getDropPartitionOpForDynamic()`. Any future method needing
TIMESTAMPTZ-aware time setup must copy-paste again, risking inconsistency.
Consider extracting a small private helper that encapsulates the single
clock read + UTC derivation + border timezone selection.
`convertToUtcTimestamp` already inspects `column.getDataType()` internally and
could potentially absorb the timezone decision entirely, eliminating the
per-call-site duplication.
##########
fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java:
##########
@@ -409,10 +422,25 @@ public static DataProperty
analyzeDataProperty(Map<String, String> properties, f
}
} else if (key.equalsIgnoreCase(PROPERTIES_STORAGE_COOLDOWN_TIME))
{
try {
- DateLiteral dateLiteral =
DateLiteralUtils.createDateLiteral(value,
- ScalarType.getDefaultDateType(Type.DATETIME));
- cooldownTimestamp =
dateLiteral.unixTimestamp(TimeUtils.getTimeZone());
- } catch (AnalysisException e) {
+ // A value with an explicit timezone offset (e.g.
+ // "2027-01-01 00:00:00+00:00" or, for TIMESTAMPTZ(6),
+ // "2027-01-01 00:00:00.000000+00:00") represents an
+ // unambiguous UTC instant. Parse it directly as a
+ // ZonedDateTime and convert to epoch millis, bypassing
+ // the DATETIME-based DateLiteralUtils.createDateLiteral()
+ // path which uses Instant.now() for the initial offset
+ // computation and produces incorrect results across DST
+ // boundaries.
+ if (value.length() >= 6
Review Comment:
**Nit (efficiency):** `value.substring(value.length() -
6).matches("[+-]\\d{2}:\\d{2}")` allocates a substring AND compiles the regex
`Pattern` on every invocation (`String.matches()` doesn't cache).
Suggestion: Use a static precompiled pattern:
```java
private static final Pattern TZ_OFFSET_PATTERN =
Pattern.compile("[+-]\\d{2}:\\d{2}$");
```
Then check with `TZ_OFFSET_PATTERN.matcher(value).find()` — this avoids the
`substring()` allocation and the guard (`$` anchor naturally returns false for
short strings).
##########
fe/fe-core/src/main/java/org/apache/doris/clone/DynamicPartitionScheduler.java:
##########
@@ -296,23 +335,29 @@ private ArrayList<AddPartitionOp>
getAddPartitionOp(Database db, OlapTable olapT
int hotPartitionNum = dynamicPartitionProperty.getHotPartitionNum();
String storagePolicyName = dynamicPartitionProperty.getStoragePolicy();
+ // Partition naming uses the same `now` clock as border computation,
+ // so names and values are based on the same timezone (UTC for
TIMESTAMPTZ).
String nowPartitionPrevBorder =
DynamicPartitionUtil.getPartitionRangeString(
dynamicPartitionProperty, now, 0, partitionFormat);
String nowPartitionName = dynamicPartitionProperty.getPrefix()
- +
DynamicPartitionUtil.getFormattedPartitionName(dynamicPartitionProperty.getTimeZone(),
+ +
DynamicPartitionUtil.getFormattedPartitionName(borderTimeZone,
nowPartitionPrevBorder,
dynamicPartitionProperty.getTimeUnit());
+ // UTC-based lower bound of the current partition, used to identify the
+ // current partition by range in getHistoricalPartitions().
+ String currentUtcBorder = DynamicPartitionUtil.getPartitionRangeString(
Review Comment:
**Nit (efficiency):** `currentUtcBorder` is initialized via
`getPartitionRangeString(property, now, 0, partitionFormat)` — the identical
call used at line 340 to compute `nowPartitionPrevBorder`. This duplicates the
date-bounds computation (formatting, date arithmetic, object allocations inside
`getPartitionRangeString`).
Suggestion: Replace with:
```java
String currentUtcBorder = convertToUtcTimestamp(partitionColumn,
nowPartitionPrevBorder, borderTimeZone);
```
This reuses the already-computed `nowPartitionPrevBorder`.
--
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]
