github-actions[bot] commented on code in PR #66761:
URL: https://github.com/apache/doris/pull/66761#discussion_r3838941920
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/SearchSignature.java:
##########
@@ -255,6 +262,28 @@ private Pair<Boolean, Pair<Integer, Integer>>
doMatchTypes(FunctionSignature sig
DataType sigArgType = sig.getArgType(i);
Expression argument = arguments.get(i);
DataType realType = argument.getDataType();
+ if (hasTimeStampNsArgument && hasOtherDateLikeArgument
Review Comment:
[P2] Preserve independent string coercions for mixed temporal arguments
This query-wide guard also rejects signatures that never choose a common
temporal representation. For example, both `concat` candidates take
VARCHAR/STRING varargs, so `concat(timestamp_ns_col, datetimev2_col)` is
rejected here even though `Concat` is explicitly castable,
TIMESTAMP_NS-to-character is admitted by `implicitCastPrimitive()`, and each
argument can be stringified without losing the other's precision. The same
applies to sibling string functions with independent arguments. Please scope
this protection to signatures that actually unify temporal values (or
explicitly exempt independent character coercions), and add mixed
TIMESTAMP_NS/DATETIMEV2 `concat`/`concat_ws` coverage.
##########
be/src/exprs/function/date_time_transforms.h:
##########
@@ -505,7 +557,8 @@ struct FromUnixTimeDecimalImpl {
if constexpr (std::is_same_v<Impl, time_format_type::UserDefinedImpl>)
{
char buf[100 + SAFE_FORMAT_STRING_MARGIN];
if (!dt.to_format_string_conservative(format.data, format.size,
buf,
- 100 +
SAFE_FORMAT_STRING_MARGIN)) {
+ 100 +
SAFE_FORMAT_STRING_MARGIN,
+ get_nanosecond(fraction))) {
Review Comment:
[P1] Format `%n` from the same instant as the calendar fields
`get_datetime_value()` rounds the decimal input to microseconds and can
carry into `epoch_second`, but this call passes the original pre-rounding
fraction to `%n`. For example, `from_unixtime(0.999999500, '%s.%n')` formats
the rounded second as `01` and the raw fraction as `999999500`, producing
`01.999999500`—a timestamp that represents neither the input instant nor the
rounded instant. Nereids folding mirrors the same split, so the parity tests
currently codify the wrong combined value. Please derive every directive from
one chosen instant (for the existing microsecond-rounded contract, pass the
rounded microseconds as nanoseconds here) and cover `%s.%n`/full-date carry in
folded and runtime paths.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/FromUnixtime.java:
##########
@@ -48,14 +48,17 @@
*/
public class FromUnixtime extends ScalarFunction
implements ExplicitlyCastableSignature, PropagateNullable,
PropagateNullLiteral, Monotonic {
+ private static final DecimalV3Type DECIMAL_MICRO_ARGUMENT_TYPE =
DecimalV3Type.createDecimalV3Type(18, 6);
+ private static final DecimalV3Type DECIMAL_NANO_ARGUMENT_TYPE =
DecimalV3Type.createDecimalV3Type(21, 9);
+
public static final List<FunctionSignature> SIGNATURES = ImmutableList.of(
FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).args(BigIntType.INSTANCE),
FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).args(BigIntType.INSTANCE,
VarcharType.SYSTEM_DEFAULT),
FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).args(BigIntType.INSTANCE,
StringType.INSTANCE),
-
FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).args(DecimalV3Type.createDecimalV3Type(18,
6)),
-
FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).args(DecimalV3Type.createDecimalV3Type(18,
6),
+
FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).args(DECIMAL_MICRO_ARGUMENT_TYPE),
+
FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).args(DECIMAL_NANO_ARGUMENT_TYPE,
Review Comment:
[P1] Apply microsecond carry before declaring `from_unixtime` monotonic
The new two-argument decimal signature preserves nanoseconds, but
execution/folding rounds those values half-up to microseconds before applying
the session timezone, while `isMonotonic()` checks DST transitions using the
raw nanosecond endpoints. In `Europe/Paris`, the raw range
`1635641999.999999000..1635641999.999999999` lies entirely before the 2021
fall-back, so it is classified monotonic; at execution the upper endpoint
carries to the transition and formats as local `02:00:00`, below the lower
endpoint's `02:59:59.999999`. That makes the monotonic metadata used by
partition/range pruning unsound even for `%Y-%m-%d %H:%i:%s`. Please round the
endpoints exactly as execution does before checking transitions (or
conservatively reject this case), and add a scale-9 carry test around a
fall-back boundary.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/SearchSignature.java:
##########
@@ -61,6 +63,11 @@ private SearchSignature(ComputeSignature computeSignature,
this.computeSignature = computeSignature;
this.signatures = signatures;
this.arguments = arguments;
+ this.hasTimeStampNsArgument = arguments.stream()
Review Comment:
[P2] Apply TIMESTAMP_NS exactness to nested collection arguments
These flags only inspect top-level argument types, so collection functions
bypass the new value-aware temporal rules. For example,
`array_contains(ARRAY<TIMESTAMP_NS>, <exact DATETIMEV2 literal>)` selects the
indexed `AnyDataType(0)` signature; `implementAnyDataTypeWithIndex()` then sees
TIMESTAMP_NS plus DATETIMEV2 only as types, finds no total common domain, and
leaves `ANY#0` unresolved until `toCatalogDataType()` throws. The literal is
exactly representable and should be promoted just as it is in scalar
comparisons/CASE. Please make indexed-Any resolution expression-aware for
nested temporal pairs (and reject nonliteral mixed domains cleanly), with
coverage for array/map contains, position/remove, and push families.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/literal/TimeV2Literal.java:
##########
@@ -314,6 +315,9 @@ protected Expression uncheckedCastTo(DataType targetType)
throws AnalysisExcepti
} else if (targetType.isDateTimeType()) {
return new DateTimeLiteral(time.getYear(), time.getMonth(),
time.getDay(), time.getHour(), time.getMinute(),
time.getSecond());
+ } else if (targetType instanceof TimeStampNsType) {
Review Comment:
[P2] Use the statement snapshot when folding TIME to TIMESTAMP_NS
This branch reuses `time`, whose date was sampled with
`LocalDateTime.now(...)` inside literal conversion. FE constant folding calls
`checkedCastTo()` directly, so a statement that starts just before local
midnight but folds after midnight can bake the next day's date into
TIME-to-TIMESTAMP_NS. The BE runtime cast instead derives the base date from
the statement's `timestamp_ms()`/`nano_seconds()` snapshot, so folded and
runtime execution disagree; the existing tests cast back to TIME and hide the
date. Please derive the FE base date from the same `ConnectContext` statement
instant (with a deterministic no-context fallback) and add a test that observes
the full TIMESTAMP_NS across a fixed midnight boundary.
--
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]
