The idea is relatively simple - encode prefixes of collated strings as
integers and use them to compute extents' ranges. Then we can eliminate
extents with strings.
The actual patch does have all the code there but miss one important
step: we do not keep collation index, we keep charset index. Because of
this, some of the tests in the bugfix suite fail and thus main
functionality is turned off.
The reason of this patch to be put into PR at all is that it contains
changes that made CHAR/VARCHAR columns unsigned. This change is needed in
vectorization work.
* Fix clang warnings
* Remove vim tab guides
* initialize variables
* 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length
* Fix ISO C++17 does not allow 'register' storage class specifier for outdated bison
* chars are unsigned on ARM, having if (ival < 0) always false
* chars are unsigned by default on ARM and comparison with -1 if always true
Part 1:
As part of MCOL-3776 to address synchronization issue while accessing
the fTimeZone member of the Func class, mutex locks were added to the
accessor and mutator methods. However, this slows down processing
of TIMESTAMP columns in PrimProc significantly as all threads across
all concurrently running queries would serialize on the mutex. This
is because PrimProc only has a single global object for the functor
class (class derived from Func in utils/funcexp/functor.h) for a given
function name. To fix this problem:
(1) We remove the fTimeZone as a member of the Func derived classes
(hence removing the mutexes) and instead use the fOperationType
member of the FunctionColumn class to propagate the timezone values
down to the individual functor processing functions such as
FunctionColumn::getStrVal(), FunctionColumn::getIntVal(), etc.
(2) To achieve (1), a timezone member is added to the
execplan::CalpontSystemCatalog::ColType class.
Part 2:
Several functors in the Funcexp code call dataconvert::gmtSecToMySQLTime()
and dataconvert::mySQLTimeToGmtSec() functions for conversion between seconds
since unix epoch and broken-down representation. These functions in turn call
the C library function localtime_r() which currently has a known bug of holding
a global lock via a call to __tz_convert. This significantly reduces performance
in multi-threaded applications where multiple threads concurrently call
localtime_r(). More details on the bug:
https://sourceware.org/bugzilla/show_bug.cgi?id=16145
This bug in localtime_r() caused processing of the Functors in PrimProc to
slowdown significantly since a query execution causes Functors code to be
processed in a multi-threaded manner.
As a fix, we remove the calls to localtime_r() from gmtSecToMySQLTime()
and mySQLTimeToGmtSec() by performing the timezone-to-offset conversion
(done in dataconvert::timeZoneToOffset()) during the execution plan
creation in the plugin. Note that localtime_r() is only called when the
time_zone system variable is set to "SYSTEM".
This fix also required changing the timezone type from a std::string to
a long across the system.
mcsconfig.h and my_config.h have the following
pre-processor definitions:
1. Conflicting definitions coming from the standard cmake definitions:
- PACKAGE
- PACKAGE_BUGREPORT
- PACKAGE_NAME
- PACKAGE_STRING
- PACKAGE_TARNAME
- PACKAGE_VERSION
- VERSION
2. Conflicting definitions of other kinds:
- HAVE_STRTOLL - this is a dirt in MariaDB headers.
Should be fixed in the server code. my_config.h erroneously
performs "#define HAVE_STRTOLL" instead of "#define HAVE_STRTOLL 1".
in some cases. The former is not CMake compatible style. The latter is.
3. Non-conflicting definitions:
Otherwise, mcsconfig.h and my_config.h should be mutually compatible,
because both are generated by cmake on the same host machine. So
they should have exactly equal definitions like "HAVE_XXX", "SIZEOF_XXX", etc.
Observations:
- It's OK to include both mcsconfig.h and my_config.h providing that we
suppress duplicate definition of the above conflicting types #1 and #2.
- There is no a need to suppress duplicate definitions mentioned in #3,
as they are compatible!
- my_sys.h and m_ctype.h must always follow a CMake configuation header,
either my_config.h or mcsconfig.h (or both).
They must never be included without any preceeding configuration header.
This change make sure that we resolve conflicts by:
- either disallowing inclusion of mcsconfig.h and my_config.h
at the same time
- or by hiding conflicting definitions #1 and #2
(with their later restoring).
- also, by making sure that my_sys.h and m_ctype.h always follow
a CMake configuration file.
Details:
- idb_mysql.h can now only be included only after my_config.h
An attempt to use idb_mysql.h with mcsconfig.h instead of
my_config.h is caught by the "#error" preprocessor directive.
- mariadb_my_sys.h can now be only included after mcsconfig.h.
An attempt to use mariadb_my_sys.h without mcscofig.h
(e.g. with my_config.h) is also caught by "#error".
- collation.h now can now be included in two ways.
It now has the following effective structure:
#if defined(PREFER_MY_CONFIG_H) && defined(MY_CONFIG_H)
// Remember current conflicting definitions on the preprocessor stack
// Undefine current conflicting definitions
#endif
#include "mcsconfig.h"
#include "m_ctype.h"
#if defined(PREFER_MY_CONFIG_H) && defined(MY_CONFIG_H)
# Restore conflicting definitions from the preprocessor stack
#endif
and can be included as follows:
a. using only mcsconfig.h as a configuration header:
// my_config.h must not be included so far
#include "collation.h"
b. using my_config.h as the first included configuration file:
#define PREFER_MY_CONFIG_H // Force conflict resolution
#include "my_config.h" // can be included directly or indirectly
...
#include "collation.h"
Other changes:
- Adding helper header files
utils/common/mcsconfig_conflicting_defs_remember.h
utils/common/mcsconfig_conflicting_defs_restore.h
utils/common/mcsconfig_conflicting_defs_undef.h
to perform conflict resolution easier.
- Removing `#include "collation.h"` from a number of files,
as it's automatically included from rowgroup.h.
- Removing redundant `#include "utils_utf8.h"`.
This change is not directly related to the problem being fixed,
but it's nice to remove redundant directives for both collation.h
and utils_utf8.h from all the files that do not really need them.
(this change could probably have gone as a separate commit)
- Changing my_init() to MY_INIT(argv[0]) in the MCS services sources.
After the fix of the complitation failure it appeared that ColumnStore
services compiled with the debug build crash due to recent changes in
safemalloc. The crash happened in strcmp() with `my_progname` as an argument
(where my_progname is a mysys global variable). This problem should
probably be fixed on the server side as well to avoid passing NULL.
But, the majority of MariaDB executable programs also use MY_INIT(argv[0])
rather than my_init(). So let's make MCS do like the other programs do.
It was a dead code. It was not even a part of the soures in CMakeList.txt.
The bit AND operator implemententation resides in func_bitwise.cpp
together with all other bit operators and functions.
Bugs fixed:
- MCOL-4649 CAST(double AS UNSIGNED) returns 0
- MCOL-4631 CAST(double AS SIGNED) returns 0 or NULL
- MCOL-4647 SEC_TO_TIME(double_or_float) returns a wrong result
Problems:
- The code in Func_cast_unsigned::getUintVal() and
Func_cast_signed::getIntVal() did not properly check
the double value to fit inside a uint64_t/int64_t range.
So the corner cases:
- numeric_limits<uint64_t>::max()-2 for uint64_t
- numeric_limits<int64_t>::max() for int64_t
produced unexpected results.
The problem was in tests like this:
if (value > (double) numeric_limits<int64_t>::max())
A correct test would be:
if (value >= (double) numeric_limits<int64_t>::max())
- The code in Func_sec_to_time::getStrVal() searched for the decimal
dot character, assuming that the next character after the dot
was the leftmost fractional digit.
This assumption was wrong because huge double numbers use
scientific notation. So for example in "2.5e-40" the
digit "5" following the dot is NOT the leftmost fractional digit.
Also, the code in Func_sec_to_time::getStrVal() was slow
because of using non necessary to-string and from-string
data conversion.
Also, the code in Func_sec_to_time::getStrVal() evaluated
the argument two times: using getStrVal() then using getIntVal().
Solution:
- Adding new classes TDouble and TLongDouble.
- Adding a few function templates to reuse the code easier.
- Moving the conversion code inside TDouble and TLongDouble
methods toMCSSInt64Round() and toMCSUInt64Round().
- Reusing new classes and their methods in func_cast.cc and
func_sec_to_time.cc.
- MCOL-4618 FLOOR(-9999.0) returns a bad result
- MCOL-4653 CEIL(negativeNarrowDecimal) returns a wrong result
Main changes:
a. Moving ROUND, CEIL, FLOOR related code into a new simple
class template DecomposedDecimal, which is reused for
64 and 128 bit decimal.
b. Using DecomposedDecimal in TDecimal64 and TDecimal128
to implement ROUND, CEIL, FLOOR related methods.
c. Adding corresponding wrapper methods to the class Decimal.
d. Using new Decimal methods in Func_ceil and Func_floor.
Additional minor changed:
- Adding "explicit" to TSInt128 constructors to avoid
hidden data type conversion and erroneous choice between
64 vs 128 bit APIs when using Decimal.
Now one can call constructors in this self explanatory way:
- Decimal(TSInt128(some_int_value), scale, precision)
to create a wide decimal
- Decimal(TSInt64(some_int_value, scale, precision)
to create a narrow decimal
TODO:
Consider changing
Decimal(int64_t val, int8_t s, uint8_t p, const int128_t &val128 = 0)
to
Decimal(int64_t val, int8_t s, uint8_t p, const int128_t &val128)
(or even removing this constructor) to disallow compilation of:
Decimal(some_trivial_type_value, scale, precision)
This patch is fixing the following bugs:
- MCOL-4609 TreeNode::getIntVal() does not round: implicit DECIMAL->INT cast is not MariaDB compatible
- MCOL-4610 TreeNode::getUintVal() looses precision for narrow decimal
- MCOL-4619 TreeNode::getUintVal() does not round: Implicit DECIMAL->UINT conversion is not like in InnoDB
- MCOL-4650 TreeNode::getIntVal() looses precision for narrow decimal
- MCOL-4651 SEC_TO_TIME(hugePositiveDecimal) returns a negative time
The "SIGNED" part of the problem was previously fixed by MCOL-4640.
Fixing the "UNSIGNED" part.
- Adding TDecimal64::toUInt64Round() and Decimal::decimal64ToUInt64Round()
- Renaming Decimal::narrowRound() to decimal64ToSInt64Round(),
for a more self-descriptive name, and for symmetry with decimal64ToUInt64Round()
- Reusing TDecimal64::toSInt64Round() inside decimal64ToSInt64Round().
This change was forgotten in MCOL-4640 :(
- Removing the old code in Func_cast_unsigned::getUintVal with pow().
It caused precision loss, hence the bug. Adding a call for
Decimal::decimal64ToUInt64Round() instead.
- Adding tests for both SIGNED and UNSIGNED casts.
Additional change:
- Moving the wide-decimal-to-uint64_t rounding code from Func_cast_unsigned::getUintVal()
to TDecimal128::toUInt64Round() (with refactoring). Adding TDecimal::toUInt64Round()
for symmetry with TDecimal::toSInt64Round(). It will be easier to reuse the code
with way.
Detailed change list:
- Splitting out the narrow part of "class Decimal" into a separate class TDecimal64
- Adding a method TDecimal64::toSInt64Round()
- Reusing the method TDecimal64::toSInt64Round() in:
* Func_cast_signed::getIntVal()
* Func_char::getStrVal()
* Func_elt::getStrVal()
* makedate()
* Func_maketime::getStrVal()
Note, reusing this method in Func_char::getStrVal() also fixed this bug:
MCOL-4634 CHAR(negativeWideDecimal) is not like InnoDB
because the old code handled negative wide decimal values
in a wrong way.
- Adding a new class TDecimal128 for symmetry.
Moving a few wide decimal methods and constexpr's from Decimal to TDecimal128.
The new class TDecimal128 does not do much at this point yet.
Later we should be able to use TDecimal128 vs TDecimal64 in templates.
This change fixes:
MCOL-4462 CAST(varchar_expr AS DECIMAL(M,N)) returns a wrong result
MCOL-4500 Bit functions processing throws internally trying to cast char into decimal representation
MCOL-4532 CAST(AS DECIMAL) returns a garbage for large values
Also, this change makes string-to-decimal conversion 5-10 times faster,
depending on exact data.
Performance implemenent is achieved by the fact that (unlike in the old
implementation), the new version does not do any "string" object copying.
CI with RelWithDebInfo builds revealed a problem in the main
patch for MCOL-4464, which did not show up with Debug builds.
Methods like:
- getDoubleVal()
- getDateIntVal()
- getDatetimeIntVal()
- getTimestampIntVal()
- getTimeIntVal()
- getUintVal()
- getIntVal()
- getStrVal()
require the caller to initialize the isNull argument to false.
This fact was not taken into account in MCOL-4464.
Adding proper initializations.
For TIMESTAMP, it should do similar. However, it didn't work. For some reason, MDB has the function set as DATETIME, which for cs, isn't the same thing. Added a kludge to ha_mcs_execplan.cpp to handle it.
MCOL-4409 This patch combines VDecimal and Decimal and makes
IDB_Decimal an alias for the result class
MCOL-4409 More boilerplate reduction in Func_mod
Removed couple TSInt128::toType() methods
1. In TupleAggregateStep::configDeliveredRowGroup(), use
jobInfo.projectionCols instead of jobInfo.nonConstCols
for setting scale and precision if the source column is
wide decimal.
2. Tighten rules for wide decimal processing. Specifically:
a. Replace (precision > INT64MAXPRECISION) checks with
(precision > INT64MAXPRECISION && precision <= INT128MAXPRECISION)
b. At places where (colWidth == MAXDECIMALWIDTH) is not enough to
determine if a column is wide decimal or not, also add a check on
type being DECIMAL/UDECIMAL.
