github-actions[bot] commented on code in PR #61918:
URL: https://github.com/apache/doris/pull/61918#discussion_r3015230239
##########
fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java:
##########
@@ -756,8 +756,14 @@ private void replaceTableInternal(Database db, OlapTable
origTable, OlapTable ne
String newTblName = newTbl.getName();
// Handle constraints for table replacement
- TableNameInfo origTableInfo = new TableNameInfo(origTable);
- TableNameInfo newTableInfo = new TableNameInfo(newTbl);
+ TableNameInfo origTableInfo = new TableNameInfo(
+ origTable.getDatabase().getCatalog().getName(),
Review Comment:
**NPE risk:** `origTable.getDatabase()` delegates to `getDbNullable()` which
can return null. This method already has a `Database db` parameter (line 752).
Use it directly to avoid the null risk and the redundant DB lookups:
```java
TableNameInfo origTableInfo = new TableNameInfo(
InternalCatalog.INTERNAL_CATALOG_NAME,
db.getFullName(),
origTable.getName());
```
This pattern is already used in `Env.renameTable()` (line 5566-5570) and is
safer.
Same issue applies to `newTbl.getDatabase()` below.
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVPartitionUtil.java:
##########
@@ -427,7 +434,12 @@ private static boolean
isSyncWithAllBaseTables(MTMVRefreshContext context, Strin
LOG.warn("get table failed, {}", baseTableInfo, e);
return false;
}
- if (isTableExcluded(excludedTriggerTables, new
TableNameInfo(table))) {
+ String tblName = table.getName();
+ if (GlobalVariable.isStoredTableNamesLowerCase()) {
+ tblName = tblName.toLowerCase();
+ }
+ if (isTableExcluded(excludedTriggerTables, new TableNameInfo(
+ table.getDatabase().getCatalog().getName(),
table.getDatabase().getFullName(), tblName))) {
Review Comment:
**NPE risk:** Same issue — `table.getDatabase()` can return null. Needs a
null guard consistent with the pattern in `ForeignKeyContext` and `MTMVService`
in this PR.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowConstraintsCommand.java:
##########
@@ -69,7 +72,13 @@ public ShowResultSetMetaData getMetaData() {
public ShowResultSet doRun(ConnectContext ctx, StmtExecutor executor)
throws Exception {
TableIf tableIf = RelationUtil.getDbAndTable(
RelationUtil.getQualifierName(ctx, nameParts), ctx.getEnv(),
Optional.empty()).value();
- TableNameInfo tableNameInfo = new TableNameInfo(tableIf);
+ DatabaseIf<?> db = tableIf.getDatabase();
Review Comment:
**NPE risk:** `tableIf.getDatabase()` can return null via `getDbNullable()`.
The next line calls `db.getCatalog()` without a null check. Consider adding a
null guard, consistent with the pattern in
`ForeignKeyContext.putAllForeignKeys()` added in this same PR.
##########
fe/fe-common/src/main/java/org/apache/doris/qe/GlobalVariable.java:
##########
@@ -92,60 +91,69 @@ public final class GlobalVariable {
public static final String ENABLE_NEW_TYPE_COERCION_BEHAVIOR
= "enable_new_type_coercion_behavior";
- @VariableMgr.VarAttr(name = VARIABLE_VERSION, flag = VariableMgr.INVISIBLE
- | VariableMgr.READ_ONLY | VariableMgr.GLOBAL)
+ @VarAttrDef.VarAttr(name = VARIABLE_VERSION, flag = VarAttrDef.INVISIBLE
+ | VarAttrDef.READ_ONLY | VarAttrDef.GLOBAL)
public static int variableVersion = CURRENT_VARIABLE_VERSION;
- @VariableMgr.VarAttr(name = VERSION_COMMENT, flag = VariableMgr.READ_ONLY)
+ @VarAttrDef.VarAttr(name = VERSION_COMMENT, flag = VarAttrDef.READ_ONLY)
public static String versionComment = Version.DORIS_BUILD_VERSION_PREFIX +
" version "
+ Version.DORIS_BUILD_VERSION + "-" +
Version.DORIS_BUILD_SHORT_HASH
+ (Config.isCloudMode() ? " (Cloud Mode)" : "");
- @VariableMgr.VarAttr(name = VERSION)
+ @VarAttrDef.VarAttr(name = VERSION)
public static String version = DEFAULT_SERVER_VERSION;
// 0: table names are stored as specified and comparisons are case
sensitive.
// 1: table names are stored in lowercase on disk and comparisons are not
case sensitive.
// 2: table names are stored as given but compared in lowercase.
- @VariableMgr.VarAttr(name = LOWER_CASE_TABLE_NAMES, flag =
VariableMgr.READ_ONLY | VariableMgr.GLOBAL)
+ @VarAttrDef.VarAttr(name = LOWER_CASE_TABLE_NAMES, flag =
VarAttrDef.READ_ONLY | VarAttrDef.GLOBAL)
public static int lowerCaseTableNames = 0;
- @VariableMgr.VarAttr(name = LICENSE, flag = VariableMgr.READ_ONLY)
+ @VarAttrDef.VarAttr(name = LICENSE, flag = VarAttrDef.READ_ONLY)
public static String license = "Apache License, Version 2.0";
- @VariableMgr.VarAttr(name = LANGUAGE, flag = VariableMgr.READ_ONLY)
+ @VarAttrDef.VarAttr(name = LANGUAGE, flag = VarAttrDef.READ_ONLY)
public static String language = "/palo/share/english/";
// A string to be executed by the server for each client that connects
- @VariableMgr.VarAttr(name = INIT_CONNECT, flag = VariableMgr.GLOBAL)
+ @VarAttrDef.VarAttr(name = INIT_CONNECT, flag = VarAttrDef.GLOBAL)
public static volatile String initConnect = "";
// A string to be executed by the server for each client that connects
- @VariableMgr.VarAttr(name = SYSTEM_TIME_ZONE, flag = VariableMgr.READ_ONLY)
- public static String systemTimeZone =
TimeUtils.getSystemTimeZone().getID();
+ @VarAttrDef.VarAttr(name = SYSTEM_TIME_ZONE, flag = VarAttrDef.READ_ONLY)
+ public static String systemTimeZone = resolveSystemTimeZone();
+
+ private static String resolveSystemTimeZone() {
Review Comment:
**Minor behavioral difference:** The old code used
`TimeUtils.getSystemTimeZone()` which resolved timezone IDs through a
comprehensive alias map (including `PRC` -> `Asia/Shanghai`, `EST` -> `-05:00`,
etc. via `ZoneId.SHORT_IDS`). This new method only handles CST.
While this is unlikely to cause issues in practice (most systems use IANA
zone IDs like `Asia/Shanghai` directly), it creates a subtle divergence:
`GlobalVariable.systemTimeZone` now uses this simplified path, while
`SessionVariable.timeZone` still uses `TimeUtils.getSystemTimeZone().getID()`.
On a system where `TimeZone.getDefault().getID()` returns `"PRC"`, the two
would report different values.
Consider documenting this intentional simplification.
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVPartitionUtil.java:
##########
@@ -103,7 +104,13 @@ public static boolean
isMTMVPartitionSync(MTMVRefreshContext refreshContext, Str
for (MTMVRelatedTableIf pctTable : pctTables) {
Set<String> relatedPartitionNames =
partitionMappings.getOrDefault(pctTable, Sets.newHashSet());
// if follow base table, not need compare with related table,
only should compare with related partition
- excludedTriggerTables.add(new TableNameInfo(pctTable));
+ String pctTblName = pctTable.getName();
+ if (GlobalVariable.isStoredTableNamesLowerCase()) {
+ pctTblName = pctTblName.toLowerCase();
+ }
+ excludedTriggerTables.add(new TableNameInfo(
+ pctTable.getDatabase().getCatalog().getName(),
Review Comment:
**NPE risk:** `pctTable.getDatabase()` can return null via
`getDbNullable()`. In contrast,
`MTMVService.shouldRefreshOnBaseTableDataChange()` in this same PR properly
guards against null `getDatabase()`. Please add null checks here, or use the
same defensive pattern as `ForeignKeyContext.putAllForeignKeys()` (also added
in this PR).
--
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]
