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 the issue that functions that rely on tipb::FieldType may produce incorrect results (#8483) #8497

Closed
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
75 changes: 74 additions & 1 deletion dbms/src/Flash/Coprocessor/DAGExpressionAnalyzerHelper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ String DAGExpressionAnalyzerHelper::buildCastFunctionInternal(
{
static const String tidb_cast_name = "tidb_cast";

String result_name = genFuncString(tidb_cast_name, argument_names, {nullptr});
String result_name = genFuncString(tidb_cast_name, argument_names, {nullptr}, {&field_type});
if (actions->getSampleBlock().has(result_name))
return result_name;

Expand Down Expand Up @@ -280,6 +280,79 @@ String DAGExpressionAnalyzerHelper::buildCastFunction(
return buildCastFunctionInternal(analyzer, {name, type_expr_name}, false, expr.field_type(), actions);
}

<<<<<<< HEAD
=======
String DAGExpressionAnalyzerHelper::buildSingleParamJsonRelatedFunctions(
DAGExpressionAnalyzer * analyzer,
const tipb::Expr & expr,
const ExpressionActionsPtr & actions)
{
auto func_name = getFunctionName(expr);
if unlikely (expr.children_size() != 1)
throw TiFlashException(
fmt::format("{} function only support one argument", func_name),
Errors::Coprocessor::BadRequest);
if unlikely (!exprHasValidFieldType(expr))
throw TiFlashException(
fmt::format("{} function without valid field type", func_name),
Errors::Coprocessor::BadRequest);

const auto & input_expr = expr.children(0);
String arg = analyzer->getActions(input_expr, actions);
const auto & collator = getCollatorFromExpr(expr);
String result_name = genFuncString(func_name, {arg}, {collator}, {&input_expr.field_type(), &expr.field_type()});
if (actions->getSampleBlock().has(result_name))
return result_name;

const FunctionBuilderPtr & ifunction_builder = FunctionFactory::instance().get(func_name, analyzer->getContext());
auto * function_build_ptr = ifunction_builder.get();
if (auto * function_builder = dynamic_cast<DefaultFunctionBuilder *>(function_build_ptr); function_builder)
{
auto * function_impl = function_builder->getFunctionImpl().get();
if (auto * function_cast_int_as_json = dynamic_cast<FunctionCastIntAsJson *>(function_impl);
function_cast_int_as_json)
{
function_cast_int_as_json->setInputTiDBFieldType(input_expr.field_type());
}
else if (auto * function_cast_string_as_json = dynamic_cast<FunctionCastStringAsJson *>(function_impl);
function_cast_string_as_json)
{
function_cast_string_as_json->setInputTiDBFieldType(input_expr.field_type());
function_cast_string_as_json->setOutputTiDBFieldType(expr.field_type());
}
else if (auto * function_cast_time_as_json = dynamic_cast<FunctionCastTimeAsJson *>(function_impl);
function_cast_time_as_json)
{
function_cast_time_as_json->setInputTiDBFieldType(input_expr.field_type());
}
else if (auto * function_json_unquote = dynamic_cast<FunctionJsonUnquote *>(function_impl);
function_json_unquote)
{
bool valid_check
= !(isScalarFunctionExpr(input_expr) && input_expr.sig() == tipb::ScalarFuncSig::CastJsonAsString);
function_json_unquote->setNeedValidCheck(valid_check);
}
else if (auto * function_cast_json_as_string = dynamic_cast<FunctionCastJsonAsString *>(function_impl);
function_cast_json_as_string)
{
function_cast_json_as_string->setOutputTiDBFieldType(expr.field_type());
}
else
{
throw Exception(fmt::format("Unexpected func {} in buildSingleParamJsonRelatedFunctions", func_name));
}
}
else
{
throw Exception(fmt::format("Unexpected func {} in buildSingleParamJsonRelatedFunctions", func_name));
}

const ExpressionAction & action = ExpressionAction::applyFunction(ifunction_builder, {arg}, result_name, collator);
actions->add(action);
return result_name;
}

>>>>>>> 3a72d53dcb (Fix the issue that functions that rely on `tipb::FieldType` may produce incorrect results (#8483))
template <typename Impl>
String DAGExpressionAnalyzerHelper::buildDateAddOrSubFunction(
DAGExpressionAnalyzer * analyzer,
Expand Down
14 changes: 13 additions & 1 deletion dbms/src/Flash/Coprocessor/DAGUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1403,7 +1403,11 @@ SortDescription getSortDescription(
return order_descr;
}

String genFuncString(const String & func_name, const Names & argument_names, const TiDB::TiDBCollators & collators)
String genFuncString(
const String & func_name,
const Names & argument_names,
const TiDB::TiDBCollators & collators,
const std::vector<const tipb::FieldType *> & field_types)
{
FmtBuffer buf;
buf.fmtAppend("{}({})_collator", func_name, fmt::join(argument_names.begin(), argument_names.end(), ", "));
Expand All @@ -1415,6 +1419,14 @@ String genFuncString(const String & func_name, const Names & argument_names, con
buf.append("_0");
}
buf.append(" ");
buf.joinStr(
field_types.begin(),
field_types.end(),
[](const auto & field_type, FmtBuffer & buffer) {
if likely (field_type)
buffer.fmtAppend("{}|{}", field_type->flag(), field_type->flen());
},
", ");
return buf.toString();
}

Expand Down
6 changes: 5 additions & 1 deletion dbms/src/Flash/Coprocessor/DAGUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,11 @@ DataTypePtr inferDataType4Literal(const tipb::Expr & expr);
SortDescription getSortDescription(
const std::vector<NameAndTypePair> & order_columns,
const google::protobuf::RepeatedPtrField<tipb::ByItem> & by_items);
String genFuncString(const String & func_name, const Names & argument_names, const TiDB::TiDBCollators & collators);
String genFuncString(
const String & func_name,
const Names & argument_names,
const TiDB::TiDBCollators & collators,
const std::vector<const tipb::FieldType *> & field_types = {});

extern const Int8 VAR_SIZE;

Expand Down
44 changes: 44 additions & 0 deletions tests/fullstack-test/expr/issue_8482.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# Copyright 2023 PingCAP, Inc.
#
# Licensed 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.

mysql> drop table if exists test.t;
mysql> create table test.t(b json);
mysql> alter table test.t set tiflash replica 1;
mysql> insert into test.t values (true);

func> wait_table test t

mysql> set tidb_allow_mpp=1;set tidb_enforce_mpp=1; set tidb_isolation_read_engines='tiflash'; select b = true from test.t;
+----------+
| b = true |
+----------+
| 0 |
+----------+

mysql> set tidb_allow_mpp=1;set tidb_enforce_mpp=1; set tidb_isolation_read_engines='tiflash'; select b = 1 from test.t;
+-------+
| b = 1 |
+-------+
| 1 |
+-------+

mysql> set tidb_allow_mpp=1;set tidb_enforce_mpp=1; set tidb_isolation_read_engines='tiflash'; select b = true, b = 1 from test.t;
+----------+-------+
| b = true | b = 1 |
+----------+-------+
| 0 | 1 |
+----------+-------+

# Clean up.
mysql> drop table if exists test.t;