feat(joblist,sorting): MCOL-3758 new ORDER BY algo

[drrtuy]

No description provided.

[mariadb-LeonidFedorov]

/gemini review

[mariadb-LeonidFedorov]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces new sorting algorithms, including PDQ sort and a heap-based merge sort, to enhance ORDER BY performance, particularly for large datasets. The changes are substantial and well-structured, with new sorting implementations, a factory method in TupleAnnexStep to select the appropriate algorithm, and supporting data structures. While this is a valuable feature, I've identified a couple of critical issues that need to be addressed before merging. Specifically, there's a memory leak in HeapOrderBy due to incorrect std::unique_ptr usage and a significant bug in the sorting key generation for unsigned integers that will lead to incorrect results. I have also included several medium-severity recommendations to improve code quality and maintainability.

[gemini-code-assist]

The logic for creating a sortable key for unsigned integers is incorrect. The code performs a sign-bit flip (*valPtr ^= 0x80;), which is a technique for signed integers to make them comparable as unsigned. For unsigned integers, this will produce an incorrect sort order. For example, for uint8_t, 128 (binary 10000000) would become 0, and 0 would become 128, causing 128 to be sorted before 0 in an ascending sort. This line should be removed. This issue also exists for USMALLINT, UINT, and UBIGINT types in this file.

[gemini-code-assist]

keyBuf_ is declared as std::unique_ptr<uint8_t>, but it's allocated as an array using new uint8_t[...] in heaporderby.cpp. This will lead to a memory leak because the default deleter for std::unique_ptr<T> calls delete instead of delete[], deallocating only the first element of the array. To fix this, you should declare keyBuf_ as std::unique_ptr<uint8_t[]>.

Suggested change

	std::unique_ptr<uint8_t> keyBuf_;
	std::unique_ptr<uint8_t[]> keyBuf_;

[gemini-code-assist]

These enum values are commented out. If they are not intended for use in the near future, they should be removed to avoid cluttering the code and confusing future maintainers.

[gemini-code-assist]

The operator== takes its TSInt128 parameter by value. For a 128-bit type, this causes an unnecessary copy. It would be more efficient to pass it by const reference.

  inline bool operator==(const TSInt128& x) const

[gemini-code-assist]

The KeyType constructor is very long and contains a lot of duplicated code for handling different integer types (e.g., TINYINT, SMALLINT, INT, BIGINT and their unsigned versions). This reduces maintainability. Consider refactoring this into a template function that can handle all integer types, parameterized by the type and size, to reduce code duplication.

[gemini-code-assist]

There are several std::cout statements used for performance logging throughout this file (and others in this PR). These should be removed or replaced with a proper logging framework before merging into a main branch to avoid polluting the output.

[gemini-code-assist]

The perm vector is populated inside a loop using perm.insert(). This can be inefficient as it may lead to multiple reallocations if the final size is large. It would be more performant to first calculate the total number of elements that will be inserted, call perm.reserve() to allocate the required memory at once, and then use perm.insert() or std::copy.

[mariadb-LeonidFedorov]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a new parallel sorting mechanism for ORDER BY operations, leveraging PDQOrderBy for initial per-thread sorting and HeapOrderBy for merging results. Key changes include adding cstdint for integral type definitions, refining decimal width constants, and implementing TSInt128 comparison. The core sorting logic in heaporderby.cpp and pdqorderby.cpp handles various data types, null values, and byte-swapping for correct ordering. The TupleAnnexStep is updated to integrate these new sorting strategies, dynamically choosing between single-threaded LimitedOrderBy, single-threaded PDQOrderBy, or a two-phase parallel PDQOrderBy and HeapOrderBy approach based on the number of sorting threads and data size. Review comments highlight a critical memory leak in HeapOrderBy due to incorrect unique_ptr usage for arrays, potential complexity and bugs in ranges_ vector population, inconsistent formatting in template specializations, significant code duplication in integer type handling within heaporderby.cpp, inefficient use of std::mutex for a counter that could be atomic, and performance concerns with perm.insert() in a loop.

[gemini-code-assist]

This unique_ptr is managing a dynamically allocated array created with new[] in heaporderby.cpp. However, it is not using the array form of unique_ptr (std::unique_ptr<uint8_t[]>). This will result in delete being called instead of delete[], leading to a memory leak and undefined behavior. This is a critical issue that must be fixed.

Suggested change

	std::unique_ptr<uint8_t> keyBuf_;
	std::unique_ptr<uint8_t[]> keyBuf_;

[gemini-code-assist]

The logic for populating the ranges_ vector seems overly complex and potentially buggy. If rangeSize > 0, the PermutationVec will contain ImpossiblePermute at the beginning, followed by the actual permutations from srcPermBegin. This is likely not the intended behavior. A cleaner and more correct approach would be to create the vector with the correct elements from the start.

    if (rangeSize)
    {
      ranges_.emplace_back(srcPermBegin, srcPermEnd);
    }
    else
    {
      ranges_.push_back({ImpossiblePermute});
    }

[gemini-code-assist]

These commented-out enum values seem to be for future work. Leaving commented-out code can be confusing for future maintenance. If these are not yet implemented, it would be better to remove them and add them back in a future PR when they are used. If they are intended as placeholders, a comment explaining their purpose would be helpful.

[gemini-code-assist]

The formatting of the template specializations is inconsistent. In some cases, the base class is on a new line (e.g., line 585), while in others it's on the same line (e.g., line 599). Adopting a consistent style would improve readability. For example, placing the base class on a new line for all specializations.

[gemini-code-assist]

There is significant code duplication in the switch statement for handling various integer types (TINYINT, SMALLINT, INT, BIGINT, and their unsigned counterparts). The logic for building the sort key for these types is nearly identical, differing only in type names, null values, and byte-swapping functions. This could be refactored into a template helper function to reduce duplication and improve maintainability.

[gemini-code-assist]

Using std::mutex to protect a simple counter (fFinishedThreads) is less efficient than using an atomic variable. The lock is held while calculatePivots4phase2 and other operations are performed, which could be a significant amount of time. Consider changing fFinishedThreads to a std::atomic<size_t> and using an atomic fetch-and-add operation to avoid the overhead of locking.

[gemini-code-assist]

The perm.insert() call is inside a loop, which can lead to multiple reallocations and copies of the vector's contents, impacting performance. It would be more efficient to pre-calculate the total number of elements to be inserted and call perm.reserve() once before the loop to allocate the required memory upfront.

[gemini-code-assist]

Code Review

This pull request introduces a new two-phase parallel sorting mechanism using a modified quicksort (PDQOrderBy) and a heap-based merge (HeapOrderBy) for handling large datasets and parallel execution. The changes include adding new PDQOrderBy and HeapOrderBy classes, integrating them into the TupleAnnexStep for query execution, and updating JobInfo to support the new sorting types. The TupleAnnexStep now dynamically selects between LimitedOrderBy, PDQOrderBy (single-threaded), or a two-phase parallel PDQOrderBy and HeapOrderBy based on the limit and thread configuration. Several critical issues were identified in the new sorting logic, including incorrect type handling for string columns in HeapOrderBy and PDQOrderBy, potential out-of-bounds access in modified_pdqsort, uninitialized KeyType objects in HeapOrderBy, and a double function call in modified_pdqsort. Concurrency issues with fRowsReturned and fFinishedThreads were also noted, along with several WIP comments indicating incomplete functionality for string sorting, null handling, and LIMIT/OFFSET in the parallel sorting paths. Additionally, the test for PDQOrderBy was found to be incorrectly configured to test LimitedOrderBy instead.

[gemini-code-assist]

The test file pqorderby-test.cpp is intended to test PDQOrderBy, but tns.addOrderBy(new joblist::LimitedOrderBy()); is being called, which initializes LimitedOrderBy. This means the test is not actually exercising the PDQOrderBy functionality as intended by the file name. This is a critical mismatch and needs to be corrected to test PDQOrderBy.

[gemini-code-assist]

The line mod_partition_right(begin, end, perm_begin, perm_end, comp); is called twice: once as part of the structured binding assignment and then immediately again as a standalone call. This is a bug; the function should only be called once to avoid redundant work and potential side effects.

[gemini-code-assist]

The condition --perm_last != perm_last_2 allows perm_last to potentially go two elements past perm_end (in the reverse direction). This is an out-of-bounds access and will lead to undefined behavior. The loop condition should ensure perm_last stays within valid bounds, typically perm_last >= perm_begin.

[gemini-code-assist]

This switch (columnWidth) block is within the VARCHAR, CHAR, TEXT case, but it attempts to use datatypes::ColDataTypeToIntegralType for these string types. This is fundamentally incorrect as VARCHAR, CHAR, and TEXT are string types, not integral types. This will lead to type mismatches, undefined behavior, or crashes. String types should be handled using string comparison functions, potentially with collation, not by reinterpreting their bytes as integers.

[gemini-code-assist]

The condition --perm_last != perm_last_2 allows perm_last to potentially go two elements past perm_end (in the reverse direction). This is an out-of-bounds access and will lead to undefined behavior. The loop condition should ensure perm_last stays within valid bounds, typically perm_last >= perm_begin.

[gemini-code-assist]

The commented-out code for null value handling (// bool isNull = strings->isNullValue(offset2stringStoreOffset); // if (isNull) // return nullValue;) suggests that null values might not be correctly handled for IsVariadicType. If null strings are possible, this logic should be re-enabled or an alternative null handling mechanism should be in place to prevent incorrect behavior.

[gemini-code-assist]

The comment // check the out of bounds invariant somehow indicates a missing bounds check. Accessing data[valueOffset] without proper bounds checking can lead to buffer overflows or crashes. This is a medium-severity correctness issue.

[gemini-code-assist]

The commented-out code for null value handling (// bool isNull = strings->isNullValue(offset2stringStoreOffset); // if (isNull) // return nullValue;) suggests that null values might not be correctly handled for IsShortString. If null strings are possible, this logic should be re-enabled or an alternative null handling mechanism should be in place to prevent incorrect behavior.

[gemini-code-assist]

The comment // check the out of bounds invariant somehow indicates a missing bounds check. Accessing data[valueOffset] without proper bounds checking can lead to buffer overflows or crashes. This is a medium-severity correctness issue.

[gemini-code-assist]

The comment // check the out of bounds invariant somehow indicates a missing bounds check. Accessing data[valueOffset] without proper bounds checking can lead to buffer overflows or crashes. This is a medium-severity correctness issue.