Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix](decimalv2) fix decimalv2 agg errors (#29246) #29189

Merged
merged 1 commit into from
Dec 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 16 additions & 18 deletions be/src/util/string_parser.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#include "common/compiler_util.h" // IWYU pragma: keep
#include "common/status.h"
#include "runtime/large_int_value.h"
#include "runtime/primitive_type.h"
#include "vec/common/int_exp.h"
#include "vec/data_types/data_type_decimal.h"

Expand Down Expand Up @@ -158,7 +159,8 @@ class StringParser {
return string_to_bool_internal(s + i, len - i, result);
}

template <PrimitiveType P, typename T>
template <PrimitiveType P, typename T,
typename DecimalType = PrimitiveTypeTraits<P>::ColumnType::value_type>
static inline T string_to_decimal(const char* s, int len, int type_precision, int type_scale,
ParseResult* result);

Expand Down Expand Up @@ -568,7 +570,7 @@ inline int StringParser::StringParseTraits<__int128>::max_ascii_len() {
return 39;
}

template <PrimitiveType P, typename T>
template <PrimitiveType P, typename T, typename DecimalType>
T StringParser::string_to_decimal(const char* s, int len, int type_precision, int type_scale,
ParseResult* result) {
static_assert(
Expand Down Expand Up @@ -646,10 +648,9 @@ T StringParser::string_to_decimal(const char* s, int len, int type_precision, in
value = (value * 10) + (c - '0'); // Benchmarks are faster with parenthesis...
} else {
*result = StringParser::PARSE_OVERFLOW;
value = is_negative ? vectorized::min_decimal_value<vectorized::Decimal<T>>(
type_precision)
: vectorized::max_decimal_value<vectorized::Decimal<T>>(
type_precision);
value = is_negative
? vectorized::min_decimal_value<DecimalType>(type_precision)
: vectorized::max_decimal_value<DecimalType>(type_precision);
return value;
}
DCHECK(value >= 0); // For some reason //DCHECK_GE doesn't work with __int128.
Expand Down Expand Up @@ -696,10 +697,9 @@ T StringParser::string_to_decimal(const char* s, int len, int type_precision, in
cur_digit = precision - scale;
} else if (!found_dot && max_digit < (precision - scale)) {
*result = StringParser::PARSE_OVERFLOW;
value = is_negative ? vectorized::min_decimal_value<vectorized::Decimal<T>>(
type_precision)
: vectorized::max_decimal_value<vectorized::Decimal<T>>(
type_precision);
value = is_negative
? vectorized::min_decimal_value<DecimalType>(type_precision)
: vectorized::max_decimal_value<DecimalType>(type_precision);
return value;
} else if (found_dot && scale >= type_scale && !has_round) {
// make rounding cases
Expand Down Expand Up @@ -739,11 +739,10 @@ T StringParser::string_to_decimal(const char* s, int len, int type_precision, in
if (!is_numeric_ascii(c)) {
if (cur_digit > type_precision) {
*result = StringParser::PARSE_OVERFLOW;
value = is_negative
? vectorized::min_decimal_value<vectorized::Decimal<T>>(
type_precision)
: vectorized::max_decimal_value<vectorized::Decimal<T>>(
type_precision);
value = is_negative ? vectorized::min_decimal_value<DecimalType>(
type_precision)
: vectorized::max_decimal_value<DecimalType>(
type_precision);
return value;
}
return is_negative ? T(-value) : T(value);
Expand Down Expand Up @@ -782,9 +781,8 @@ T StringParser::string_to_decimal(const char* s, int len, int type_precision, in
*result = StringParser::PARSE_OVERFLOW;
if constexpr (TYPE_DECIMALV2 != P) {
// decimalv3 overflow will return max min value for type precision
value = is_negative
? vectorized::min_decimal_value<vectorized::Decimal<T>>(type_precision)
: vectorized::max_decimal_value<vectorized::Decimal<T>>(type_precision);
value = is_negative ? vectorized::min_decimal_value<DecimalType>(type_precision)
: vectorized::max_decimal_value<DecimalType>(type_precision);
return value;
}
} else if (UNLIKELY(scale > type_scale)) {
Expand Down
2 changes: 1 addition & 1 deletion be/src/vec/core/decimal_comparison.h
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ class DecimalComparison {
}

static bool compare(A a, B b, UInt32 scale_a, UInt32 scale_b) {
static const UInt32 max_scale = max_decimal_precision<Decimal128>();
static const UInt32 max_scale = max_decimal_precision<Decimal128I>();
if (scale_a > max_scale || scale_b > max_scale) {
LOG(FATAL) << "Bad scale of decimal field";
}
Expand Down
23 changes: 18 additions & 5 deletions be/src/vec/data_types/data_type_decimal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -166,11 +166,12 @@ bool DataTypeDecimal<T>::parse_from_string(const std::string& str, T* res) const
}

DataTypePtr create_decimal(UInt64 precision_value, UInt64 scale_value, bool use_v2) {
if (precision_value < min_decimal_precision() ||
precision_value > max_decimal_precision<Decimal128>()) {
auto max_precision =
use_v2 ? max_decimal_precision<Decimal128>() : max_decimal_precision<Decimal128I>();
if (precision_value < min_decimal_precision() || precision_value > max_precision) {
throw doris::Exception(doris::ErrorCode::NOT_IMPLEMENTED_ERROR,
"Wrong precision {}, min: {}, max: {}", precision_value,
min_decimal_precision(), max_decimal_precision<Decimal128>());
min_decimal_precision(), max_precision);
}

if (static_cast<UInt64>(scale_value) > precision_value) {
Expand Down Expand Up @@ -228,10 +229,16 @@ Int64 max_decimal_value<Decimal64>(UInt32 precision) {
}
template <>
Int128 max_decimal_value<Decimal128>(UInt32 precision) {
return DecimalV2Value::get_max_decimal().value() /
DataTypeDecimal<Decimal128>::get_scale_multiplier(
(UInt64)max_decimal_precision<Decimal128>() - precision);
}
template <>
Int128 max_decimal_value<Decimal128I>(UInt32 precision) {
return (static_cast<int128_t>(999999999999999999ll) * 100000000000000000ll * 1000ll +
static_cast<int128_t>(99999999999999999ll) * 1000ll + 999ll) /
DataTypeDecimal<Decimal128>::get_scale_multiplier(
(UInt64)max_decimal_precision<Decimal128>() - precision);
(UInt64)max_decimal_precision<Decimal128I>() - precision);
}

template <typename T>
Expand All @@ -249,9 +256,15 @@ Int64 min_decimal_value<Decimal64>(UInt32 precision) {
(UInt64)max_decimal_precision<Decimal64>() - precision);
}
template <>
Int128 min_decimal_value<Decimal128>(UInt32 precision) {
Int128 min_decimal_value<Decimal128I>(UInt32 precision) {
return -(static_cast<int128_t>(999999999999999999ll) * 100000000000000000ll * 1000ll +
static_cast<int128_t>(99999999999999999ll) * 1000ll + 999ll) /
DataTypeDecimal<Decimal128>::get_scale_multiplier(
(UInt64)max_decimal_precision<Decimal128I>() - precision);
}
template <>
Int128 min_decimal_value<Decimal128>(UInt32 precision) {
return DecimalV2Value::get_min_decimal().value() /
DataTypeDecimal<Decimal128>::get_scale_multiplier(
(UInt64)max_decimal_precision<Decimal128>() - precision);
}
Expand Down
25 changes: 1 addition & 24 deletions be/src/vec/data_types/data_type_decimal.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,36 +85,13 @@ constexpr size_t max_decimal_precision<Decimal64>() {
}
template <>
constexpr size_t max_decimal_precision<Decimal128>() {
return 38;
return 27;
}
template <>
constexpr size_t max_decimal_precision<Decimal128I>() {
return 38;
}

template <typename T>
constexpr typename T::NativeType max_decimal_value() {
return 0;
}
template <>
constexpr Int32 max_decimal_value<Decimal32>() {
return 999999999;
}
template <>
constexpr Int64 max_decimal_value<Decimal64>() {
return 999999999999999999;
}
template <>
constexpr Int128 max_decimal_value<Decimal128>() {
return static_cast<int128_t>(999999999999999999ll) * 100000000000000000ll * 1000ll +
static_cast<int128_t>(99999999999999999ll) * 1000ll + 999ll;
}
template <>
constexpr Int128 max_decimal_value<Decimal128I>() {
return static_cast<int128_t>(999999999999999999ll) * 100000000000000000ll * 1000ll +
static_cast<int128_t>(99999999999999999ll) * 1000ll + 999ll;
}

DataTypePtr create_decimal(UInt64 precision, UInt64 scale, bool use_v2);

inline UInt32 least_decimal_precision_for(TypeIndex int_type) {
Expand Down
2 changes: 1 addition & 1 deletion be/src/vec/sink/vtablet_sink.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1900,7 +1900,7 @@ Status VOlapTableSink::_validate_column(RuntimeState* state, const TypeDescripto
break;
}
case TYPE_DECIMAL128I: {
CHECK_VALIDATION_FOR_DECIMALV3(Decimal128I, Decimal128);
CHECK_VALIDATION_FOR_DECIMALV3(Decimal128I, Decimal128I);
break;
}
case TYPE_ARRAY: {
Expand Down
19 changes: 19 additions & 0 deletions regression-test/data/datatype_p0/decimalv2/test_decimalv2_agg.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
-- This file is automatically generated. You should know what you did if you want to edit this
-- !decimalv2_min --
-123456789012345678.901234567

-- !decimalv2_max --
123456789012345678.901234567

-- !decimalv2_count --
3

-- !decimalv2_sum --
0.012345679

-- !decimalv2_avg --
0.004115226

-- !decimalv2_sum2 --
1000000000000000000.000000000

Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,7 @@
11.99990
837.43444

-- !decimalv2_insert --
999999999999999999.999999999 1.000000000
-999999999999999999.999999999 2.000000000

4 changes: 4 additions & 0 deletions regression-test/data/datatype_p0/decimalv3/test_load.out
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,7 @@
0.000135039891190100
0.000390160098269360

-- !decimalv3_insert --
-99999999999999999999999999999999.999999 4.000000
99999999999999999999999999999999.999999 3.000000

Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// 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.

suite("test_decimalv2_agg", "nonConcurrent") {

sql """
admin set frontend config("enable_decimal_conversion" = "false");
"""

sql """
drop table if exists test_decimalv2_agg;
"""
sql """
create table test_decimalv2_agg (
k1 decimalv2(27,9)
)distributed by hash(k1)
properties("replication_num"="1");
"""
sql """
insert into test_decimalv2_agg values
(123456789012345678.901234567),
(-123456789012345678.901234567),
(0.012345679),
(NULL);
"""
qt_decimalv2_min " select min(k1) from test_decimalv2_agg; "
qt_decimalv2_max " select max(k1) from test_decimalv2_agg; "
qt_decimalv2_count " select count(k1) from test_decimalv2_agg; "
qt_decimalv2_sum " select sum(k1) from test_decimalv2_agg; "
qt_decimalv2_avg " select avg(k1) from test_decimalv2_agg; "

// test overflow
sql """
drop table if exists test_decimalv2_agg;
"""
sql """
create table test_decimalv2_agg (
k1 decimalv2(27,9)
)distributed by hash(k1)
properties("replication_num"="1");
"""
sql """
insert into test_decimalv2_agg values
(999999999999999999.999999999),
(0.000000001),
(NULL);
"""
qt_decimalv2_sum2 " select sum(k1) from test_decimalv2_agg; "

}
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,64 @@ suite("test_decimalv2_load", "nonConcurrent") {
select * from ${tableName2} order by 1;
"""

sql """
drop table if exists test_decimalv2_insert;
"""
sql """
CREATE TABLE `test_decimalv2_insert` (
`k1` decimalv2(27, 9) null,
`k2` decimalv2(27, 9) null
)
DISTRIBUTED BY HASH(`k1`) BUCKETS 10
PROPERTIES (
"replication_num" = "1"
);
"""
sql "set enable_insert_strict=true;"
test {
sql """
insert into test_decimalv2_insert values("999999999999999999999999999999",1);
"""
exception "Invalid"
}
test {
sql """
insert into test_decimalv2_insert values("-999999999999999999999999999999",2);
"""
exception "Invalid"
}
test {
sql """
insert into test_decimalv2_insert values("999999999999999999.9999999991", 3);
"""
exception "Invalid"
}
test {
sql """
insert into test_decimalv2_insert values("-999999999999999999.9999999991",4);
"""
exception "Invalid"
}
test {
sql """
insert into test_decimalv2_insert values("999999999999999999.9999999995",5);
"""
exception "Invalid"
}
test {
sql """
insert into test_decimalv2_insert values("-999999999999999999.9999999995",6);
"""
exception "Invalid"
}
sql """
insert into test_decimalv2_insert values("999999999999999999.999999999", 1);
"""
sql """
insert into test_decimalv2_insert values("-999999999999999999.999999999", 2);
"""
qt_decimalv2_insert "select * from test_decimalv2_insert order by 2; "

sql """
admin set frontend config("enable_decimal_conversion" = "true");
"""
Expand Down
Loading
Loading