From fa9f18553a4f01f9d22fa2c2381ae083865c6549 Mon Sep 17 00:00:00 2001 From: Alexander Barkov Date: Thu, 16 Sep 2021 13:27:39 +0400 Subject: [PATCH] MCOL-4728 Query with unusual use of aggregate functions on ColumnStore table crashes MariaDB Server After an AggreateColumn corresponding to SUM(1+1) is created, it is pushed to the list: gwi.count_asterisk_list.push_back(ac) Later, in getSelectPlan(), the expression SUM(1+1) was erroneously treated as a constant: if (!hasNonSupportItem && !nonConstFunc(ifp) && !(parseInfo & AF_BIT) && tmpVec.size() == 0) { srcp.reset(buildReturnedColumn(item, gwi, gwi.fatalParseError)); This code freed the original AggregateColumn and replaced to a ConstantColumn. But gwi.count_asterisk_list still pointer to the freed AggregateColumn(). The expression SUM(1+1) was treated as a constant because tmpVec was empty due to a bug in this code: // special handling for count(*). This should not be treated as constant. if (isp->argument_count() == 1 && ( sfitempp[0]->type() == Item::CONST_ITEM && (sfitempp[0]->cmp_type() == INT_RESULT || sfitempp[0]->cmp_type() == STRING_RESULT || sfitempp[0]->cmp_type() == REAL_RESULT || sfitempp[0]->cmp_type() == DECIMAL_RESULT) ) ) { field_vec.push_back((Item_field*)item); //dummy Notice, it handles only aggregate functions with explicit literals passed as an argument, while it does not handle constant expressions such as 1+1. Fix: - Adding new classes ConstantColumnNull, ConstantColumnString, ConstantColumnNum, ConstantColumnUInt, ConstantColumnSInt, ConstantColumnReal, ValStrStdString, to reuse the code easier. - Moving a part of the code from the case branch handling CONST_ITEM in buildReturnedColumn() into a new function newConstantColumnNotNullUsingValNativeNoTz(). This makes the code easier to read and to reuse in the future. - Adding a new function newConstantColumnMaybeNullFromValStrNoTz(). Removing dulplicate code from !!!four!!! places, using the new function instead. - Adding a function isSupportedAggregateWithOneConstArg() to properly catch all constant expressions. Using the new function parse_item() in the code commented as "special handling for count(*)". Now it pushes all constant expressions to field_vec, not only explicit literals. - Moving a part of the code from buildAggregateColumn() to a helper function processAggregateColumnConstArg(). Using processAggregateColumnConstArg() in the CONST_ITEM and NULL_ITEM branches. - Adding a new branch in buildReturnedColumn() handling FUNC_ITEM. If a function has constant arguments, a ConstantColumn() is immediately created, without going to buildArithmeticColumn()/buildFunctionColumn(). - Reusing isSupportedAggregateWithOneConstArg() and processAggregateColumnConstArg() in buildAggregateColumn(). A new branch catches aggregate function has only one constant argument and immediately creates a single ConstantColumn without traversing to the argument sub-components. --- dbcon/execplan/constantcolumn.h | 77 +++ dbcon/mysql/ha_mcs_execplan.cpp | 576 ++++++++++-------- dbcon/mysql/ha_mcs_impl_if.h | 2 +- .../columnstore/bugfixes/mcol-4728.result | 63 ++ .../columnstore/bugfixes/mcol-4728.test | 38 ++ mysql-test/columnstore/bugfixes/suite.opt | 1 + mysql-test/columnstore/bugfixes/suite.pm | 23 + 7 files changed, 519 insertions(+), 261 deletions(-) create mode 100644 mysql-test/columnstore/bugfixes/mcol-4728.result create mode 100644 mysql-test/columnstore/bugfixes/mcol-4728.test create mode 100644 mysql-test/columnstore/bugfixes/suite.opt create mode 100644 mysql-test/columnstore/bugfixes/suite.pm diff --git a/dbcon/execplan/constantcolumn.h b/dbcon/execplan/constantcolumn.h index 5be0051ef..33a02fdd2 100644 --- a/dbcon/execplan/constantcolumn.h +++ b/dbcon/execplan/constantcolumn.h @@ -375,6 +375,83 @@ public: }; + +class ConstantColumnNull: public ConstantColumn +{ +public: + ConstantColumnNull() + :ConstantColumn("", ConstantColumn::NULLDATA) + { } +}; + + +class ConstantColumnString: public ConstantColumn +{ +public: + ConstantColumnString(const std::string &str) + :ConstantColumn(str, ConstantColumn::LITERAL) + { } +}; + + +class ConstantColumnNum: public ConstantColumn +{ +public: + ConstantColumnNum(const CalpontSystemCatalog::ColType &type, + const std::string &str) + :ConstantColumn(str, ConstantColumn::NUM) + { + resultType(type); + } +}; + + +class ConstantColumnUInt: public ConstantColumn +{ +public: + ConstantColumnUInt(uint64_t val, int8_t scale, uint8_t precision) + :ConstantColumn(val, ConstantColumn::NUM, scale, precision) + { } +}; + + +class ConstantColumnSInt: public ConstantColumn +{ +public: + ConstantColumnSInt(const CalpontSystemCatalog::ColType &type, + const std::string &str, int64_t val) + :ConstantColumn(str, val, ConstantColumn::NUM) + { + resultType(type); + } +}; + + +class ConstantColumnReal: public ConstantColumn +{ +public: + ConstantColumnReal(const CalpontSystemCatalog::ColType &type, + const std::string &str, + double val) + :ConstantColumn(str, val) + { + resultType(type); + } +}; + + +class ConstantColumnTemporal: public ConstantColumn +{ +public: + ConstantColumnTemporal(const CalpontSystemCatalog::ColType &type, + const std::string &str) + :ConstantColumn(str) + { + resultType(type); + } +}; + + /** * ostream operator */ diff --git a/dbcon/mysql/ha_mcs_execplan.cpp b/dbcon/mysql/ha_mcs_execplan.cpp index c7e77e810..ae985c526 100755 --- a/dbcon/mysql/ha_mcs_execplan.cpp +++ b/dbcon/mysql/ha_mcs_execplan.cpp @@ -3250,6 +3250,190 @@ CalpontSystemCatalog::ColType colType_MysqlToIDB (const Item* item) return ct; } + +ReturnedColumn * +buildReturnedColumnNull(gp_walk_info& gwi) +{ + if (gwi.condPush) + return new SimpleColumn("noop"); + ConstantColumn *rc = new ConstantColumnNull(); + if (rc) + rc->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); + return rc; +} + + +class ValStrStdString: public string +{ + bool mIsNull; +public: + ValStrStdString(Item *item) + { + String val, *str = item->val_str(&val); + mIsNull = (str == nullptr); + DBUG_ASSERT(mIsNull == item->null_value); + if (!mIsNull) + assign(str->ptr(), str->length()); + } + bool isNull() const { return mIsNull; } +}; + + +/* + Create a ConstantColumn according to cmp_type(). + But do not set the time zone yet. + + Handles NOT NULL values. + + Three ways of value extraction are used depending on the data type: + 1. Using a native val_xxx(). + 2. Using val_str() with further convertion to the native representation. + 3. Using both val_str() and a native val_xxx(). + + We should eventually get rid of N2 and N3 and use N1 for all data types: + - N2 contains a redundant code for str->native conversion. + It should be replaced to an existing code (a Type_handler method call?). + - N3 performs double evalation of the value, which may cause + various negative effects (double side effects or double warnings). +*/ +static ConstantColumn * +newConstantColumnNotNullUsingValNativeNoTz(Item *item, gp_walk_info& gwi) +{ + DBUG_ASSERT(item->const_item()); + + switch (item->cmp_type()) + { + case INT_RESULT: + { + if (item->unsigned_flag) + return new ConstantColumnUInt((uint64_t) item->val_uint(), + (int8_t) item->decimal_scale(), + (uint8_t) item->decimal_precision()); + ValStrStdString str(item); + DBUG_ASSERT(!str.isNull()); + return new ConstantColumnSInt(colType_MysqlToIDB(item), str, + (int64_t) item->val_int()); + } + case STRING_RESULT: + { + // Special handling for 0xHHHH literals + if (item->type_handler() == &type_handler_hex_hybrid) + return new ConstantColumn((int64_t)item->val_int(), + ConstantColumn::NUM); + ValStrStdString str(item); + DBUG_ASSERT(!str.isNull()); + return new ConstantColumnString(str); + } + case REAL_RESULT: + { + ValStrStdString str(item); + DBUG_ASSERT(!str.isNull()); + return new ConstantColumnReal(colType_MysqlToIDB(item), + str, item->val_real()); + } + case DECIMAL_RESULT: + { + ValStrStdString str(item); + DBUG_ASSERT(!str.isNull()); + return buildDecimalColumn(item, str, gwi); + } + case TIME_RESULT: + { + ValStrStdString str(item); + DBUG_ASSERT(!str.isNull()); + return new ConstantColumnTemporal(colType_MysqlToIDB(item), str); + } + default: + { + gwi.fatalParseError = true; + gwi.parseErrorText = "Unknown item type"; + break; + } + } + + return nullptr; +} + + +/* + Create a ConstantColumn according to cmp_type(). + But do not set the time zone yet. + + Handles NULL and NOT NULL values. + + Uses a simplified logic regarding to data types: + always extracts the value through val_str(). + + Should probably be joined with the previous function, to have + a single function which can at the same time: + a. handle both NULL and NOT NULL values + b. extract values using a native val_xxx() method, + to avoid possible negative effects mentioned in the comments + to newConstantColumnNotNullUsingValNativeNoTz(). +*/ +static ConstantColumn * +newConstantColumnMaybeNullFromValStrNoTz(const Item *item, + const ValStrStdString &valStr, + gp_walk_info& gwi) +{ + if (valStr.isNull()) + return new ConstantColumnNull(); + + switch (item->result_type()) + { + case STRING_RESULT: + return new ConstantColumnString(valStr); + case DECIMAL_RESULT: + return buildDecimalColumn(item, valStr, gwi); + case TIME_RESULT: + case INT_RESULT: + case REAL_RESULT: + case ROW_RESULT: + return new ConstantColumnNum(colType_MysqlToIDB(item), valStr); + } + return nullptr; +} + + +// Create a ConstantColumn from a previously evaluated val_str() result, +// Supports both NULL and NOT NULL values. +// Sets the time zone according to gwi. + +static ConstantColumn * +buildConstantColumnMaybeNullFromValStr(const Item *item, + const ValStrStdString &valStr, + gp_walk_info& gwi) +{ + ConstantColumn *rc= newConstantColumnMaybeNullFromValStrNoTz(item, valStr, gwi); + if (rc) + rc->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); + return rc; +} + + +// Create a ConstantColumn by calling val_str(). +// Supports both NULL and NOT NULL values. +// Sets the time zone according to gwi. + +static ConstantColumn * +buildConstantColumnMaybeNullUsingValStr(Item *item, gp_walk_info& gwi) +{ + return buildConstantColumnMaybeNullFromValStr(item, ValStrStdString(item), gwi); +} + + +// Create a ConstantColumn for a NOT NULL expression. +// Sets the time zone according to gwi. +static ConstantColumn * +buildConstantColumnNotNullUsingValNative(Item *item, gp_walk_info& gwi) +{ + ConstantColumn *rc = newConstantColumnNotNullUsingValNativeNoTz(item, gwi); + if (rc) + rc->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); + return rc; +} + + ReturnedColumn* buildReturnedColumn( Item* item, gp_walk_info& gwi, bool& nonSupport, @@ -3281,82 +3465,20 @@ ReturnedColumn* buildReturnedColumn( return buildSimpleColumn(ifp, gwi); } + case Item::NULL_ITEM: + return buildReturnedColumnNull(gwi); case Item::CONST_ITEM: { - switch (item->cmp_type()) - { - case INT_RESULT: - { - String val, *str = item->val_str(&val); - string valStr; - valStr.assign(str->ptr(), str->length()); - - if (item->unsigned_flag) - { - rc = new ConstantColumn((uint64_t)item->val_uint(), ConstantColumn::NUM, - (int8_t) item->decimal_scale(), (uint8_t) item->decimal_precision()); - } - else - { - rc = new ConstantColumn(valStr, (int64_t)item->val_int(), ConstantColumn::NUM); - } - - break; - } - case STRING_RESULT: - { - // Special handling for 0xHHHH literals - if (item->type_handler() == &type_handler_hex_hybrid) - { - Item_hex_hybrid *hip = reinterpret_cast(const_cast(item)); - rc = new ConstantColumn((int64_t)hip->val_int(), ConstantColumn::NUM); - break; - } - - String val, *str = item->val_str(&val); - string valStr; - valStr.assign(str->ptr(), str->length()); - rc = new ConstantColumn(valStr); - break; - } - case REAL_RESULT: - { - String val, *str = item->val_str(&val); - string valStr; - valStr.assign(str->ptr(), str->length()); - rc = new ConstantColumn(valStr, item->val_real()); - break; - } - case DECIMAL_RESULT: - { - rc = buildDecimalColumn(item, gwi); - break; - } - case TIME_RESULT: - { - String val, *str = item->val_str(&val); - string valStr; - valStr.assign(str->ptr(), str->length()); - rc = new ConstantColumn(valStr); - break; - } - default: - { - gwi.fatalParseError = true; - gwi.parseErrorText = "Unknown item type"; - break; - } - } - - if (rc && (item->cmp_type() != DECIMAL_RESULT)) - { - (dynamic_cast(rc))->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); - } - + rc = buildConstantColumnNotNullUsingValNative(item, gwi); break; } case Item::FUNC_ITEM: { + if (item->const_item()) + { + rc = buildConstantColumnMaybeNullUsingValStr(item, gwi); + break; + } Item_func* ifp = (Item_func*)item; string func_name = ifp->func_name(); @@ -3379,34 +3501,7 @@ ReturnedColumn* buildReturnedColumn( !(parseInfo & AF_BIT) && (tmpVec.size() == 0)) { - String val, *str = ifp->val_str(&val); - string valStr; - - if (str) - { - valStr.assign(str->ptr(), str->length()); - } - - if (!str) - { - rc = new ConstantColumn("", ConstantColumn::NULLDATA); - (dynamic_cast(rc))->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); - } - else if (ifp->result_type() == STRING_RESULT) - { - rc = new ConstantColumn(valStr, ConstantColumn::LITERAL); - (dynamic_cast(rc))->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); - rc->resultType(colType_MysqlToIDB(item)); - } - else if (ifp->result_type() == DECIMAL_RESULT) - rc = buildDecimalColumn(ifp, gwi); - else - { - rc = new ConstantColumn(valStr, ConstantColumn::NUM); - (dynamic_cast(rc))->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); - rc->resultType(colType_MysqlToIDB(item)); - } - + rc = buildConstantColumnMaybeNullUsingValStr(item, gwi); break; } @@ -3452,19 +3547,8 @@ ReturnedColumn* buildReturnedColumn( gwi.parseErrorText = "Unknown REF item"; break; } + return buildReturnedColumnNull(gwi); } - /* fall through */ - - case Item::NULL_ITEM: - { - if (gwi.condPush) - return new SimpleColumn("noop"); - - ConstantColumn *tmp = new ConstantColumn("", ConstantColumn::NULLDATA); - tmp->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); - return tmp; - } - case Item::CACHE_ITEM: { Item* col = ((Item_cache*)item)->get_example(); @@ -4579,19 +4663,17 @@ FunctionColumn* buildCaseFunction(Item_func* item, gp_walk_info& gwi, bool& nonS return fc; } -ConstantColumn* buildDecimalColumn(Item* item, gp_walk_info& gwi) +ConstantColumn* buildDecimalColumn(const Item *idp, + const std::string &valStr, + gp_walk_info& gwi) { - Item_decimal* idp = (Item_decimal*)item; IDB_Decimal columnstore_decimal; - String val, *str = item->val_str(&val); - string valStr; - valStr.assign(str->ptr(), str->length()); ostringstream columnstore_decimal_val; uint32_t i = 0; - if (str->ptr()[0] == '+' || str->ptr()[0] == '-') + if (valStr[0] == '+' || valStr[0] == '-') { - columnstore_decimal_val << str->ptr()[0]; + columnstore_decimal_val << valStr[0]; i = 1; } @@ -4599,16 +4681,16 @@ ConstantColumn* buildDecimalColumn(Item* item, gp_walk_info& gwi) // handle the case when the constant value is 0.12345678901234567890123456789012345678 // In this case idp->decimal_precision() = 39, but we can - if (((i + 1) < str->length()) && - str->ptr()[i] == '0' && str->ptr()[i + 1] == '.') + if (((i + 1) < valStr.length()) && + valStr[i] == '0' && valStr[i + 1] == '.') specialPrecision = true; - for (; i < str->length(); i++) + for (; i < valStr.length(); i++) { - if (str->ptr()[i] == '.') + if (valStr[i] == '.') continue; - columnstore_decimal_val << str->ptr()[i]; + columnstore_decimal_val << valStr[i]; } if (idp->decimal_precision() <= datatypes::INT64MAXPRECISION) @@ -4635,7 +4717,6 @@ ConstantColumn* buildDecimalColumn(Item* item, gp_walk_info& gwi) columnstore_decimal.precision = (idp->decimal_precision() > datatypes::INT128MAXPRECISION) ? datatypes::INT128MAXPRECISION : idp->decimal_precision(); ConstantColumn* cc = new ConstantColumn(valStr, columnstore_decimal); - cc->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); cc->charsetNumber(idp->collation.collation->number); return cc; } @@ -4770,15 +4851,104 @@ ParseTree* buildParseTree(Item_func* item, gp_walk_info& gwi, bool& nonSupport) return pt; } +class ConstArgParam +{ +public: + unsigned int precision; + unsigned int scale; + bool bIsConst; + bool hasDecimalConst; + ConstArgParam() + :precision(0), + scale(0), + bIsConst(false), + hasDecimalConst(false) + { } +}; + + +static bool +isSupportedAggregateWithOneConstArg(const Item_sum *item, Item ** orig_args) +{ + if (item->argument_count() != 1 || !orig_args[0]->const_item()) + return false; + switch(orig_args[0]->cmp_type()) + { + case INT_RESULT: + case STRING_RESULT: + case REAL_RESULT: + case DECIMAL_RESULT: + return true; + default: + break; + } + return false; +} + + +static void +processAggregateColumnConstArg(gp_walk_info& gwi, SRCP &parm, + AggregateColumn* ac, Item *sfitemp, + ConstArgParam & constParam) +{ + DBUG_ASSERT(sfitemp->const_item()); + switch(sfitemp->cmp_type()) + { + case INT_RESULT: + case STRING_RESULT: + case REAL_RESULT: + case DECIMAL_RESULT: + { + ReturnedColumn *rt = buildReturnedColumn(sfitemp, gwi, gwi.fatalParseError); + if (!rt) + { + gwi.fatalParseError = true; + return; + } + ConstantColumn *cc; + if ((cc= dynamic_cast(rt)) && + cc->type() == ConstantColumn::NULLDATA) + { + // Explicit NULL or a const function that evaluated to NULL + cc = new ConstantColumnNull(); + cc->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); + parm.reset(cc); + ac->constCol(SRCP(rt)); + return; + } + + // treat as count(*) + if (ac->aggOp() == AggregateColumn::COUNT) + ac->aggOp(AggregateColumn::COUNT_ASTERISK); + + parm.reset(rt); + ac->constCol(parm); + constParam.bIsConst = true; + if (sfitemp->cmp_type() == DECIMAL_RESULT) + { + constParam.hasDecimalConst = true; + constParam.precision = sfitemp->decimal_precision(); + constParam.scale = sfitemp->decimal_scale(); + } + break; + } + case TIME_RESULT: + // QQ: why temporal constants are not handled? + case ROW_RESULT: + { + gwi.fatalParseError = true; + } + } +} + + ReturnedColumn* buildAggregateColumn(Item* item, gp_walk_info& gwi) { // MCOL-1201 For UDAnF multiple parameters vector selCols; vector orderCols; - bool bIsConst = false; - unsigned int constValPrecision = 0; - unsigned int constValScale = 0; - bool hasDecimalConst = false; + ConstArgParam constArgParam; + if (get_fe_conn_info_ptr() == NULL) set_fe_conn_info_ptr((void*)new cal_connection_info()); @@ -4920,6 +5090,10 @@ ReturnedColumn* buildAggregateColumn(Item* item, gp_walk_info& gwi) (dynamic_cast(ac))->separator(separator); } } + else if (isSupportedAggregateWithOneConstArg(isp, sfitempp)) + { + processAggregateColumnConstArg(gwi, parm, ac, sfitempp[0], constArgParam); + } else { for (uint32_t i = 0; i < isp->argument_count(); i++) @@ -4948,43 +5122,11 @@ ReturnedColumn* buildAggregateColumn(Item* item, gp_walk_info& gwi) } case Item::CONST_ITEM: - { - switch(sfitemp->cmp_type()) - { - case INT_RESULT: - case STRING_RESULT: - case REAL_RESULT: - case DECIMAL_RESULT: - { - // treat as count(*) - if (ac->aggOp() == AggregateColumn::COUNT) - ac->aggOp(AggregateColumn::COUNT_ASTERISK); - - parm.reset(buildReturnedColumn(sfitemp, gwi, gwi.fatalParseError)); - ac->constCol(parm); - bIsConst = true; - if (sfitemp->cmp_type() == DECIMAL_RESULT) - { - hasDecimalConst = true; - Item_decimal* idp = (Item_decimal*)sfitemp; - constValPrecision = idp->decimal_precision(); - constValScale = idp->decimal_scale(); - } - break; - } - default: - { - gwi.fatalParseError = true; - } - } - break; - } - case Item::NULL_ITEM: { - parm.reset(new ConstantColumn("", ConstantColumn::NULLDATA)); - (dynamic_cast(parm.get()))->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); - ac->constCol(SRCP(buildReturnedColumn(sfitemp, gwi, gwi.fatalParseError))); + processAggregateColumnConstArg(gwi, parm, + ac, sfitemp, + constArgParam); break; } @@ -5096,7 +5238,7 @@ ReturnedColumn* buildAggregateColumn(Item* item, gp_walk_info& gwi) // Get result type // Modified for MCOL-1201 multi-argument aggregate - if (!bIsConst && ac->aggParms().size() > 0) + if (!constArgParam.bIsConst && ac->aggParms().size() > 0) { // These are all one parm functions, so we can safely // use the first parm for result type. @@ -5202,13 +5344,13 @@ because it has multiple arguments."; return nullptr; } } - else if (bIsConst && hasDecimalConst && isAvg) + else if (constArgParam.bIsConst && constArgParam.hasDecimalConst && isAvg) { CalpontSystemCatalog::ColType ct = parm->resultType(); - if (datatypes::Decimal::isWideDecimalTypeByPrecision(constValPrecision)) + if (datatypes::Decimal::isWideDecimalTypeByPrecision(constArgParam.precision)) { - ct.precision = constValPrecision; - ct.scale = constValScale; + ct.precision = constArgParam.precision; + ct.scale = constArgParam.scale; ct.colWidth = datatypes::MAXDECIMALWIDTH; } ac->resultType(ct); @@ -5739,34 +5881,9 @@ void gp_walk(const Item* item, void* arg) tmpVec.size() == 0 && ifp->functype() != Item_func::MULT_EQUAL_FUNC) { - String val, *str = ifp->val_str(&val); - string valStr; + ValStrStdString valStr(ifp); - if (str) - valStr.assign(str->ptr(), str->length()); - - ConstantColumn* cc = NULL; - - if (!str) //@ bug 2844 check whether parameter is defined - { - cc = new ConstantColumn("", ConstantColumn::NULLDATA); - cc->timeZone(gwip->thd->variables.time_zone->get_name()->ptr()); - } - else if (ifp->result_type() == STRING_RESULT) - { - cc = new ConstantColumn(valStr, ConstantColumn::LITERAL); - cc->timeZone(gwip->thd->variables.time_zone->get_name()->ptr()); - } - else if (ifp->result_type() == DECIMAL_RESULT) - { - cc = buildDecimalColumn(ifp, *gwip); - } - else - { - cc = new ConstantColumn(valStr, ConstantColumn::NUM); - cc->timeZone(gwip->thd->variables.time_zone->get_name()->ptr()); - cc->resultType(colType_MysqlToIDB(item)); - } + ConstantColumn* cc = buildConstantColumnMaybeNullFromValStr(ifp, valStr, *gwip); for (uint32_t i = 0; i < ifp->argument_count() && !gwip->rcWorkStack.empty(); i++) { @@ -5782,7 +5899,7 @@ void gp_walk(const Item* item, void* arg) else gwip->rcWorkStack.push(cc); - if (str) + if (!valStr.isNull()) IDEBUG( cerr << "Const F&E " << item->full_name() << " evaluate: " << valStr << endl ); break; @@ -6297,14 +6414,7 @@ void parse_item (Item* item, vector& field_vec, Item** sfitempp = isp->arguments(); // special handling for count(*). This should not be treated as constant. - if (isp->argument_count() == 1 && - ( sfitempp[0]->type() == Item::CONST_ITEM && - (sfitempp[0]->cmp_type() == INT_RESULT || - sfitempp[0]->cmp_type() == STRING_RESULT || - sfitempp[0]->cmp_type() == REAL_RESULT || - sfitempp[0]->cmp_type() == DECIMAL_RESULT) - ) - ) + if (isSupportedAggregateWithOneConstArg(isp, sfitempp)) { field_vec.push_back((Item_field*)item); //dummy } @@ -7449,7 +7559,7 @@ int getSelectPlan(gp_walk_info& gwi, SELECT_LEX& select_lex, if (rc) { // MCOL-2178 CS has to process determenistic functions with constant arguments. - if (!hasNonSupportItem && !nonConstFunc(ifp) && !(parseInfo & AF_BIT) && tmpVec.size() == 0) + if (!hasNonSupportItem && ifp->const_item() && !(parseInfo & AF_BIT) && tmpVec.size() == 0) { srcp.reset(buildReturnedColumn(item, gwi, gwi.fatalParseError)); gwi.returnedCols.push_back(srcp); @@ -7475,34 +7585,7 @@ int getSelectPlan(gp_walk_info& gwi, SELECT_LEX& select_lex, if (!hasNonSupportItem && (after_size - before_size) == 0 && !(parseInfo & AGG_BIT) && !(parseInfo & SUB_BIT)) { - String val, *str = ifp->val_str(&val); - string valStr; - - if (str) - valStr.assign(str->ptr(), str->length()); - - ConstantColumn* cc = NULL; - - if (!str) - { - cc = new ConstantColumn("", ConstantColumn::NULLDATA); - cc->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); - } - else if (ifp->result_type() == STRING_RESULT) - { - cc = new ConstantColumn(valStr, ConstantColumn::LITERAL); - cc->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); - } - else if (ifp->result_type() == DECIMAL_RESULT) - { - cc = buildDecimalColumn(ifp, gwi); - } - else - { - cc = new ConstantColumn(valStr, ConstantColumn::NUM); - cc->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); - cc->resultType(colType_MysqlToIDB(item)); - } + ConstantColumn* cc = buildConstantColumnMaybeNullUsingValStr(ifp, gwi); SRCP srcp(cc); @@ -9341,34 +9424,7 @@ int getGroupPlan(gp_walk_info& gwi, SELECT_LEX& select_lex, SCSEP& csep, cal_gro !(parseInfo & AGG_BIT) && !(parseInfo & SUB_BIT) ) { - String val, *str = ifp->val_str(&val); - string valStr; - - if (str) - valStr.assign(str->ptr(), str->length()); - - ConstantColumn* cc = NULL; - - if (!str) - { - cc = new ConstantColumn("", ConstantColumn::NULLDATA); - cc->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); - } - else if (ifp->result_type() == STRING_RESULT) - { - cc = new ConstantColumn(valStr, ConstantColumn::LITERAL); - cc->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); - } - else if (ifp->result_type() == DECIMAL_RESULT) - { - cc = buildDecimalColumn(ifp, gwi); - } - else - { - cc = new ConstantColumn(valStr, ConstantColumn::NUM); - cc->timeZone(gwi.thd->variables.time_zone->get_name()->ptr()); - cc->resultType(colType_MysqlToIDB(item)); - } + ConstantColumn* cc = buildConstantColumnMaybeNullUsingValStr(ifp, gwi); SRCP srcp(cc); diff --git a/dbcon/mysql/ha_mcs_impl_if.h b/dbcon/mysql/ha_mcs_impl_if.h index c9470cd17..f8556bdfb 100644 --- a/dbcon/mysql/ha_mcs_impl_if.h +++ b/dbcon/mysql/ha_mcs_impl_if.h @@ -373,7 +373,7 @@ bool isUpdateHasForeignTable(THD* thd); execplan::ReturnedColumn* buildReturnedColumn(Item* item, gp_walk_info& gwi, bool& nonSupport, bool isRefItem = false); execplan::ReturnedColumn* buildFunctionColumn(Item_func* item, gp_walk_info& gwi, bool& nonSupport, bool selectBetweenIn = false); execplan::ArithmeticColumn* buildArithmeticColumn(Item_func* item, gp_walk_info& gwi, bool& nonSupport); -execplan::ConstantColumn* buildDecimalColumn(Item* item, gp_walk_info& gwi); +execplan::ConstantColumn* buildDecimalColumn(const Item *item, const std::string &str, gp_walk_info& gwi); execplan::SimpleColumn* buildSimpleColumn(Item_field* item, gp_walk_info& gwi); execplan::FunctionColumn* buildCaseFunction(Item_func* item, gp_walk_info& gwi, bool& nonSupport); execplan::ParseTree* buildParseTree(Item_func* item, gp_walk_info& gwi, bool& nonSupport); diff --git a/mysql-test/columnstore/bugfixes/mcol-4728.result b/mysql-test/columnstore/bugfixes/mcol-4728.result new file mode 100644 index 000000000..8a2fab434 --- /dev/null +++ b/mysql-test/columnstore/bugfixes/mcol-4728.result @@ -0,0 +1,63 @@ +DROP DATABASE IF EXISTS mcol_4728; +CREATE DATABASE mcol_4728; +USE mcol_4728; +SET columnstore_select_handler=ON; +CREATE TABLE t1 (a INT) ENGINE=ColumnStore; +SELECT SUM(0+0)-SUM(0+0) FROM t1; +SUM(0+0)-SUM(0+0) +NULL +SELECT SUM(0) FROM t1; +SUM(0) +NULL +SELECT COUNT(0) FROM t1; +COUNT(0) +0 +SELECT COUNT(NULL) FROM t1; +COUNT(NULL) +0 +SELECT COUNT(COALESCE(NULL)) FROM t1; +COUNT(COALESCE(NULL)) +0 +SELECT MAX(0) FROM t1; +MAX(0) +NULL +SELECT SUM(1)+1 FROM t1; +SUM(1)+1 +NULL +SELECT SUM(COALESCE(1))+1 FROM t1; +SUM(COALESCE(1))+1 +NULL +SELECT sum(rand(0))+1 FROM t1; +sum(rand(0))+1 +NULL +INSERT INTO t1 VALUES (100); +SELECT SUM(0+0)-SUM(0+0) FROM t1; +SUM(0+0)-SUM(0+0) +0 +SELECT SUM(0) FROM t1; +SUM(0) +0 +SELECT COUNT(0) FROM t1; +COUNT(0) +1 +SELECT COUNT(NULL) FROM t1; +COUNT(NULL) +0 +SELECT COUNT(COALESCE(NULL)) FROM t1; +COUNT(COALESCE(NULL)) +0 +SELECT MAX(0) FROM t1; +MAX(0) +0 +SELECT SUM(1)+1 FROM t1; +SUM(1)+1 +2 +SELECT SUM(COALESCE(1))+1 FROM t1; +SUM(COALESCE(1))+1 +2 +SELECT sum(rand(0))+1 FROM t1; +sum(rand(0))+1 +1.1552204276949358 +DROP TABLE t1; +DROP DATABASE mcol_4728; +USE test; diff --git a/mysql-test/columnstore/bugfixes/mcol-4728.test b/mysql-test/columnstore/bugfixes/mcol-4728.test new file mode 100644 index 000000000..b386a2608 --- /dev/null +++ b/mysql-test/columnstore/bugfixes/mcol-4728.test @@ -0,0 +1,38 @@ +--source ../include/have_columnstore.inc + +--disable_warnings +DROP DATABASE IF EXISTS mcol_4728; +--enable_warnings + +CREATE DATABASE mcol_4728; +USE mcol_4728; + +SET columnstore_select_handler=ON; +CREATE TABLE t1 (a INT) ENGINE=ColumnStore; + +SELECT SUM(0+0)-SUM(0+0) FROM t1; +SELECT SUM(0) FROM t1; +SELECT COUNT(0) FROM t1; +SELECT COUNT(NULL) FROM t1; +SELECT COUNT(COALESCE(NULL)) FROM t1; +SELECT MAX(0) FROM t1; +SELECT SUM(1)+1 FROM t1; +SELECT SUM(COALESCE(1))+1 FROM t1; +SELECT sum(rand(0))+1 FROM t1; + +INSERT INTO t1 VALUES (100); + +SELECT SUM(0+0)-SUM(0+0) FROM t1; +SELECT SUM(0) FROM t1; +SELECT COUNT(0) FROM t1; +SELECT COUNT(NULL) FROM t1; +SELECT COUNT(COALESCE(NULL)) FROM t1; +SELECT MAX(0) FROM t1; +SELECT SUM(1)+1 FROM t1; +SELECT SUM(COALESCE(1))+1 FROM t1; +SELECT sum(rand(0))+1 FROM t1; + +DROP TABLE t1; + +DROP DATABASE mcol_4728; +USE test; diff --git a/mysql-test/columnstore/bugfixes/suite.opt b/mysql-test/columnstore/bugfixes/suite.opt new file mode 100644 index 000000000..fbd322fdd --- /dev/null +++ b/mysql-test/columnstore/bugfixes/suite.opt @@ -0,0 +1 @@ +--plugin-load-add=$HA_COLUMNSTORE_SO diff --git a/mysql-test/columnstore/bugfixes/suite.pm b/mysql-test/columnstore/bugfixes/suite.pm new file mode 100644 index 000000000..93a24238c --- /dev/null +++ b/mysql-test/columnstore/bugfixes/suite.pm @@ -0,0 +1,23 @@ +package My::Suite::ColumnStore; + +@ISA = qw(My::Suite); + +my $mcs_bin_dir_compiled=$::bindir . '/storage/columnstore/columnstore/bin'; +my $mcs_ins_dir_installed=$::bindir . '/bin'; + +if (-d $mcs_bin_dir_compiled) +{ + $ENV{MCS_MCSSETCONFIG}=$mcs_bin_dir_compiled . "/mcsSetConfig"; + $ENV{MCS_CPIMPORT}=$mcs_bin_dir_compiled . "/cpimport"; + $ENV{MCS_SYSCATALOG_MYSQL_SQL}=$::mysqld_variables{'basedir'} . "/storage/columnstore/columnstore/dbcon/mysql/syscatalog_mysql.sql"; +} +elsif (-d $mcs_ins_dir_installed) +{ + $ENV{MCS_MCSSETCONFIG}=$mcs_ins_dir_installed . "/mcsSetConfig"; + $ENV{MCS_CPIMPORT}=$mcs_ins_dir_installed . "/cpimport"; + $ENV{MCS_SYSCATALOG_MYSQL_SQL}=$::mysqld_variables{'basedir'} . "/share/columnstore/syscatalog_mysql.sql"; +} + +sub is_default { 0 } + +bless { };