You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2022/05/24 15:00:34 UTC
[GitHub] [incubator-doris] morningman commented on a diff in pull request #9582: [refactor](decimalv3) optimize decimal performance and precision
morningman commented on code in PR #9582:
URL: https://github.com/apache/incubator-doris/pull/9582#discussion_r880616200
##########
be/src/vec/functions/function_binary_arithmetic.h:
##########
@@ -527,6 +563,31 @@ struct DecimalBinaryOperation {
return apply(a, b, is_null);
}
}
+
+ template <bool scale_left>
+ static NativeResultType apply_scaled_mod(NativeResultType a, NativeResultType b,
+ NativeResultType scale, UInt8& is_null) {
+ if constexpr (OpTraits::is_mod) {
+ if constexpr (check_overflow) {
+ bool overflow = false;
+ if constexpr (scale_left)
+ overflow |= common::mul_overflow(a, scale, a);
+ else
+ overflow |= common::mul_overflow(b, scale, b);
+
+ if (overflow) {
+ LOG(FATAL) << "Decimal math overflow";
Review Comment:
Better not using FATAL
##########
be/src/vec/columns/column_decimal.cpp:
##########
@@ -130,6 +131,38 @@ void ColumnDecimal<T>::insert_data(const char* src, size_t /*length*/) {
data.emplace_back(tmp);
}
+template <typename T>
+void ColumnDecimal<T>::insert_many_decimalv2_data(const char* data_ptr, size_t num) {
+ for (int i = 0; i < num; i++) {
+ const char* cur_ptr = data_ptr + sizeof(decimal12_t) * i;
+ int64_t int_value = *(int64_t*)(cur_ptr);
+ int32_t frac_value = *(int32_t*)(cur_ptr + sizeof(int64_t));
+ if (config::enable_execution_decimalv3) {
+ bool is_negative = (int_value < 0 || frac_value < 0);
+ if (is_negative) {
+ int_value = std::abs(int_value);
+ frac_value = std::abs(frac_value);
+ }
+ frac_value /= (DecimalV2Value::ONE_BILLION / get_scale_multiplier());
+ T value = T(int_value) * get_scale_multiplier() + T(frac_value);
+ if (is_negative) {
+ value = -value;
+ }
+ this->insert_data(reinterpret_cast<char*>(&value), 0);
Review Comment:
Does it mean that in this method, we may insert many decimal with different precision and scale?
##########
be/src/vec/columns/column_decimal.cpp:
##########
@@ -130,6 +131,38 @@ void ColumnDecimal<T>::insert_data(const char* src, size_t /*length*/) {
data.emplace_back(tmp);
}
+template <typename T>
+void ColumnDecimal<T>::insert_many_decimalv2_data(const char* data_ptr, size_t num) {
+ for (int i = 0; i < num; i++) {
+ const char* cur_ptr = data_ptr + sizeof(decimal12_t) * i;
+ int64_t int_value = *(int64_t*)(cur_ptr);
+ int32_t frac_value = *(int32_t*)(cur_ptr + sizeof(int64_t));
+ if (config::enable_execution_decimalv3) {
+ bool is_negative = (int_value < 0 || frac_value < 0);
+ if (is_negative) {
+ int_value = std::abs(int_value);
+ frac_value = std::abs(frac_value);
+ }
+ frac_value /= (DecimalV2Value::ONE_BILLION / get_scale_multiplier());
+ T value = T(int_value) * get_scale_multiplier() + T(frac_value);
+ if (is_negative) {
+ value = -value;
+ }
+ this->insert_data(reinterpret_cast<char*>(&value), 0);
+ } else {
+ DecimalV2Value decimal_val(int_value, frac_value);
+ this->insert_data(reinterpret_cast<char*>(&decimal_val), 0);
+ }
+ }
+}
+
+template <typename T>
+void ColumnDecimal<T>::insert_many_fix_len_data(const char* data_ptr, size_t num) {
Review Comment:
where to use this method now?
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/FunctionCallExpr.java:
##########
@@ -1017,6 +1030,18 @@ public void analyzeImpl(Analyzer analyzer) throws AnalysisException {
} else {
this.type = fn.getReturnType();
}
+
+ // DECIMAL need to pass precision and scale to be
+ if (DECIMAL_FUNCTION_SET.contains(fn.getFunctionName().getFunction()) && this.type.isDecimalV2()) {
+ if (DECIMAL_SAME_TYPE_SET.contains(fnName.getFunction())) {
+ this.type = argTypes[0];
+ } else if (DECIMAL_WIDER_TYPE_SET.contains(fnName.getFunction())) {
+ this.type = ScalarType.createDecimalV2Type(ScalarType.MAX_DECIMAL128_PRECISION,
+ ((ScalarType) argTypes[0]).getScalarScale());
+ } else if (STDDEV_FUNCTION_SET.contains(fnName.getFunction())) {
+ this.type = ScalarType.createDecimalV2Type(ScalarType.MAX_DECIMAL128_PRECISION, 9);
Review Comment:
Define this `9` somewhere
##########
be/src/vec/data_types/data_type_factory.cpp:
##########
@@ -26,13 +26,75 @@ namespace doris::vectorized {
DataTypePtr DataTypeFactory::create_data_type(const doris::Field& col_desc) {
DataTypePtr nested = nullptr;
- if (col_desc.type() == OLAP_FIELD_TYPE_ARRAY) {
+ switch (col_desc.type()) {
Review Comment:
Why move this large `switch..case` out of `_create_primitive_data_type()` method?
##########
be/src/vec/functions/function_binary_arithmetic.h:
##########
@@ -449,6 +469,18 @@ struct DecimalBinaryOperation {
private:
/// there's implicit type convertion here
static NativeResultType apply(NativeResultType a, NativeResultType b) {
+ if (config::enable_execution_decimalv3) {
+ if constexpr (OpTraits::can_overflow && check_overflow) {
+ NativeResultType res;
+ if (Op::template apply<NativeResultType>(a, b, res)) {
+ LOG(FATAL) << "Decimal math overflow";
Review Comment:
Better not use FATAL log, it will cause BE crash.
Could we return a invalid value or null to indicate this error?
And in what situation can we get here?
##########
be/src/vec/sink/vtablet_sink.cpp:
##########
@@ -518,6 +518,9 @@ Status VOlapTableSink::_validate_data(RuntimeState* state, vectorized::Block* bl
break;
}
case TYPE_DECIMALV2: {
+ if (config::enable_execution_decimalv3) {
+ break;
Review Comment:
why break for v3? please add comment in code.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org