More convenient usage of big integers and ORDER BY WITH FILL.#46152
More convenient usage of big integers and ORDER BY WITH FILL.#46152alexey-milovidov merged 12 commits intomasterfrom
Conversation
|
I also noticed that the code around type conversion is stupid. |
|
@alexey-milovidov seems there is an exception in debug build: |
|
@yakov-olkhovskiy I want WITH FILL to use |
|
But I don't understand - whether we can change the types after WITH FILL. |
|
Looks like no. |
|
And there are some complications with STEP. |
I think it's done this way to simplify step function generation |
|
Ready for review. |
|
|
||
| T operator() (const UInt64 & x) const { return T(x); } | ||
| T operator() (const Int64 & x) const { return T(x); } | ||
| T operator() (const Int128 & x) const { return T(x); } |
There was a problem hiding this comment.
Here were leftovers from when we used UInt128 for UUID a long time ago. I've cleaned it up. Some bugs can be automatically fixed by this change...
| if constexpr (std::is_same_v<T, U>) | ||
| return l == r; | ||
|
|
||
| if constexpr (std::is_arithmetic_v<T> && std::is_arithmetic_v<U>) |
There was a problem hiding this comment.
This enables comparison for big integers.
| size_t pos = 0; | ||
|
|
||
| /// Find position we need to increment for generating next row. | ||
| for (; pos < size(); ++pos) |
There was a problem hiding this comment.
This is a minor change, but the compiler cannot typically optimize size calls out of the loop due to not enough strict-aliasing rules in C++ (in comparison to Rust).
| for (auto & column : header) | ||
| if (column.column && isColumnConst(*column.column) && !sort_keys.contains(column.name)) | ||
| column.column = column.type->createColumn(); | ||
| column.column = column.column->convertToFullColumnIfConst(); |
There was a problem hiding this comment.
This is more natural way to do the same.
| WhichDataType which_from(descr.fill_from_type); | ||
| if ((which_from.isDateOrDate32() || which_from.isDateTime() || which_from.isDateTime64()) && | ||
| !descr.fill_from_type->equals(*type)) | ||
| !descr.fill_from_type->equals(*removeNullable(type))) |
There was a problem hiding this comment.
Enables WITH FILL when the type is Nullable, see the test.
|
|
||
| /// TODO Wrong results for big integers. | ||
| if (isInteger(type) || which.isDate() || which.isDate32() || which.isDateTime()) | ||
| if (which.isInt128() || which.isUInt128()) |
There was a problem hiding this comment.
Implemented TODO.
The code remains tricky, maybe it's worth simplifying later.
| descr.fill_from = convertFieldToType(descr.fill_from, *to_type); | ||
| descr.fill_to = convertFieldToType(descr.fill_to, *to_type); | ||
| descr.fill_step = convertFieldToType(descr.fill_step, *to_type); | ||
| if (!descr.fill_from.isNull()) |
There was a problem hiding this comment.
This is much better - we are explicitly asking if the field is convertible, instead of having a risk it is confused with NULL, which has another meaning for the WITH FILL logic.
| auto & descr = filling_row.getFillDescription(i); | ||
| const auto & type = header_.getByPosition(block_position).type; | ||
|
|
||
| const Block & output_header = getOutputPort().getHeader(); |
There was a problem hiding this comment.
Taking it from the output header is unneeded, as it is currently identical to the input header by the data types, but I was not sure about it and added this code. It is more natural, so worth keeping this change.
| const auto & type = header_.getByPosition(block_position).type; | ||
|
|
||
| const Block & output_header = getOutputPort().getHeader(); | ||
| const DataTypePtr & type = removeNullable(output_header.getByPosition(block_position).type); |
There was a problem hiding this comment.
Support for Nullable.
| "Incompatible types of WITH FILL expression values with column type {}", type->getName()); | ||
|
|
||
| if (type->isValueRepresentedByUnsignedInteger() && | ||
| if (isUnsignedInteger(type) && |
There was a problem hiding this comment.
Support for big integers.
|
|
||
| if (!is_fill_column[idx] && !(interpolate_description && interpolate_description->result_columns_set.contains(column.name))) | ||
| other_column_positions.push_back(idx); | ||
| other_column_positions.push_back(idx); |
| { | ||
| insertFromFillingRow(res_fill_columns, res_interpolate_columns, res_other_columns, filling_row, interpolate_block); | ||
| interpolate(); | ||
| insertFromFillingRow(res_fill_columns, res_interpolate_columns, res_other_columns, filling_row, interpolate_block); |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
More convenient usage of big integers and ORDER BY WITH FILL. Allow using plain integers for start and end points in WITH FILL when ORDER BY big (128-bit and 256-bit) integers. Fix the wrong result for big integers with negative start or end points. This closes #16733
See also the assertion about unexpected type of exception in debug build https://s3.amazonaws.com/clickhouse-test-reports/46149/a69f9c05a981a6c95af496b47413e540c87e2c09/fuzzer_astfuzzerdebug/report.html