Skip to content

Commit

Permalink
feat(datatypes): MCOL-4632 and MCOL-4648, fix cast leads to NULL.
Browse files Browse the repository at this point in the history
Remove redundant cast.

As C-style casts with a type name in parantheses are interpreted as static_casts this literally just changes the interpretation around (and forces an implicit cast to match the return value of the function).

Switch UBIGINTNULL and UBIGINTEMPTYROW constants for consistency.

Make consistent with relation between BIGINTNULL and BIGINTEMPTYROW & make adapted cast behaviour due to NULL markers more intuitive. (After this change we can simply block the highest possible uint64_t value and if a cast results in it, print the next lower value (2^64 - 2). Previously, (2^64 - 1) was able to be printed, but (2^64 - 2) as being blocked by the UBIGINTNULL constant was not, making finding the appropiate replacement value to give out more confusing.

Introduce MAX_MCS_UBIGINT and MIN_MCS_BIGINT and adapt casts.

Adapt casting to BIGINT to remove NULL marker error.

Add bugfix regression test for MCOL 4632

Add regression test for mcol_4648

Revert "Switch UBIGINTNULL and UBIGINTEMPTYROW constants for consistency."

This reverts commit 83eac11.
Due to backwards compatability issues.

Refactor casting to MCS[U]Int to datatype functions.

Update regression tests to include other affected datatypes.

Apply formatting.

Refactor according to PR review

Remove redundant new constant, switch to using already existing constant.

Adapt nullstring casting to EMPTYROW markers for backwards compatability.

Adapt tests for backward compatability behaviour allowing text datatypes to be casted to EMPTYROW constant.

Adapt mcol641-functions test according to bug fix.

Update tests according to new expected behaviour.

Adapt tests to new understanding of issue.

Update comments/documentation for MCOL_4632 test.

Adapt to new cast limit logic.

Make bracketing consistent.

Adapt previous regression test to new expected behaviour.
  • Loading branch information
phoeinx committed Aug 11, 2023
1 parent 4b20ced commit 48562e4
Show file tree
Hide file tree
Showing 12 changed files with 280 additions and 94 deletions.
41 changes: 1 addition & 40 deletions datatypes/mcs_datatype.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <boost/any.hpp>
#include "exceptclasses.h"
#include "conststring.h"
#include "mcs_datatype_basic.h"
#include "mcs_numeric_limits.h"
#include "mcs_data_condition.h"
#include "mcs_decimal.h"
Expand All @@ -34,46 +35,6 @@ typedef int32_t mcs_sint32_t;
struct charset_info_st;
typedef const struct charset_info_st CHARSET_INFO;


namespace
{
const int64_t MIN_TINYINT __attribute__((unused)) = std::numeric_limits<int8_t>::min() + 2; // -126;
const int64_t MAX_TINYINT __attribute__((unused)) = std::numeric_limits<int8_t>::max(); // 127;
const int64_t MIN_SMALLINT __attribute__((unused)) = std::numeric_limits<int16_t>::min() + 2; // -32766;
const int64_t MAX_SMALLINT __attribute__((unused)) = std::numeric_limits<int16_t>::max(); // 32767;
const int64_t MIN_MEDINT __attribute__((unused)) = -(1ULL << 23); // -8388608;
const int64_t MAX_MEDINT __attribute__((unused)) = (1ULL << 23) - 1; // 8388607;
const int64_t MIN_INT __attribute__((unused)) = std::numeric_limits<int32_t>::min() + 2; // -2147483646;
const int64_t MAX_INT __attribute__((unused)) = std::numeric_limits<int32_t>::max(); // 2147483647;
const int64_t MIN_BIGINT __attribute__((unused)) =
std::numeric_limits<int64_t>::min() + 2; // -9223372036854775806LL;
const int64_t MAX_BIGINT __attribute__((unused)) =
std::numeric_limits<int64_t>::max(); // 9223372036854775807

const uint64_t MIN_UINT __attribute__((unused)) = 0;
const uint64_t MIN_UTINYINT __attribute__((unused)) = 0;
const uint64_t MIN_USMALLINT __attribute__((unused)) = 0;
const uint64_t MIN_UMEDINT __attribute__((unused)) = 0;
const uint64_t MIN_UBIGINT __attribute__((unused)) = 0;
const uint64_t MAX_UINT __attribute__((unused)) = std::numeric_limits<uint32_t>::max() - 2; // 4294967293
const uint64_t MAX_UTINYINT __attribute__((unused)) = std::numeric_limits<uint8_t>::max() - 2; // 253;
const uint64_t MAX_USMALLINT __attribute__((unused)) = std::numeric_limits<uint16_t>::max() - 2; // 65533;
const uint64_t MAX_UMEDINT __attribute__((unused)) = (1ULL << 24) - 1; // 16777215
const uint64_t MAX_UBIGINT __attribute__((unused)) =
std::numeric_limits<uint64_t>::max() - 2; // 18446744073709551613

const float MAX_FLOAT __attribute__((unused)) = std::numeric_limits<float>::max(); // 3.402823466385289e+38
const float MIN_FLOAT __attribute__((unused)) = -std::numeric_limits<float>::max();
const double MAX_DOUBLE __attribute__((unused)) =
std::numeric_limits<double>::max(); // 1.7976931348623157e+308
const double MIN_DOUBLE __attribute__((unused)) = -std::numeric_limits<double>::max();
const long double MAX_LONGDOUBLE __attribute__((unused)) =
std::numeric_limits<long double>::max(); // 1.7976931348623157e+308
const long double MIN_LONGDOUBLE __attribute__((unused)) = -std::numeric_limits<long double>::max();

const uint64_t AUTOINCR_SATURATED __attribute__((unused)) = std::numeric_limits<uint64_t>::max();
} // namespace

using namespace std; // e.g. string

namespace ddlpackage
Expand Down
59 changes: 54 additions & 5 deletions datatypes/mcs_datatype_basic.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,50 @@
*/
#pragma once

#include <limits>
#include "joblisttypes.h"

/*
This file contains simple definitions that can be
needed in multiple mcs_TYPE.h files.
*/

namespace
{
const int64_t MIN_TINYINT __attribute__((unused)) = std::numeric_limits<int8_t>::min() + 2; // -126;
const int64_t MAX_TINYINT __attribute__((unused)) = std::numeric_limits<int8_t>::max(); // 127;
const int64_t MIN_SMALLINT __attribute__((unused)) = std::numeric_limits<int16_t>::min() + 2; // -32766;
const int64_t MAX_SMALLINT __attribute__((unused)) = std::numeric_limits<int16_t>::max(); // 32767;
const int64_t MIN_MEDINT __attribute__((unused)) = -(1ULL << 23); // -8388608;
const int64_t MAX_MEDINT __attribute__((unused)) = (1ULL << 23) - 1; // 8388607;
const int64_t MIN_INT __attribute__((unused)) = std::numeric_limits<int32_t>::min() + 2; // -2147483646;
const int64_t MAX_INT __attribute__((unused)) = std::numeric_limits<int32_t>::max(); // 2147483647;
const int64_t MIN_BIGINT = std::numeric_limits<int64_t>::min() + 2; // -9223372036854775806LL;
const int64_t MAX_BIGINT = std::numeric_limits<int64_t>::max(); // 9223372036854775807

const uint64_t MIN_UINT __attribute__((unused)) = 0;
const uint64_t MIN_UTINYINT __attribute__((unused)) = 0;
const uint64_t MIN_USMALLINT __attribute__((unused)) = 0;
const uint64_t MIN_UMEDINT __attribute__((unused)) = 0;
const uint64_t MIN_UBIGINT = 0;
const uint64_t MAX_UINT __attribute__((unused)) = std::numeric_limits<uint32_t>::max() - 2; // 4294967293
const uint64_t MAX_UTINYINT __attribute__((unused)) = std::numeric_limits<uint8_t>::max() - 2; // 253;
const uint64_t MAX_USMALLINT __attribute__((unused)) = std::numeric_limits<uint16_t>::max() - 2; // 65533;
const uint64_t MAX_UMEDINT __attribute__((unused)) = (1ULL << 24) - 1; // 16777215
const uint64_t MAX_UBIGINT = std::numeric_limits<uint64_t>::max() - 2; // 18446744073709551613

const float MAX_FLOAT __attribute__((unused)) = std::numeric_limits<float>::max(); // 3.402823466385289e+38
const float MIN_FLOAT __attribute__((unused)) = -std::numeric_limits<float>::max();
const double MAX_DOUBLE __attribute__((unused)) =
std::numeric_limits<double>::max(); // 1.7976931348623157e+308
const double MIN_DOUBLE __attribute__((unused)) = -std::numeric_limits<double>::max();
const long double MAX_LONGDOUBLE __attribute__((unused)) =
std::numeric_limits<long double>::max(); // 1.7976931348623157e+308
const long double MIN_LONGDOUBLE __attribute__((unused)) = -std::numeric_limits<long double>::max();

const uint64_t AUTOINCR_SATURATED __attribute__((unused)) = std::numeric_limits<uint64_t>::max();
} // namespace

namespace datatypes
{
// Convert a positive floating point value to
Expand Down Expand Up @@ -57,9 +96,14 @@ template <typename SRC>
int64_t xFloatToMCSSInt64Round(SRC value)
{
if (value > 0)
return positiveXFloatToXIntRound<SRC, int64_t>(value, numeric_limits<int64_t>::max());
{
return positiveXFloatToXIntRound<SRC, int64_t>(value, MAX_BIGINT);
}
if (value < 0)
return negativeXFloatToXIntRound<SRC, int64_t>(value, numeric_limits<int64_t>::min() + 2);
{
//@Bug 4632 and 4648: Don't return marker value for NULL, but allow return of marker value for EMPTYROW.
return negativeXFloatToXIntRound<SRC, int64_t>(value, joblist::BIGINTEMPTYROW);
}
return 0;
}

Expand All @@ -69,12 +113,17 @@ template <typename SRC>
uint64_t xFloatToMCSUInt64Round(SRC value)
{
if (value > 0)
return positiveXFloatToXIntRound<SRC, uint64_t>(value, numeric_limits<uint64_t>::max() - 2);
{
//@Bug 4632 and 4648: Don't return marker value for NULL, but allow return of marker value for EMPTYROW.
const auto val = positiveXFloatToXIntRound<SRC, uint64_t>(value, joblist::UBIGINTEMPTYROW);
return val == joblist::UBIGINTNULL ? MAX_UBIGINT : val;
}
if (value < 0)
return negativeXFloatToXIntRound<SRC, uint64_t>(value, 0);
{
return negativeXFloatToXIntRound<SRC, uint64_t>(value, MIN_UBIGINT);
}

return 0;
}

} // end of namespace datatypes

13 changes: 12 additions & 1 deletion datatypes/mcs_decimal.h
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,6 @@ const long long columnstore_precision[19] = {0,
99999999999999999LL,
999999999999999999LL};


const int128_t ConversionRangeMaxValue[20] = {9999999999999999999_xxl,
99999999999999999999_xxl,
999999999999999999999_xxl,
Expand Down Expand Up @@ -667,6 +666,18 @@ class Decimal : public TDecimal128, public TDecimal64
: TDecimal64::toUInt64Round((uint32_t)scale);
}

int64_t toMCSInt64Round() const
{
//@Bug 4632 and 4648: Don't return marker value for NULL, but allow return of marker value for EMPTYROW.
return std::max(toSInt64Round(), static_cast<int64_t>(joblist::BIGINTEMPTYROW));
}
uint64_t toMCSUInt64Round() const
{
const auto val = toUInt64Round();
//@Bug 4632 and 4648: Don't return marker value for NULL, but allow return of marker value for EMPTYROW.
return val == joblist::UBIGINTNULL ? MAX_UBIGINT : val;
}

// FLOOR related routines
int64_t toSInt64Floor() const
{
Expand Down
25 changes: 24 additions & 1 deletion datatypes/mcs_int64.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@
*/
#pragma once

#include <algorithm>
#include "exceptclasses.h"
#include "mcs_datatype_basic.h"

namespace datatypes
{
Expand Down Expand Up @@ -60,6 +62,18 @@ class TUInt64
{
*(uint64_t*)dst = mValue;
}

uint64_t toMCSUInt64() const
{
//@Bug 4632 and 4648: Don't return marker value for NULL, but allow return of marker value for EMPTYROW.
return mValue == joblist::UBIGINTNULL ? MAX_UBIGINT : mValue;
}

int64_t toMCSInt64() const
{
//@Bug 4632 and 4648: Don't return marker value for NULL, but allow return of marker value for EMPTYROW.
return std::max(static_cast<int64_t>(mValue), static_cast<int64_t>(joblist::BIGINTEMPTYROW));
}
};

class TSInt64
Expand All @@ -84,6 +98,16 @@ class TSInt64
{
return mValue < 0 ? 0 : static_cast<uint64_t>(mValue);
}

uint64_t toMCSUInt64() const
{
return static_cast<uint64_t>(mValue);
}

int64_t toMCSInt64() const
{
return std::max(static_cast<int64_t>(mValue), static_cast<int64_t>(joblist::BIGINTEMPTYROW));
}
};

class TUInt64Null : public TUInt64, public TNullFlag
Expand Down Expand Up @@ -178,4 +202,3 @@ class TSInt64Null : public TSInt64, public TNullFlag
};

} // end of namespace datatypes

3 changes: 1 addition & 2 deletions dbcon/ddlpackageproc/altertableprocessor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,6 @@ AlterTableProcessor::DDLResult AlterTableProcessor::processPackage(
abs_ts.tv_nsec = rm_ts.tv_nsec;
} while (nanosleep(&abs_ts, &rm_ts) < 0);


try
{
processID = ::getpid();
Expand Down Expand Up @@ -939,7 +938,7 @@ void AlterTableProcessor::addColumn(uint32_t sessionID, execplan::CalpontSystemC
createWriteDropLogFile(ropair.objnum, uniqueId, oidList);

//@Bug 1358,1427 Always use the first column in the table, not the first one in columnlist to prevent
//random result
// random result
//@Bug 4182. Use widest column as reference column

// Find the widest column
Expand Down
4 changes: 2 additions & 2 deletions mysql-test/columnstore/basic/r/mcol641-functions.result
Original file line number Diff line number Diff line change
Expand Up @@ -1112,8 +1112,8 @@ SELECT "convert_test", CONVERT(d1, SIGNED), CONVERT(d2, SIGNED), CONVERT(d3, SIG
convert_test CONVERT(d1, SIGNED) CONVERT(d2, SIGNED) CONVERT(d3, SIGNED)
convert_test NULL NULL NULL
convert_test 0 0 0
convert_test NULL NULL 0
convert_test NULL NULL 0
convert_test -9223372036854775807 -9223372036854775807 0
convert_test -9223372036854775807 -9223372036854775807 0
convert_test -123456 -123 0
convert_test 123456 123 0
convert_test 9223372036854775807 9223372036854775807 0
Expand Down
41 changes: 41 additions & 0 deletions mysql-test/columnstore/bugfixes/mcol_4632.result
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
#
# MCOL-4632: Disallow casting to NULL marker values, allow casting to EMPTY ROW values.
#
# Defined in joblisttypes.h: BIGINTNULL = 0x8000000000000000ULL == -9223372036854775808 (when casted to int64_t)
# Defined in joblisttypes.h: BIGINTEMPTYROW = 0x8000000000000001ULL == -9223372036854775807 (when casted to int64_t)
#
DROP DATABASE IF EXISTS mcol_4632;
CREATE DATABASE mcol_4632;
USE mcol_4632;
CREATE TABLE t1
(d1 DECIMAL(30,0), d2 DECIMAL(30,0) NOT NULL,
i1 BIGINT UNSIGNED, i2 BIGINT UNSIGNED NOT NULL,
str1 TEXT, str2 TEXT NOT NULL,
double1 DOUBLE(30,0), double2 DOUBLE(30,0) NOT NULL) ENGINE=ColumnStore;
INSERT INTO t1 VALUES
(-9223372036854775808, -9223372036854775808,
9223372036854775808, 9223372036854775808,
"-9223372036854775808", "-9223372036854775808",
-9223372036854775808.0, -9223372036854775808.0);
INSERT INTO t1 VALUES
(-9223372036854775807, -9223372036854775807,
9223372036854775809, 9223372036854775809,
"-9223372036854775807", "-9223372036854775807",
-9223372036854775807.0, -9223372036854775807.0);
SELECT d1, CAST(d1 AS SIGNED), CAST(d2 AS SIGNED) FROM t1;
d1 CAST(d1 AS SIGNED) CAST(d2 AS SIGNED)
-9223372036854775808 -9223372036854775807 -9223372036854775807
-9223372036854775807 -9223372036854775807 -9223372036854775807
SELECT i1, CAST(i1 AS SIGNED), CAST(i2 AS SIGNED) FROM t1;
i1 CAST(i1 AS SIGNED) CAST(i2 AS SIGNED)
9223372036854775808 -9223372036854775807 -9223372036854775807
9223372036854775809 -9223372036854775807 -9223372036854775807
SELECT str1, CAST(str1 AS SIGNED), CAST(str2 AS SIGNED) FROM t1;
str1 CAST(str1 AS SIGNED) CAST(str2 AS SIGNED)
-9223372036854775808 -9223372036854775807 -9223372036854775807
-9223372036854775807 -9223372036854775807 -9223372036854775807
SELECT double1, CAST(double1 AS SIGNED), CAST(double2 AS SIGNED) FROM t1;
double1 CAST(double1 AS SIGNED) CAST(double2 AS SIGNED)
-9223372036854776000 -9223372036854775807 -9223372036854775807
-9223372036854776000 -9223372036854775807 -9223372036854775807
DROP DATABASE mcol_4632;
32 changes: 32 additions & 0 deletions mysql-test/columnstore/bugfixes/mcol_4632.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
--source ../include/have_columnstore.inc
--echo #
--echo # MCOL-4632: Disallow casting to NULL marker values, allow casting to EMPTY ROW values.
--echo #
--echo # Defined in joblisttypes.h: BIGINTNULL = 0x8000000000000000ULL == -9223372036854775808 (when casted to int64_t)
--echo # Defined in joblisttypes.h: BIGINTEMPTYROW = 0x8000000000000001ULL == -9223372036854775807 (when casted to int64_t)
--echo #
--disable_warnings
DROP DATABASE IF EXISTS mcol_4632;
--enable_warnings
CREATE DATABASE mcol_4632;
USE mcol_4632;
CREATE TABLE t1
(d1 DECIMAL(30,0), d2 DECIMAL(30,0) NOT NULL,
i1 BIGINT UNSIGNED, i2 BIGINT UNSIGNED NOT NULL,
str1 TEXT, str2 TEXT NOT NULL,
double1 DOUBLE(30,0), double2 DOUBLE(30,0) NOT NULL) ENGINE=ColumnStore;
INSERT INTO t1 VALUES
(-9223372036854775808, -9223372036854775808,
9223372036854775808, 9223372036854775808,
"-9223372036854775808", "-9223372036854775808",
-9223372036854775808.0, -9223372036854775808.0);
INSERT INTO t1 VALUES
(-9223372036854775807, -9223372036854775807,
9223372036854775809, 9223372036854775809,
"-9223372036854775807", "-9223372036854775807",
-9223372036854775807.0, -9223372036854775807.0);
SELECT d1, CAST(d1 AS SIGNED), CAST(d2 AS SIGNED) FROM t1;
SELECT i1, CAST(i1 AS SIGNED), CAST(i2 AS SIGNED) FROM t1;
SELECT str1, CAST(str1 AS SIGNED), CAST(str2 AS SIGNED) FROM t1;
SELECT double1, CAST(double1 AS SIGNED), CAST(double2 AS SIGNED) FROM t1;
DROP DATABASE mcol_4632;
36 changes: 36 additions & 0 deletions mysql-test/columnstore/bugfixes/mcol_4648.result
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#
# MCOL-4648: Disallow casting to NULL marker values, allow casting to EMPTY ROW values.
#
# Defined in joblisttypes.h: UBIGINTNULL == 0xFFFFFFFFFFFFFFFEULL == 18446744073709551614
# Defined in joblisttypes.h: UBIGINTEMPTYROW == 0xFFFFFFFFFFFFFFFFULL == 18446744073709551615
#
DROP DATABASE IF EXISTS mcol_4648;
CREATE DATABASE mcol_4648;
USE mcol_4648;
CREATE TABLE t1
(d1 DECIMAL(30,0), d2 DECIMAL(30,0) NOT NULL,
str1 TEXT, str2 TEXT NOT NULL,
double1 DOUBLE(30,0), double2 DOUBLE(30,0) NOT NULL) ENGINE=ColumnStore;
INSERT INTO t1 VALUES
(18446744073709551614, 18446744073709551614,
"18446744073709551614", "18446744073709551614",
18446744073709551614.0, 18446744073709551614.0);
INSERT INTO t1 VALUES
(18446744073709551615, 18446744073709551615,
"18446744073709551615", "18446744073709551615",
18446744073709551615.0, 18446744073709551615.0);
SELECT d1, CAST(d1 AS UNSIGNED), CAST(d2 AS UNSIGNED) FROM t1;
d1 CAST(d1 AS UNSIGNED) CAST(d2 AS UNSIGNED)
18446744073709551614 18446744073709551613 18446744073709551613
18446744073709551615 18446744073709551615 18446744073709551615
SELECT str1, CAST(str1 AS UNSIGNED), CAST(str2 AS UNSIGNED) FROM t1;
str1 CAST(str1 AS UNSIGNED) CAST(str2 AS UNSIGNED)
18446744073709551614 18446744073709551613 18446744073709551613
18446744073709551615 18446744073709551615 18446744073709551615
# Doubles only store about 15 digits of precision for decimal places, therefore both inserted numbers
# are rounded to 18446744073709552000.0 and we expect them to be casted to EMPTYROW value each.
SELECT double1, CAST(double1 AS UNSIGNED), CAST(double2 AS UNSIGNED) FROM t1;
double1 CAST(double1 AS UNSIGNED) CAST(double2 AS UNSIGNED)
18446744073709552000 18446744073709551615 18446744073709551615
18446744073709552000 18446744073709551615 18446744073709551615
DROP DATABASE mcol_4648;
30 changes: 30 additions & 0 deletions mysql-test/columnstore/bugfixes/mcol_4648.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
--source ../include/have_columnstore.inc
--echo #
--echo # MCOL-4648: Disallow casting to NULL marker values, allow casting to EMPTY ROW values.
--echo #
--echo # Defined in joblisttypes.h: UBIGINTNULL == 0xFFFFFFFFFFFFFFFEULL == 18446744073709551614
--echo # Defined in joblisttypes.h: UBIGINTEMPTYROW == 0xFFFFFFFFFFFFFFFFULL == 18446744073709551615
--echo #
--disable_warnings
DROP DATABASE IF EXISTS mcol_4648;
--enable_warnings
CREATE DATABASE mcol_4648;
USE mcol_4648;
CREATE TABLE t1
(d1 DECIMAL(30,0), d2 DECIMAL(30,0) NOT NULL,
str1 TEXT, str2 TEXT NOT NULL,
double1 DOUBLE(30,0), double2 DOUBLE(30,0) NOT NULL) ENGINE=ColumnStore;
INSERT INTO t1 VALUES
(18446744073709551614, 18446744073709551614,
"18446744073709551614", "18446744073709551614",
18446744073709551614.0, 18446744073709551614.0);
INSERT INTO t1 VALUES
(18446744073709551615, 18446744073709551615,
"18446744073709551615", "18446744073709551615",
18446744073709551615.0, 18446744073709551615.0);
SELECT d1, CAST(d1 AS UNSIGNED), CAST(d2 AS UNSIGNED) FROM t1;
SELECT str1, CAST(str1 AS UNSIGNED), CAST(str2 AS UNSIGNED) FROM t1;
--echo # Doubles only store about 15 digits of precision for decimal places, therefore both inserted numbers
--echo # are rounded to 18446744073709552000.0 and we expect them to be casted to EMPTYROW value each.
SELECT double1, CAST(double1 AS UNSIGNED), CAST(double2 AS UNSIGNED) FROM t1;
DROP DATABASE mcol_4648;

0 comments on commit 48562e4

Please sign in to comment.