github-actions[bot] commented on code in PR #65693:
URL: https://github.com/apache/doris/pull/65693#discussion_r3593657031
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:
##########
@@ -1322,8 +1347,9 @@ public static Expression
processComparisonPredicate(ComparisonPredicate comparis
throw new AnalysisException("data type " + commonType.get()
+ " could not used in ComparisonPredicate " +
comparisonPredicate.toSql());
}
- left = castIfNotSameType(left, commonType.get());
- right = castIfNotSameType(right, commonType.get());
+ boolean losslessDecimalCast = comparisonPredicate instanceof
EqualPredicate;
+ left = castComparisonOperand(left, commonType.get(),
losslessDecimalCast);
+ right = castComparisonOperand(right, commonType.get(),
losslessDecimalCast);
Review Comment:
**[correctness] Keep conversion rejection distinct from SQL NULL.**
`NullSafeEqual` is an `EqualPredicate`, so this analyzes `(decimal_col <=>
string_col)` as `NullSafeEqual(decimal_col, LosslessCast(string_col AS
DECIMAL(38,0)))`. For a row `(NULL, '100.4')`, the cast marks the non-null
string NULL because scale 0 cannot represent it, and `eq_for_null` then returns
true because both null maps are set. The same sentinel makes `CAST(100 AS
DECIMAL(38,0)) NOT IN ('100.4')` evaluate UNKNOWN instead of true. Please
preserve source NULL separately from an inexact non-match (or use
comparison-specific failure handling), and cover both `<=>` and typed `NOT IN`
regressions.
##########
be/src/exprs/function/cast/cast_to_decimal.h:
##########
@@ -704,6 +833,33 @@ class CastToImpl<Mode, DataTypeString, ToDataType> :
public CastToBase {
RETURN_IF_ERROR(
serde->from_string_strict_mode_batch(*col_from,
*column_to, {}, null_map));
block.get_by_position(result).column = std::move(column_to);
+ } else if constexpr (Mode == CastModeType::LosslessMode) {
+ auto nullable_col_to =
create_empty_nullable_column(nested_to_type);
+ auto& nullable_to = assert_cast<ColumnNullable&>(*nullable_col_to);
+ nullable_to.resize(col_from->size());
+ auto& column_to = assert_cast<typename ToDataType::ColumnType&>(
+ nullable_to.get_nested_column());
+ auto& values_to = column_to.get_data();
+ auto& null_map_to = nullable_to.get_null_map_data();
+ auto* decimal_to_type = assert_cast<const
ToDataType*>(nested_to_type.get());
+ const auto& chars = col_from->get_chars();
+ const auto& offsets = col_from->get_offsets();
+ size_t current_offset = 0;
+ UInt32 precision = decimal_to_type->get_precision();
+ UInt32 scale = decimal_to_type->get_scale();
+ CastParameters params;
+ params.is_strict = false;
+ for (size_t i = 0; i < col_from->size(); ++i) {
+ size_t next_offset = offsets[i];
+ size_t string_size = next_offset - current_offset;
+ bool source_is_null = null_map != nullptr && null_map[i];
+ null_map_to[i] = source_is_null
Review Comment:
**[correctness] Initialize the nested decimal slot on every NULL/failure
path.** `nullable_to.resize()` reaches `PODArray::resize()`, which only
advances the end pointer. Here `source_is_null || ...` skips parsing for source
NULLs, and `from_string_lossless()` returns before writing `values_to[i]` for
rejected strings. Existing `eq` and `eq_for_null` paths evaluate the nested
decimal vectors before applying their null maps, so these rows read
indeterminate storage (UB/MSAN failure) even when the final value is NULL.
Write a decimal default before the short circuit or on both failure paths, and
add a vectorized test with source NULL and an inexact string.
##########
be/src/exprs/vcast_expr.h:
##########
@@ -44,7 +44,8 @@ class VCastExpr : public VExpr {
#ifdef BE_TEST
VCastExpr() = default;
#endif
- VCastExpr(const TExprNode& node) : VExpr(node) {}
+ VCastExpr(const TExprNode& node)
+ : VExpr(node), _lossless_decimal_cast(node.lossless_decimal_cast)
{}
Review Comment:
**[correctness] Preserve the lossless mode in prefilter clones.**
`VCastExpr::clone_node()` rebuilds from `clone_texpr_node()`, which omits this
field, while the format-v2 override recreates a cast from only its target type.
For an external row `(d=0, s='0e2147483648')`, the original lossless cast
treats zero as exact and accepts the parser's overflow status as value 0, but
the localized ordinary cast returns NULL and filters the row before
`Scanner::_filter_output_block()` can run the original residual. Copy the flag
through both clone paths or refuse localization, and add this external-file
false-negative regression.
##########
be/src/exprs/vcast_expr.h:
##########
@@ -82,6 +83,7 @@ class VCastExpr : public VExpr {
std::string _target_data_type_name;
DataTypePtr _cast_param_data_type;
+ bool _lossless_decimal_cast = false;
Review Comment:
**[correctness] Include this mode in `VCastExpr::get_digest()`.** The
current digest hashes the child/base expression and target type only, so
ordinary and lossless casts collide despite different results. For a granule
containing `(d=100, s='100.4')`, the implicit lossless predicate can cache the
granule as all-false; a later explicit `d = CAST(s AS DECIMAL(38,0))` has the
same condition-cache key even though ordinary casting rounds and should match,
so the cached bitmap suppresses the row. Hash `_lossless_decimal_cast` (or
disable condition caching for this mode) and add a digest/cache distinction
test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:
##########
@@ -1092,6 +1104,19 @@ private static Optional<DataType>
getCommonDataTypeWithFixedNumericType(
return Optional.empty();
}
+ private static DecimalV3Type getNumericStringComparisonType(NumericType
numericType) {
+ DecimalV3Type decimalType = DecimalV3Type.forType(numericType);
Review Comment:
**[correctness] Preserve the full `LARGEINT` integer range here.**
`DecimalV3Type.forType(LargeIntType)` is `DECIMAL(38,0)`, but signed LARGEINT
extrema require 39 digits. This helper therefore chooses `DECIMAL(38,0)` with
decimal256 off, or e.g. `DECIMAL(44,6)` with it on—still only 38 integer
digits. The analyzed tree casts both the LARGEINT slot and string slot to that
type, so a maximum LARGEINT and its identical text overflow to NULL instead of
matching. Derive capacity from the actual 39-digit range (with a non-narrowing
fallback when decimal256 is unavailable) and test both extrema under both
settings.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:
##########
@@ -1322,8 +1347,9 @@ public static Expression
processComparisonPredicate(ComparisonPredicate comparis
throw new AnalysisException("data type " + commonType.get()
+ " could not used in ComparisonPredicate " +
comparisonPredicate.toSql());
}
- left = castIfNotSameType(left, commonType.get());
- right = castIfNotSameType(right, commonType.get());
+ boolean losslessDecimalCast = comparisonPredicate instanceof
EqualPredicate;
Review Comment:
**[correctness] Do not use the scale-exhausted equality type for ordering.**
The new numeric/string common type also reaches `<`, `<=`, `>`, `>=`, and
BETWEEN, but this flag is enabled only for equality. With `DECIMAL(38,0)`, the
common type remains scale 0, so ordinary parsing rounds `'100.4'` to 100: `100
>= '100.4'` becomes true and `100 < '100.4'` becomes false. Decimal256 just
moves the same boundary to `DECIMAL(76,0)`. Preserve enough information to
order the fractional string without rounding, and add range/BETWEEN boundary
tests in both coercion modes.
##########
be/src/exprs/function/cast/cast_to_decimal.h:
##########
@@ -35,45 +37,172 @@ namespace doris {
value, from_type_name, precision, scale))
struct CastToDecimal {
+ static bool is_lossless_decimal_string(const char* data, size_t size,
UInt32 precision,
+ UInt32 target_scale) {
+ size_t begin = 0;
+ size_t end = size;
+ while (begin < end && std::isspace(static_cast<unsigned
char>(data[begin]))) {
+ ++begin;
+ }
+ while (begin < end && std::isspace(static_cast<unsigned char>(data[end
- 1]))) {
+ --end;
+ }
+ if (begin == end) {
+ return false;
+ }
+
+ size_t pos = begin;
+ if (data[pos] == '+' || data[pos] == '-') {
+ ++pos;
+ }
+
+ bool seen_dot = false;
+ bool seen_digit = false;
+ size_t digits_before_dot = 0;
+ size_t digits = 0;
+ size_t first_non_zero = 0;
+ size_t last_non_zero = 0;
+ while (pos < end && data[pos] != 'e' && data[pos] != 'E') {
+ unsigned char ch = static_cast<unsigned char>(data[pos]);
+ if (std::isdigit(ch)) {
+ seen_digit = true;
+ ++digits;
+ if (!seen_dot) {
+ ++digits_before_dot;
+ }
+ if (ch != '0') {
+ if (first_non_zero == 0) {
+ first_non_zero = digits;
+ }
+ last_non_zero = digits;
+ }
+ } else if (data[pos] == '.' && !seen_dot) {
+ seen_dot = true;
+ } else {
+ return false;
+ }
+ ++pos;
+ }
+ if (!seen_digit) {
+ return false;
+ }
+
+ int exponent = 0;
+ if (pos < end) {
+ ++pos;
+ bool negative_exponent = false;
+ if (pos < end && (data[pos] == '+' || data[pos] == '-')) {
+ negative_exponent = data[pos] == '-';
+ ++pos;
+ }
+ if (pos == end) {
+ return false;
+ }
+
+ constexpr int max_relevant_exponent = 1024;
+ int exponent_magnitude = 0;
+ while (pos < end) {
+ unsigned char ch = static_cast<unsigned char>(data[pos]);
+ if (!std::isdigit(ch)) {
+ return false;
+ }
+ if (exponent_magnitude <= max_relevant_exponent) {
+ exponent_magnitude = exponent_magnitude * 10 + (ch - '0');
+ if (exponent_magnitude > max_relevant_exponent) {
+ exponent_magnitude = max_relevant_exponent + 1;
+ }
+ }
+ ++pos;
+ }
+ if (exponent_magnitude > max_relevant_exponent) {
Review Comment:
**[correctness] Combine the exponent and significand offset before applying
a cap.** For example, `"0." + 1024 zeroes + "1e1025"` is only 1032 bytes and
equals exactly 1, which the existing `StringParser` accepts. This branch
rejects every nonzero exponent above 1024 before adding the opposite
`base_decimal_point`, so the new lossless equality turns that exact value into
NULL and misses a match. Use overflow-safe signed arithmetic for the combined
decimal-point position, and test large positive and negative offsets that
cancel.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java:
##########
@@ -474,6 +474,16 @@ public static Expression castIfNotSameType(Expression
input, DataType targetType
}
}
+ private static Expression castComparisonOperand(
+ Expression input, DataType targetType, boolean
losslessDecimalCast) {
+ if (losslessDecimalCast && input.getDataType().isStringLikeType()
Review Comment:
**[correctness] Apply lossless coercion recursively for complex comparison
types.** Common-type derivation recurses, so `ARRAY<DECIMAL(38,0)> =
ARRAY<STRING>` becomes an outer cast to `ARRAY<DECIMAL(38,0)>`; this
scalar-only check sees neither an outer string nor an outer decimal and creates
an ordinary cast. BE then rounds the nested `'100.4'` to 100, making `[100] =
['100.4']` true. Struct-valued IN has the same gap. Detect nested
numeric/string coercion and mark the containing cast (the BE wrapper propagates
its context), or reject these shapes until supported; add array equality and
struct-IN controls.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/ExpressionTranslator.java:
##########
@@ -449,7 +449,7 @@ public Expr visitCaseWhen(CaseWhen caseWhen,
PlanTranslatorContext context) {
public Expr visitCast(Cast cast, PlanTranslatorContext context) {
// left child of cast is expression, right child of cast is target type
CastExpr castExpr = new
CastExpr(cast.getDataType().toCatalogDataType(),
- cast.child().accept(this, context), cast.nullable());
+ cast.child().accept(this, context), cast.nullable(),
cast.isLosslessDecimalCast());
Review Comment:
**[correctness] Update the COPY INTO `visitCast` override as well.**
`CopyIntoInfo.ExpressionToExpr` analyzes file filters/column mappings with this
coercion, but then reconstructs every cast with the three-argument `CastExpr`
constructor, which defaults `losslessDecimalCast` to false. A numeric/string
file filter therefore reaches BE as ordinary CAST and can round `'100.4'` to
scale-zero 100. Forward both `isLosslessDecimalCast()` and the
implicit/explicit bit there (or delegate to this implementation), and add a
COPY INTO translation/end-to-end regression.
--
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]
