Skip to content

Commit

Permalink
[fix](unary function) Fix wrong result of asin, acos and sqrt when pr…
Browse files Browse the repository at this point in the history
…ocessing invalid input #40267 (#40358)

cherry pick from #40267
  • Loading branch information
zhiqiang-hhhh authored Sep 5, 2024
1 parent 961d2c9 commit 0928c9c
Show file tree
Hide file tree
Showing 8 changed files with 291 additions and 11 deletions.
94 changes: 94 additions & 0 deletions be/src/vec/functions/function_math_unary_alway_nullable.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
// 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.

#pragma once

#include "vec/columns/column.h"
#include "vec/columns/column_decimal.h"
#include "vec/columns/column_nullable.h"
#include "vec/columns/columns_number.h"
#include "vec/core/call_on_type_index.h"
#include "vec/core/types.h"
#include "vec/data_types/data_type_decimal.h"
#include "vec/data_types/data_type_nullable.h"
#include "vec/data_types/data_type_number.h"
#include "vec/functions/function.h"
#include "vec/functions/function_helpers.h"
#include "vec/utils/util.hpp"

namespace doris::vectorized {

template <typename Impl>
class FunctionMathUnaryAlwayNullable : public IFunction {
public:
using IFunction::execute;

static constexpr auto name = Impl::name;
static FunctionPtr create() { return std::make_shared<FunctionMathUnaryAlwayNullable>(); }

private:
String get_name() const override { return name; }
size_t get_number_of_arguments() const override { return 1; }

DataTypePtr get_return_type_impl(const DataTypes& arguments) const override {
return make_nullable(std::make_shared<typename Impl::Type>());
}

static void execute_in_iterations(const double* src_data, double* dst_data, size_t size) {
for (size_t i = 0; i < size; i++) {
Impl::execute(&src_data[i], &dst_data[i]);
}
}

Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments,
size_t result, size_t input_rows_count) const override {
const ColumnFloat64* col =
assert_cast<const ColumnFloat64*>(block.get_by_position(arguments[0]).column.get());
auto dst = ColumnFloat64::create();
auto& dst_data = dst->get_data();
dst_data.resize(input_rows_count);

execute_in_iterations(col->get_data().data(), dst_data.data(), input_rows_count);

auto result_null_map = ColumnUInt8::create(input_rows_count, 0);

for (size_t i = 0; i < input_rows_count; i++) {
if (Impl::is_invalid_input(col->get_data()[i])) [[unlikely]] {
result_null_map->get_data().data()[i] = 1;
}
}

block.replace_by_position(
result, ColumnNullable::create(std::move(dst), std::move(result_null_map)));
return Status::OK();
}
};

template <typename Name, Float64(Function)(Float64)>
struct UnaryFunctionPlainAlwayNullable {
using Type = DataTypeFloat64;
static constexpr auto name = Name::name;

static constexpr bool is_invalid_input(Float64 x) { return Name::is_invalid_input(x); }

template <typename T, typename U>
static void execute(const T* src, U* dst) {
*dst = static_cast<Float64>(Function(*src));
}
};

} // namespace doris::vectorized
16 changes: 13 additions & 3 deletions be/src/vec/functions/math.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include "vec/functions/function_const.h"
#include "vec/functions/function_math_log.h"
#include "vec/functions/function_math_unary.h"
#include "vec/functions/function_math_unary_alway_nullable.h"
#include "vec/functions/function_string.h"
#include "vec/functions/function_totype.h"
#include "vec/functions/function_unary_arithmetic.h"
Expand All @@ -51,13 +52,19 @@ struct Log2Impl;
namespace doris::vectorized {
struct AcosName {
static constexpr auto name = "acos";
// https://dev.mysql.com/doc/refman/8.4/en/mathematical-functions.html#function_acos
static constexpr bool is_invalid_input(Float64 x) { return x < -1 || x > 1; }
};
using FunctionAcos = FunctionMathUnary<UnaryFunctionPlain<AcosName, std::acos>>;
using FunctionAcos =
FunctionMathUnaryAlwayNullable<UnaryFunctionPlainAlwayNullable<AcosName, std::acos>>;

struct AsinName {
static constexpr auto name = "asin";
// https://dev.mysql.com/doc/refman/8.4/en/mathematical-functions.html#function_asin
static constexpr bool is_invalid_input(Float64 x) { return x < -1 || x > 1; }
};
using FunctionAsin = FunctionMathUnary<UnaryFunctionPlain<AsinName, std::asin>>;
using FunctionAsin =
FunctionMathUnaryAlwayNullable<UnaryFunctionPlainAlwayNullable<AsinName, std::asin>>;

struct AtanName {
static constexpr auto name = "atan";
Expand Down Expand Up @@ -221,8 +228,11 @@ using FunctionSin = FunctionMathUnary<UnaryFunctionPlain<SinName, std::sin>>;

struct SqrtName {
static constexpr auto name = "sqrt";
// https://dev.mysql.com/doc/refman/8.4/en/mathematical-functions.html#function_sqrt
static constexpr bool is_invalid_input(Float64 x) { return x < 0; }
};
using FunctionSqrt = FunctionMathUnary<UnaryFunctionPlain<SqrtName, std::sqrt>>;
using FunctionSqrt =
FunctionMathUnaryAlwayNullable<UnaryFunctionPlainAlwayNullable<SqrtName, std::sqrt>>;

struct CbrtName {
static constexpr auto name = "cbrt";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@

import org.apache.doris.catalog.FunctionSignature;
import org.apache.doris.nereids.trees.expressions.Expression;
import org.apache.doris.nereids.trees.expressions.functions.AlwaysNullable;
import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature;
import org.apache.doris.nereids.trees.expressions.functions.PropagateNullable;
import org.apache.doris.nereids.trees.expressions.shape.UnaryExpression;
import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor;
import org.apache.doris.nereids.types.DoubleType;
Expand All @@ -34,7 +34,7 @@
* ScalarFunction 'acos'. This class is generated by GenerateFunction.
*/
public class Acos extends ScalarFunction
implements UnaryExpression, ExplicitlyCastableSignature, PropagateNullable {
implements UnaryExpression, ExplicitlyCastableSignature, AlwaysNullable {

public static final List<FunctionSignature> SIGNATURES = ImmutableList.of(
FunctionSignature.ret(DoubleType.INSTANCE).args(DoubleType.INSTANCE)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@

import org.apache.doris.catalog.FunctionSignature;
import org.apache.doris.nereids.trees.expressions.Expression;
import org.apache.doris.nereids.trees.expressions.functions.AlwaysNullable;
import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature;
import org.apache.doris.nereids.trees.expressions.functions.PropagateNullable;
import org.apache.doris.nereids.trees.expressions.shape.UnaryExpression;
import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor;
import org.apache.doris.nereids.types.DoubleType;
Expand All @@ -34,7 +34,7 @@
* ScalarFunction 'asin'. This class is generated by GenerateFunction.
*/
public class Asin extends ScalarFunction
implements UnaryExpression, ExplicitlyCastableSignature, PropagateNullable {
implements UnaryExpression, ExplicitlyCastableSignature, AlwaysNullable {

public static final List<FunctionSignature> SIGNATURES = ImmutableList.of(
FunctionSignature.ret(DoubleType.INSTANCE).args(DoubleType.INSTANCE)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@

import org.apache.doris.catalog.FunctionSignature;
import org.apache.doris.nereids.trees.expressions.Expression;
import org.apache.doris.nereids.trees.expressions.functions.AlwaysNullable;
import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature;
import org.apache.doris.nereids.trees.expressions.functions.PropagateNullable;
import org.apache.doris.nereids.trees.expressions.shape.UnaryExpression;
import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor;
import org.apache.doris.nereids.types.DoubleType;
Expand All @@ -34,7 +34,7 @@
* ScalarFunction 'dsqrt'. This class is generated by GenerateFunction.
*/
public class Dsqrt extends ScalarFunction
implements UnaryExpression, ExplicitlyCastableSignature, PropagateNullable {
implements UnaryExpression, ExplicitlyCastableSignature, AlwaysNullable {

public static final List<FunctionSignature> SIGNATURES = ImmutableList.of(
FunctionSignature.ret(DoubleType.INSTANCE).args(DoubleType.INSTANCE)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@

import org.apache.doris.catalog.FunctionSignature;
import org.apache.doris.nereids.trees.expressions.Expression;
import org.apache.doris.nereids.trees.expressions.functions.AlwaysNullable;
import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature;
import org.apache.doris.nereids.trees.expressions.functions.PropagateNullable;
import org.apache.doris.nereids.trees.expressions.shape.UnaryExpression;
import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor;
import org.apache.doris.nereids.types.DoubleType;
Expand All @@ -34,7 +34,7 @@
* ScalarFunction 'sqrt'. This class is generated by GenerateFunction.
*/
public class Sqrt extends ScalarFunction
implements UnaryExpression, ExplicitlyCastableSignature, PropagateNullable {
implements UnaryExpression, ExplicitlyCastableSignature, AlwaysNullable {

public static final List<FunctionSignature> SIGNATURES = ImmutableList.of(
FunctionSignature.ret(DoubleType.INSTANCE).args(DoubleType.INSTANCE)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
-- This file is automatically generated. You should know what you did if you want to edit this
-- !acos_1 --
\N true

-- !acos_2 --
\N true

-- !acos_3 --
\N true 0
\N true 1
\N true 2
\N true 3
\N true 4
\N true 5
\N true 6
\N true 7
\N true 8
\N true 9

-- !asin_1 --
\N true

-- !asin_2 --
\N true

-- !asin_3 --
\N true 0
\N true 1
\N true 2
\N true 3
\N true 4
\N true 5
\N true 6
\N true 7
\N true 8
\N true 9

-- !sqrt_1 --
\N true

-- !sqrt_2 --
\N true

-- !sqrt_3 --
\N true 0
\N true 1
\N true 2
\N true 3
\N true 4
\N true 5
\N true 6
\N true 7
\N true 8
\N true 9

-- !acos_tbl_1 --
1 \N true
2 \N true
3 1.5707963267948966 false
4 \N true
5 \N true
6 \N true
7 \N true
8 \N true

-- !asin_tbl_1 --
1 \N true
2 \N true
3 0.0 false
4 \N true
5 \N true
6 \N true
7 \N true
8 \N true

-- !sqrt_tbl_1 --
1 1.0488088481701516 false
2 \N true
3 0.0 false
4 \N true
5 \N true
6 \N true
7 \N true
8 \N true

-- !dsqrt_tbl_1 --
1 1.0488088481701516 false
2 \N true
3 0.0 false
4 \N true
5 \N true
6 \N true
7 \N true
8 \N true

Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// 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_math_unary_alway_nullable") {
qt_acos_1 """
select acos(1.1), acos(1.1) is null;
"""
qt_acos_2 """
select acos(-1.1), acos(-1.1) is null;
"""
qt_acos_3 """
select acos(-1.1), acos(-1.1) is NULL, number from numbers("number"="10")
"""

qt_asin_1 """
select asin(1.1), asin(1.1) is null;
"""
qt_asin_2 """
select asin(-1.1), asin(-1.1) is null;
"""
qt_asin_3 """
select asin(-1.1), asin(-1.1) is NULL, number from numbers("number"="10")
"""

qt_sqrt_1 """
select sqrt(-1), sqrt(-1) is null;
"""
qt_sqrt_2 """
select sqrt(-1.1), sqrt(-1.1) is null;
"""
qt_sqrt_3 """
select sqrt(-1.1), sqrt(-1.1) is NULL, number from numbers("number"="10")
"""

sql "drop table if exists test_math_unary_alway_nullable"

sql """
create table if not exists test_math_unary_alway_nullable (rowid int, val double NULL)
distributed by hash(rowid) properties ("replication_num"="1");
"""

sql """
insert into test_math_unary_alway_nullable values
(1, 1.1), (2, -1.1), (3, 0), (4, NULL)
"""
sql """
insert into test_math_unary_alway_nullable values
(5, NULL), (6, NULL), (7, NULL), (8, NULL)
"""

qt_acos_tbl_1 """
select rowid, acos(val), acos(val) is null from test_math_unary_alway_nullable order by rowid;
"""

qt_asin_tbl_1 """
select rowid, asin(val), asin(val) is null from test_math_unary_alway_nullable order by rowid;
"""

qt_sqrt_tbl_1 """
select rowid, sqrt(val), sqrt(val) is null from test_math_unary_alway_nullable order by rowid;
"""

qt_dsqrt_tbl_1 """
select rowid, dsqrt(val), dsqrt(val) is null from test_math_unary_alway_nullable order by rowid;
"""

}

0 comments on commit 0928c9c

Please sign in to comment.