github-actions[bot] commented on code in PR #64795:
URL: https://github.com/apache/doris/pull/64795#discussion_r3492447160
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/PartitionKey.java:
##########
@@ -147,14 +144,7 @@ private static Literal getDateTimeLiteral(String value,
Type type) throws Analys
} else if (type.isDatetimeV2()) {
return new DateTimeV2Literal(value);
} else if (type.isTimeStampTz()) {
- DateTimeV2Literal literal = new DateTimeV2Literal(value);
- DateTimeV2Literal dtV2Lit = (DateTimeV2Literal)
(DateTimeExtractAndTransform.convertTz(
- literal,
- new
StringLiteral(ConnectContext.get().getSessionVariable().timeZone),
- new StringLiteral("UTC")));
- return new TimestampTzLiteral((TimeStampTzType)
DataType.fromCatalogType(type),
- dtV2Lit.getYear(), dtV2Lit.getMonth(),
dtV2Lit.getDay(),
- dtV2Lit.getHour(), dtV2Lit.getMinute(),
dtV2Lit.getSecond(), dtV2Lit.getMicroSecond());
+ return
TimestampTzLiteral.fromSessionTimeZone((TimeStampTzType)
DataType.fromCatalogType(type), value);
Review Comment:
This fixes the range-partition path, but the LIST partition sibling path
still bypasses this helper. `createListPartitionKeyWithTypes()` below calls
`values.get(i).getValue(types.get(i))`, which goes through
`LiteralExprUtils.createLiteral()` and the legacy
`DateLiteralUtils.createDateLiteral()` TIMESTAMPTZ parser instead of
`TimestampTzLiteral`. That parser does not have the same named/lowercase
timezone behavior: for `2024-01-15 20:00:00 Asia/Shanghai`,
`getTimeZoneSplitPos()` scans back using only uppercase letters and `/`, so it
leaves the split at the end and passes an empty zone to `ZoneId.of()`;
lowercase values such as `uTc` are not detected by `haveTimeZoneName()` either.
A TIMESTAMPTZ `PARTITION BY LIST(ts)` value using the same IANA-zone syntax
accepted by the new range tests will still fail or parse differently. Please
route TIMESTAMPTZ list partition values through this same helper, or move the
corrected parser into the shared literal creation path, and add a LIST par
tition regression.
##########
fe/fe-core/src/test/java/org/apache/doris/catalog/DynamicPartitionTableTest.java:
##########
@@ -2014,4 +2018,286 @@ public void testAutoBuckets() throws Exception {
// due to partition size eq 0, use previous partition's(54th) bucket
num
Assert.assertEquals(53, partitions.get(partitions.size() -
1).getDistributionInfo().getBucketNum());
}
+
+ @Test
+ public void testTimeStampTzDynamicPartition() throws Exception {
+ // Set session timezone to something different from the partition
timezone
+ // to verify scheduler uses dynamic_partition.time_zone, not session
timezone.
+ // With time_zone = "+00:00" and session = "Asia/Shanghai" (UTC+8),
+ // a session-timezone leak would shift bounds by 8 hours (hour=16, not
00).
+ String originalTimeZone =
connectContext.getSessionVariable().getTimeZone();
+ try {
+ connectContext.getSessionVariable().setTimeZone("Asia/Shanghai");
+
+ String createOlapTblStmt = "CREATE TABLE
test.`timestamptz_dynamic_partition` (\n"
+ + " `k1` TIMESTAMPTZ NULL COMMENT \"\",\n"
+ + " `k2` int NULL COMMENT \"\"\n"
+ + ") ENGINE=OLAP\n"
+ + "DUPLICATE KEY(`k1`, `k2`)\n"
+ + "PARTITION BY RANGE(`k1`)\n"
+ + "()\n"
+ + "DISTRIBUTED BY HASH(`k2`) BUCKETS 3\n"
+ + "PROPERTIES (\n"
+ + "\"replication_num\" = \"1\",\n"
+ + "\"dynamic_partition.enable\" = \"true\",\n"
+ + "\"dynamic_partition.start\" = \"-3\",\n"
+ + "\"dynamic_partition.end\" = \"3\",\n"
+ + "\"dynamic_partition.create_history_partition\" =
\"true\",\n"
+ + "\"dynamic_partition.time_unit\" = \"day\",\n"
+ + "\"dynamic_partition.prefix\" = \"p\",\n"
+ + "\"dynamic_partition.buckets\" = \"1\",\n"
+ + "\"dynamic_partition.time_zone\" = \"+00:00\"\n"
+ + ");";
+ createTable(createOlapTblStmt);
+
+ Database db =
Env.getCurrentInternalCatalog().getDbOrAnalysisException("test");
+ OlapTable table = (OlapTable)
db.getTableOrAnalysisException("timestamptz_dynamic_partition");
+ Assert.assertTrue(table.dynamicPartitionExists());
+
+ // Execute dynamic partition scheduling
+ Env.getCurrentEnv().getDynamicPartitionScheduler()
+ .executeDynamicPartitionFirstTime(db.getId(),
table.getId());
+
+ // Verify total partitions (7 = start(-3) to end(3), inclusive)
+ int partitionCount = table.getPartitionNames().size();
+ Assert.assertEquals(7, partitionCount);
+
+ // Verify partition names are clean and partition values are UTC
timestamps
+ RangePartitionInfo partitionInfo = (RangePartitionInfo)
table.getPartitionInfo();
+ for (Map.Entry<Long, PartitionItem> entry :
partitionInfo.getIdToItem(false).entrySet()) {
+ RangePartitionItem item = (RangePartitionItem)
entry.getValue();
+
+ // Verify the partition name is clean
+ String partitionName =
table.getPartition(entry.getKey()).getName();
+ Assert.assertTrue("Partition name should start with 'p': " +
partitionName,
+ partitionName.startsWith("p"));
+ Assert.assertEquals("Partition name should be exactly 9 chars
(p + yyyyMMdd): " + partitionName,
+ 9, partitionName.length());
+
+ // Verify the range endpoints are valid and correctly ordered
+ Range<PartitionKey> range = item.getItems();
+ PartitionKey lower = range.lowerEndpoint();
+ PartitionKey upper = range.upperEndpoint();
+ Assert.assertTrue("lower must be < upper: " + range,
+ lower.compareTo(upper) < 0);
+
+ // Verify partition keys are UTC timestamps (with +00:00
suffix)
+ List<LiteralExpr> lowerKeys = lower.getKeys();
+ Assert.assertEquals(1, lowerKeys.size());
+ String lowerStr = lowerKeys.get(0).getStringValue();
+ Assert.assertTrue("Lower key must be UTC with +00:00 suffix: "
+ lowerStr,
+ lowerStr.contains("+00:00"));
+
+ List<LiteralExpr> upperKeys = upper.getKeys();
+ Assert.assertEquals(1, upperKeys.size());
+ String upperStr = upperKeys.get(0).getStringValue();
+ Assert.assertTrue("Upper key must be UTC with +00:00 suffix: "
+ upperStr,
+ upperStr.contains("+00:00"));
+
+ // With time_zone = "+00:00", partition boundaries must be
midnight UTC
+ // (hour = 00). If the scheduler incorrectly used session
timezone
+ // (Asia/Shanghai, UTC+8), the hour would be 16 instead of 00.
+ String lowerHour = lowerStr.substring(11, 13);
+ Assert.assertEquals("Lower bound hour should be 00 (midnight
UTC), proving"
+ + " dynamic_partition.time_zone was used: " + lowerStr,
+ "00", lowerHour);
+ }
+
+ for (Partition partition : table.getPartitions()) {
+ RangePartitionItem item = (RangePartitionItem)
partitionInfo.getItem(partition.getId());
+ Assert.assertNotNull("Each partition should have a range
item", item);
+ }
+ } finally {
+ connectContext.getSessionVariable().setTimeZone(originalTimeZone);
+ }
+ }
+
+ @Test
+ public void testTimeStampTzDynamicPartitionWeekUnit() throws Exception {
+ String originalTimeZone =
connectContext.getSessionVariable().getTimeZone();
+ try {
+ connectContext.getSessionVariable().setTimeZone("Europe/London");
+
+ String createOlapTblStmt = "CREATE TABLE
test.`timestamptz_dynamic_week` (\n"
+ + " `k1` TIMESTAMPTZ NULL COMMENT \"\",\n"
+ + " `k2` int NULL COMMENT \"\"\n"
+ + ") ENGINE=OLAP\n"
+ + "DUPLICATE KEY(`k1`, `k2`)\n"
+ + "PARTITION BY RANGE(`k1`)\n"
+ + "()\n"
+ + "DISTRIBUTED BY HASH(`k2`) BUCKETS 3\n"
+ + "PROPERTIES (\n"
+ + "\"replication_num\" = \"1\",\n"
+ + "\"dynamic_partition.enable\" = \"true\",\n"
+ + "\"dynamic_partition.start\" = \"-3\",\n"
+ + "\"dynamic_partition.end\" = \"3\",\n"
+ + "\"dynamic_partition.create_history_partition\" =
\"true\",\n"
+ + "\"dynamic_partition.time_unit\" = \"week\",\n"
+ + "\"dynamic_partition.prefix\" = \"p\",\n"
+ + "\"dynamic_partition.buckets\" = \"1\",\n"
+ + "\"dynamic_partition.time_zone\" = \"Asia/Tokyo\"\n"
+ + ");";
+ createTable(createOlapTblStmt);
+
+ Database db =
Env.getCurrentInternalCatalog().getDbOrAnalysisException("test");
+ OlapTable table = (OlapTable)
db.getTableOrAnalysisException("timestamptz_dynamic_week");
+ Assert.assertTrue(table.dynamicPartitionExists());
+
+ Env.getCurrentEnv().getDynamicPartitionScheduler()
+ .executeDynamicPartitionFirstTime(db.getId(),
table.getId());
+
+ int partitionCount = table.getPartitionNames().size();
+ Assert.assertEquals(7, partitionCount);
+
+ // Verify partition names follow week pattern (clean, no timezone
suffix)
+ RangePartitionInfo partitionInfo = (RangePartitionInfo)
table.getPartitionInfo();
+ for (Map.Entry<Long, PartitionItem> entry :
partitionInfo.getIdToItem(false).entrySet()) {
+ RangePartitionItem item = (RangePartitionItem)
entry.getValue();
+ String partitionName =
table.getPartition(entry.getKey()).getName();
+ Assert.assertTrue("Partition name should start with 'p': " +
partitionName,
+ partitionName.startsWith("p"));
+ // Week partition name should be like "p2026_26" (year_week)
+ Assert.assertFalse("Partition name must not contain timezone:
" + partitionName,
+ partitionName.contains("Asia") ||
partitionName.contains("Tokyo"));
+
+ // Verify range validity
+ Range<PartitionKey> range = item.getItems();
+ Assert.assertTrue("lower must be < upper",
+ range.lowerEndpoint().compareTo(range.upperEndpoint())
< 0);
+ }
+ } finally {
+ connectContext.getSessionVariable().setTimeZone(originalTimeZone);
+ }
+ }
+
+ @Test
+ public void testTimeStampTzDynamicPartitionHourUnit() throws Exception {
+ String originalTimeZone =
connectContext.getSessionVariable().getTimeZone();
+ try {
+ connectContext.getSessionVariable().setTimeZone("America/Chicago");
+
+ String createOlapTblStmt = "CREATE TABLE
test.`timestamptz_dynamic_hour` (\n"
+ + " `k1` TIMESTAMPTZ NULL COMMENT \"\",\n"
+ + " `k2` int NULL COMMENT \"\"\n"
+ + ") ENGINE=OLAP\n"
+ + "DUPLICATE KEY(`k1`, `k2`)\n"
+ + "PARTITION BY RANGE(`k1`)\n"
+ + "()\n"
+ + "DISTRIBUTED BY HASH(`k2`) BUCKETS 3\n"
+ + "PROPERTIES (\n"
+ + "\"replication_num\" = \"1\",\n"
+ + "\"dynamic_partition.enable\" = \"true\",\n"
+ + "\"dynamic_partition.start\" = \"-3\",\n"
+ + "\"dynamic_partition.end\" = \"3\",\n"
+ + "\"dynamic_partition.create_history_partition\" =
\"true\",\n"
+ + "\"dynamic_partition.time_unit\" = \"hour\",\n"
+ + "\"dynamic_partition.prefix\" = \"p\",\n"
+ + "\"dynamic_partition.buckets\" = \"1\",\n"
+ + "\"dynamic_partition.time_zone\" = \"+00:00\"\n"
+ + ");";
+ createTable(createOlapTblStmt);
+
+ Database db =
Env.getCurrentInternalCatalog().getDbOrAnalysisException("test");
+ OlapTable table = (OlapTable)
db.getTableOrAnalysisException("timestamptz_dynamic_hour");
+ Assert.assertTrue(table.dynamicPartitionExists());
+
+ Env.getCurrentEnv().getDynamicPartitionScheduler()
+ .executeDynamicPartitionFirstTime(db.getId(),
table.getId());
+
+ int partitionCount = table.getPartitionNames().size();
+ Assert.assertEquals(7, partitionCount);
+
+ // Hour partition names are "p" + yyyyMMddHH (10 chars)
+ RangePartitionInfo partitionInfo = (RangePartitionInfo)
table.getPartitionInfo();
+ for (Map.Entry<Long, PartitionItem> entry :
partitionInfo.getIdToItem(false).entrySet()) {
+ RangePartitionItem item = (RangePartitionItem)
entry.getValue();
+ String partitionName =
table.getPartition(entry.getKey()).getName();
+ Assert.assertTrue("Partition name should start with 'p': " +
partitionName,
+ partitionName.startsWith("p"));
+ // Hour partition names: p + yyyyMMddHH → length 11 (p + 10
digits)
+ Assert.assertEquals("Hour partition name length: " +
partitionName,
+ 11, partitionName.length());
+
+ // Verify range validity and UTC boundaries
+ 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"));
+ }
+ } finally {
+ connectContext.getSessionVariable().setTimeZone(originalTimeZone);
+ }
+ }
+
+ @Test
+ public void testAutoPartitionRetentionTimestampTzCutoffNormalized() throws
Exception {
+ String originalTimeZone =
connectContext.getSessionVariable().getTimeZone();
+ try {
+ // Session TZ Asia/Shanghai (UTC+8). Without normalization, the
+ // buggy cutoff would be ~8h ahead of UTC, misclassifying a
+ // partition whose upper bound is just ahead of UTC now as history.
+ connectContext.getSessionVariable().setTimeZone("Asia/Shanghai");
Review Comment:
This regression still does not exercise the background-thread case that made
the auto-retention cutoff unsafe. `beforeClass()` creates `connectContext` with
`UtFrameUtils.createDefaultCtx()`, which installs it as the thread-local
context, and this test only changes that session timezone before calling
`executeDynamicPartitionFirstTime()`. In that setup, the old suffix-free path
would format `currentTimeStr` with `DateUtils.getTimeZone()` and parse it
through `TimestampTzLiteral.fromSessionTimeZone()` using the same thread-local
`Asia/Shanghai` session, so the test can pass even if the new normalization in
`getDropPartitionOpForAutoPartition()` is removed. The production scheduler
thread normally has no `ConnectContext`: `DateUtils.getTimeZone()` falls back
to the JVM timezone, while TIMESTAMPTZ parsing falls back to UTC. Please clear
and restore the thread-local context and force and restore a non-UTC JVM
default timezone around the retention execution, or otherwise inject the sou
rce zone, so this test covers the actual no-session fallback.
--
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]
