github-actions[bot] commented on code in PR #61918:
URL: https://github.com/apache/doris/pull/61918#discussion_r3014081105
##########
fe/fe-common/src/main/java/org/apache/doris/qe/GlobalVariable.java:
##########
@@ -92,60 +91,60 @@ 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 =
java.util.TimeZone.getDefault().getID();
Review Comment:
**[Medium] Behavioral change in timezone resolution**
The old code used `TimeUtils.getSystemTimeZone().getID()`, which applies
Doris-specific timezone alias resolution via `timeZoneAliasMap` (e.g., `CST` →
`Asia/Shanghai`, `PRC` → `Asia/Shanghai`). The new code
`java.util.TimeZone.getDefault().getID()` bypasses this alias resolution.
On systems where the JVM default timezone uses a short code like `CST`, the
old code would report `system_time_zone = Asia/Shanghai` while the new code
would report `system_time_zone = CST`. In Java's standard library, `CST` maps
to `America/Chicago` (UTC-6), not `Asia/Shanghai` (UTC+8).
Since `GlobalVariable` is now in `fe-common` and `TimeUtils` is in
`fe-core`, you can't use `TimeUtils` directly. Consider one of:
1. Move the timezone alias resolution logic to a utility in `fe-common`
2. Use a static initializer block that duplicates the critical aliases
3. Accept this as a known difference and document it
Note: This only affects the read-only `system_time_zone` session variable
display, not actual time computations.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnToProtobuf.java:
##########
@@ -0,0 +1,131 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.catalog;
+
+import org.apache.doris.common.Config;
+import org.apache.doris.common.DdlException;
+import org.apache.doris.proto.OlapFile;
+import org.apache.doris.proto.OlapFile.PatternTypePB;
+import org.apache.doris.thrift.TPrimitiveType;
+
+import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
+import com.google.protobuf.ByteString;
+
+import java.util.List;
+import java.util.Set;
+
+public class ColumnToProtobuf {
+ public static OlapFile.ColumnPB toPb(Column column, Set<String> bfColumns,
List<Index> indexes)
+ throws DdlException {
+ OlapFile.ColumnPB.Builder builder = OlapFile.ColumnPB.newBuilder();
+
+ // when doing schema change, some modified column has a prefix in name.
+ // this prefix is only used in FE, not visible to BE, so we should
remove this prefix.
+ String name = column.getName();
+ builder.setName(name.startsWith(Column.SHADOW_NAME_PREFIX)
+ ? name.substring(Column.SHADOW_NAME_PREFIX.length()) : name);
+
+ builder.setUniqueId(column.getUniqueId());
+ builder.setType(column.getDataType().toThrift().name());
+ builder.setIsKey(column.isKey());
+ if (column.getFieldPatternType() != null) {
+ if (column.getFieldPatternType() == PatternType.MATCH_NAME) {
+ builder.setPatternType(PatternTypePB.MATCH_NAME);
+ } else {
+ builder.setPatternType(PatternTypePB.MATCH_NAME_GLOB);
+ }
Review Comment:
**[Low] Fragile else-branch for PatternType mapping**
Unlike `ColumnToThrift.toThriftPatternType()` which uses a `switch` with a
`default` that throws `IllegalArgumentException`, this code uses an if/else
that silently maps any future `PatternType` enum value to `MATCH_NAME_GLOB`.
Consider using a consistent pattern:
```java
if (column.getFieldPatternType() != null) {
switch (column.getFieldPatternType()) {
case MATCH_NAME:
builder.setPatternType(PatternTypePB.MATCH_NAME);
break;
case MATCH_NAME_GLOB:
builder.setPatternType(PatternTypePB.MATCH_NAME_GLOB);
break;
default:
throw new IllegalArgumentException("Unknown pattern type: " +
column.getFieldPatternType());
}
}
```
This ensures fail-fast behavior if new enum values are added.
--
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]
