morrySnow commented on code in PR #64026:
URL: https://github.com/apache/doris/pull/64026#discussion_r3418608079
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:
##########
@@ -655,27 +653,15 @@ public static Optional<Expression>
characterLiteralTypeCoercion(String value, Da
&& DateTimeChecker.isValidDateTime(value)) {
ret = DateTimeLiteral.parseDateTimeLiteral(value,
true).orElse(null);
} else if (dataType.isTimeStampTzType() &&
DateTimeChecker.isValidDateTime(value)) {
- if (DateTimeChecker.hasTimeZone(value)) {
- // Signature search can pass TIMESTAMPTZ(*) here.
TimestampTzLiteral rounds by scale,
- // so derive a concrete scale from the literal before
preserving its explicit offset.
- TimeStampTzType timeStampTzType = (TimeStampTzType)
dataType;
- if (timeStampTzType.getScale() < 0) {
- timeStampTzType =
TimeStampTzType.forTypeFromString(value);
- }
- ret = new TimestampTzLiteral(timeStampTzType, value);
- } else {
- DateTimeV2Literal dtV2Lit = (DateTimeV2Literal)
DateTimeLiteral
- .parseDateTimeLiteral(value,
true).orElse(null);
- if (dtV2Lit != null) {
- dtV2Lit = (DateTimeV2Literal)
(DateTimeExtractAndTransform.convertTz(
- dtV2Lit,
- new
StringLiteral(ConnectContext.get().getSessionVariable().timeZone),
- new StringLiteral("UTC")));
- ret = new TimestampTzLiteral(dtV2Lit.getYear(),
dtV2Lit.getMonth(), dtV2Lit.getDay(),
- dtV2Lit.getHour(), dtV2Lit.getMinute(),
dtV2Lit.getSecond(),
- dtV2Lit.getMicroSecond());
- }
+ // Signature search can pass TIMESTAMPTZ(*) here.
TimestampTzLiteral rounds by scale,
+ // so derive a concrete scale from the literal before parsing.
+ TimeStampTzType timeStampTzType = (TimeStampTzType) dataType;
+ if (timeStampTzType.getScale() < 0) {
+ timeStampTzType = TimeStampTzType.forTypeFromString(value);
+ } else if (dataType == TimeStampTzType.MAX &&
!DateTimeChecker.hasTimeZone(value)) {
+ timeStampTzType = TimeStampTzType.forTypeFromString(value);
Review Comment:
⚠️ **Reference equality check for `TimeStampTzType.MAX`**: `dataType ==
TimeStampTzType.MAX` uses Java reference equality. This only works if there is
exactly one canonical `TimeStampTzType.MAX` instance. If any code creates
`TimeStampTzType(9)` separately (e.g., through deserialization or
`DataType.fromCatalogType()`), this check fails, and the scale is incorrectly
overridden.
建议：使用 `dataType.equals(TimeStampTzType.MAX)` 或
`((TimeStampTzType)dataType).getScale() < 0` 替代引用相等性检查。
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/TimestampTzLiteral.java:
##########
@@ -92,72 +129,7 @@ public String toString() {
@Override
public String getStringValue() {
Review Comment:
⚠️ **Potential infinite recursion risk**: The new `getStringValue()`
delegates to `toLegacyLiteral().getStringValue()`. If `toLegacyLiteral()`
internally (or through `DateLiteral`'s `getStringValue()`) calls back to
`TimestampTzLiteral.getStringValue()`, this would cause infinite recursion and
stack overflow.
Please verify the complete call chain:
`TimestampTzLiteral.toLegacyLiteral()` → `DateLiteral`'s serialization paths —
make sure they don't depend on `getStringValue()` round-tripping back.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/StepPartition.java:
##########
@@ -83,4 +95,12 @@ public List<Expression> getFromExpression() {
public List<Expression> getToExpression() {
return toExpression;
}
+
+ private void validateBoundaryValueCount() {
+ if (fromExpression.size() > partitionTypes.size() ||
toExpression.size() > partitionTypes.size()) {
+ throw new AnalysisException("partition column number in multi
partition clause must be one but start "
+ + "column size is " + fromExpression.size() + ", end
column size is " + toExpression.size()
Review Comment:
🔧 **Confusing error message**: The message says "partition column number in
multi partition clause **must be one** but start column size is X, end column
size is Y." This strongly implies only single-column partition is supported,
which is incorrect. The actual constraint is that boundary value count must not
exceed partition column count.
建议改为: `"Number of partition boundary values (%d start, %d end) exceeds
partition column count (%d)"`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/AlterMultiPartitionOp.java:
##########
@@ -117,12 +128,33 @@ private String getPartitionValuesStr(List<Expression>
values) {
return sb.toString();
}
- private PartitionValue toLegacyPartitionValue(Expression e) {
- if (e.isLiteral()) {
- return new PartitionValue(((Literal) e).getStringValue(),
e.isNullLiteral());
- } else if (e instanceof PartitionDefinition.MaxValue) {
+ private PartitionValue toLegacyPartitionValue(Expression typedExpression) {
+ if (typedExpression.isLiteral()) {
+ return new PartitionValue(((Literal)
typedExpression).toLegacyLiteral(), typedExpression.isNullLiteral(),
+ typedExpression.isNullLiteral() ? null : ((Literal)
typedExpression).getStringValue());
+ } else if (typedExpression instanceof PartitionDefinition.MaxValue) {
return PartitionValue.MAX_VALUE;
}
throw new AnalysisException("Unsupported partition value");
}
+
+ private void ensurePartitionTypesInitialized() {
+ if (partitionTypes == null) {
+ throw new AnalysisException("partitionTypes should be initialized
before validating partition definition");
+ }
+ }
+
+ private Expression typedExpression(Expression expression, int index) {
+ ensurePartitionTypesInitialized();
+ return PartitionDefinition.strictTypedPartitionExpression(expression,
partitionTypes.get(index));
+ }
+
+ private void validateBoundaryValueCount() {
+ ensurePartitionTypesInitialized();
+ if (fromExpression.size() > partitionTypes.size() ||
toExpression.size() > partitionTypes.size()) {
+ throw new AnalysisException("partition column number in multi
partition clause must be one but start "
+ + "column size is " + fromExpression.size() + ", end
column size is " + toExpression.size()
Review Comment:
🔧 **Same confusing error message as `StepPartition`**: "must be one" implies
single-column only. 建议与 `StepPartition.validateBoundaryValueCount()`
一起改为更清晰的描述：`"Number of partition boundary values (%d start, %d end) exceeds
partition column count (%d)"`.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/LiteralExprUtils.java:
##########
@@ -85,6 +93,51 @@ public static LiteralExpr createLiteral(String value, Type
type) throws Analysis
return literalExpr;
}
+ private static LiteralExpr createFloatingPointLiteral(String value, Type
type) throws AnalysisException {
+ try {
+ return new FloatLiteral(Double.parseDouble(value), type);
+ } catch (NumberFormatException e) {
+ throw new AnalysisException("Invalid floating-point literal: " +
value, e);
+ }
+ }
+
+ private static LiteralExpr createDecimalLiteral(String value, Type type)
throws AnalysisException {
+ Preconditions.checkArgument(type instanceof ScalarType,
+ String.format("%s's type %s is not ScalarType", value, type));
+ ScalarType scalarType = (ScalarType) type;
+ BigDecimal decimalValue;
+ try {
+ decimalValue = new BigDecimal(value);
+ } catch (NumberFormatException e) {
+ throw new AnalysisException("Invalid decimal literal: " + value,
e);
+ }
+ // Reject partition values that cannot be exactly represented at the
column's
+ // scale. Trailing zeros (e.g. '10.000' for DECIMAL(10,2)) are exact
and pass;
+ // non-zero digits beyond the target scale (e.g. '10.005' for
DECIMAL(10,2))
+ // would be silently rounded by the old code and now throw an
AnalysisException.
+ try {
+ decimalValue = decimalValue.setScale(scalarType.getScalarScale(),
RoundingMode.UNNECESSARY);
+ } catch (ArithmeticException e) {
Review Comment:
✅ **Good: Stricter DECIMAL validation**. Using `RoundingMode.UNNECESSARY`
catches values that would be silently rounded.
⚠️ However, this is a **breaking behavioral change**: a partition value like
`'10.001'` for `DECIMAL(10,2)` was previously silently rounded to `10.00`; now
it throws `AnalysisException`. Consider mentioning this in release notes with
the migration path.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/PartitionValue.java:
##########
@@ -76,13 +74,15 @@ public boolean equals(Object o) {
return false;
}
PartitionValue that = (PartitionValue) o;
- return isNullPartition == that.isNullPartition
- && Objects.equal(value, that.value);
+ boolean result = isNullPartition == that.isNullPartition
+ && Objects.equal(value, that.value)
Review Comment:
⚠️ **`equals()`/`hashCode()` contract change**: `stringValue` now
participates in equality. Two `PartitionValue` objects with the same
`LiteralExpr` value but different `stringValue` are now **unequal**. For
`MAX_VALUE`, `stringValue` is `"MAXVALUE"` — were there callers relying on
`equals()` matching based on `LiteralExpr` value alone?
请确认依赖 `PartitionValue.equals()` 的去重/比较逻辑不会因此产生行为差异。
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/DateTimeLiteral.java:
##########
@@ -203,8 +202,13 @@ protected void roundMicroSecond(int scale) {
this.microSecond = Math.round(this.microSecond / factor) * (int)
factor;
if (this.microSecond >= 1000000) {
- LocalDateTime localDateTime =
DateUtils.getTime(StandardDateFormat.DATE_TIME_FORMATTER_TO_MICRO_SECOND,
- getStringValue()).plusSeconds(1);
+ LocalDateTime localDateTime = LocalDateTime.of(
+ (int) this.year,
Review Comment:
⚠️ **`roundMicroSecond` overflow at year 9999 boundary**: The new code uses
`LocalDateTime.of(year, month, day, hour, minute, second).plusSeconds(1)`. At
the boundary case `9999-12-31 23:59:59.999999`, the seconds overflow would
advance to year 10000, causing `LocalDateTime.of()` to throw
`DateTimeException`.
Note that the old code had the same issue via `getStringValue()` reparse.
But since this PR's purpose is to fix exactly this class of timezone-unsafe
reparsing bug, consider adding a `catch (DateTimeException)` here that clamps
to the maximum supported date to be fully robust.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/PartitionKey.java:
##########
@@ -564,7 +532,20 @@ public PartitionKey deserialize(JsonElement json,
java.lang.reflect.Type typeOfT
key = MaxLiteral.MAX_VALUE;
}
if (type != PrimitiveType.DATETIMEV2 && type !=
PrimitiveType.TIMESTAMPTZ) {
- key.setType(Type.fromPrimitiveType(type));
+ // Preserve the literal's type from the deserialized JSON
payload
+ // when it carries parameterized information
(precision/scale/length).
Review Comment:
⚠️ **Type preservation edge case with wildcard types**: When
`existingType.getPrimitiveType() == type` but `existingType` is a wildcard type
(e.g., `VARCHAR(-1)` with `len=-1`), the parameterized type information is
silently lost — the primitive type matches, so the fallback to
`Type.fromPrimitiveType(type)` is skipped.
建议：额外检查 `existingType` 是否包含有效的参数化信息（如 for `VARCHAR`: `len > 0`），如果含有
wildcard 值则仍然回退。
##########
fe/fe-core/src/main/java/org/apache/doris/planner/ScanNode.java:
##########
@@ -367,6 +382,64 @@ public static ColumnRanges expressionToRanges(Expr expr,
}
}
+ /**
+ * Re-type a literal value to the partition column's type when the two
+ * primitive types belong to the same family (fixed-point, date, string,
etc.)
+ * but differ in detail (e.g. INT vs BIGINT, DATE vs DATETIMEV2, STRING vs
+ * VARCHAR). This is necessary because {@link ColumnBound} delegates to
+ * {@link PartitionKey#compareLiteralExpr} which rejects cross-type
+ * comparisons. For example, a predicate {@code CAST(k AS BIGINT) = 1} on
an
+ * INT partition column produces a BIGINT-typed literal that would fail to
+ * compare against INT partition bounds.
+ *
+ * <p>The re-parse is a best-effort: if {@code value} already has the exact
Review Comment:
💡 **Silent fallback on normalization failure**: When
`LiteralExprUtils.createLiteral()` throws, the method silently returns the
original value. A failed normalization means `compareLiteral` will return -1
(cross-type rejected) and partition pruning opportunities will be missed.
建议：添加 `LOG.debug("Failed to normalize partition filter literal from {} to
{}, pruning may be incomplete", value.getStringValue(), columnType)`
帮助排查潜在的剪枝失效。
##########
fe/fe-core/src/main/java/org/apache/doris/clone/DynamicPartitionScheduler.java:
##########
@@ -298,16 +300,23 @@ private ArrayList<AddPartitionOp>
getAddPartitionOp(Database db, OlapTable olapT
nowPartitionPrevBorder,
dynamicPartitionProperty.getTimeUnit());
for (; idx <= dynamicPartitionProperty.getEnd(); idx++) {
- String prevBorder = DynamicPartitionUtil.getPartitionRangeString(
- dynamicPartitionProperty, now, idx, partitionFormat);
- String nextBorder = DynamicPartitionUtil.getPartitionRangeString(
- dynamicPartitionProperty, now, idx + 1, partitionFormat);
- PartitionValue lowerValue = new PartitionValue(prevBorder);
- PartitionValue upperValue = new PartitionValue(nextBorder);
-
boolean isPartitionExists = false;
Range<PartitionKey> addPartitionKeyRange;
+ PartitionValue lowerValue;
+ PartitionValue upperValue;
Review Comment:
🔧 **Overly broad try-catch**: The `try` block now encompasses
`prevBorder`/`nextBorder` creation, `normalizePartitionValueString`, and
`createLiteral`. If `normalizePartitionValueString` throws `AnalysisException`,
the catch at the bottom sets `isPartitionExists = true` and skips the partition
— even though it may genuinely not exist.
建议：将 normalization/creation 与 range 创建拆分为独立的 try-catch，或至少在 catch 块中区分
normalization 错误与 range 错误并记录日志。
--
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]
