morrySnow commented on code in PR #64795:
URL: https://github.com/apache/doris/pull/64795#discussion_r3489176315
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:
##########
@@ -655,27 +653,13 @@ 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,
Review Comment:
## `TimeStampTzType.MAX` Reference Equality Check
```java
if (timeStampTzType.getScale() < 0 || dataType == TimeStampTzType.MAX) {
timeStampTzType = TimeStampTzType.forTypeFromString(value);
}
```
The check `dataType == TimeStampTzType.MAX` uses object identity (`==`).
While `TimeStampTzType.MAX` is a `static final` singleton,
`TimeStampTzType.of(6)` creates a **new instance** every time:
```java
public static TimeStampTzType of(int scale) {
if (scale == SYSTEM_DEFAULT.scale) return SYSTEM_DEFAULT;
else return new TimeStampTzType(scale); // new instance even for scale=6
}
```
This means a column defined as `TIMESTAMPTZ(6)` would have
`TimeStampTzType.of(6)` (not `MAX`), so the `forTypeFromString` call is
skipped. Is this the intended behavior? If `MAX` is meant to represent "derive
from input," consider using `instanceof` or a `boolean isWildcard()` helper. If
the distinction is deliberate, a comment explaining why would be helpful.
Also: the removed `if (DateTimeChecker.hasTimeZone(value))` check previously
split the code into two paths (with-tz vs without-tz). The new unified
`fromSessionTimeZone` handles both cases internally, which is cleaner. Just
confirming the scale handling is identical in both paths — the test coverage
looks good for this.
--
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]
