From 6e40b9fb6c0eb0bfa2188af0fa146a55b79b4ad3 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Tue, 9 Feb 2021 14:56:11 +0300 Subject: [PATCH] fix for the DESC frame --- src/Interpreters/WindowDescription.cpp | 16 ++- src/Interpreters/WindowDescription.h | 8 +- src/Processors/Transforms/WindowTransform.cpp | 111 +++++++----------- .../01591_window_functions.reference | 49 ++++++++ .../0_stateless/01591_window_functions.sql | 19 +++ 5 files changed, 127 insertions(+), 76 deletions(-) diff --git a/src/Interpreters/WindowDescription.cpp b/src/Interpreters/WindowDescription.cpp index b5fe91188fd..a410ba0de1b 100644 --- a/src/Interpreters/WindowDescription.cpp +++ b/src/Interpreters/WindowDescription.cpp @@ -54,7 +54,9 @@ void WindowFrame::toString(WriteBuffer & buf) const } else if (begin_type == BoundaryType::Unbounded) { - buf << "UNBOUNDED PRECEDING"; + buf << "UNBOUNDED"; + buf << " " + << (begin_preceding ? "PRECEDING" : "FOLLOWING"); } else { @@ -69,7 +71,9 @@ void WindowFrame::toString(WriteBuffer & buf) const } else if (end_type == BoundaryType::Unbounded) { - buf << "UNBOUNDED PRECEDING"; + buf << "UNBOUNDED"; + buf << " " + << (end_preceding ? "PRECEDING" : "FOLLOWING"); } else { @@ -119,10 +123,10 @@ void WindowFrame::checkValid() const { if (!(end_preceding && !begin_preceding)) { - if (begin_offset <= end_offset) - { - return; - } + // Should probably check here that starting offset is less than + // ending offset, but with regard to ORDER BY direction for RANGE + // frames. + return; } } diff --git a/src/Interpreters/WindowDescription.h b/src/Interpreters/WindowDescription.h index 9388a4c7cf8..ee181680b94 100644 --- a/src/Interpreters/WindowDescription.h +++ b/src/Interpreters/WindowDescription.h @@ -38,8 +38,9 @@ struct WindowFrame FrameType type = FrameType::Range; - // UNBOUNDED FOLLOWING for the frame end doesn't make much sense, so - // Unbounded here means UNBOUNDED PRECEDING. + // UNBOUNDED FOLLOWING for the frame end is forbidden by the standard, but for + // uniformity the begin_preceding still has to be set to true for UNBOUNDED + // frame start. // Offset might be both preceding and following, controlled by begin_preceding, // but the offset value must be positive. BoundaryType begin_type = BoundaryType::Unbounded; @@ -47,7 +48,8 @@ struct WindowFrame int64_t begin_offset = 0; bool begin_preceding = true; - // Here as well, Unbounded is UNBOUNDED FOLLOWING. + // Here as well, Unbounded can only be UNBOUNDED FOLLOWING, and end_preceding + // must be false. BoundaryType end_type = BoundaryType::Current; int64_t end_offset = 0; bool end_preceding = false; diff --git a/src/Processors/Transforms/WindowTransform.cpp b/src/Processors/Transforms/WindowTransform.cpp index bb52b8779ba..944c8047be6 100644 --- a/src/Processors/Transforms/WindowTransform.cpp +++ b/src/Processors/Transforms/WindowTransform.cpp @@ -63,38 +63,6 @@ WindowTransform::WindowTransform(const Block & input_header_, order_by_indices.push_back( input_header.getPositionByName(column.column_name)); } - -// FIXME this is just all wrong. Disabled desc order for now. - const auto & frame = window_description.frame; - if (frame.type == WindowFrame::FrameType::Range - && window_description.order_by.size() == 1 - && window_description.order_by[0].direction < 0 - && (frame.begin_type == WindowFrame::BoundaryType::Offset - || frame.end_type == WindowFrame::BoundaryType::Offset)) - { - throw Exception(ErrorCodes::NOT_IMPLEMENTED, - "ORDER BY DESC for RANGE OFFSET frames is not implemented"); - } -// // If we have at least one RANGE OFFSET frame boundary, no UNBOUNDED frame -// // boundaries, and the ORDER BY is DESC, we have to swap the frame end -// // and frame start. This is tricky and I'm not sure how to explain the -// // reason, so I can only suggest to draw various RANGE OFFSET frames with -// // ASC and DESC orders to understand... -// // swap is a no-op if both frames are CURRENT ROW, so use a simpler condition. -// auto & frame = window_description.frame; -// if (frame.type == WindowFrame::FrameType::Range -// && (frame.end_type != WindowFrame::BoundaryType::Unbounded -// && frame.begin_type != WindowFrame::BoundaryType::Unbounded) -// && window_description.order_by.size() == 1 -// && window_description.order_by[0].direction < 0) -// { -// std::swap(frame.begin_type, frame.end_type); -// std::swap(frame.begin_preceding, frame.end_preceding); -// std::swap(frame.begin_offset, frame.end_offset); -// -// fmt::print(stderr, "swapped frame boundaries\n"); -// } - } WindowTransform::~WindowTransform() @@ -357,11 +325,12 @@ static int compareValuesWithOffset(const ColumnType * compared_column, overflow_to_negative = offset < 0; } - fmt::print(stderr, - "compared [{}] = {}, ref [{}] = {}, offset {} overflow {} to negative {}\n", - compared_row, toString(compared_value), - reference_row, toString(reference_value), - toString(offset), is_overflow, overflow_to_negative); +// fmt::print(stderr, +// "compared [{}] = {}, ref [{}] = {}, offset {} preceding {} overflow {} to negative {}\n", +// compared_row, toString(compared_value), +// reference_row, toString(reference_value), +// toString(offset), offset_is_preceding, +// is_overflow, overflow_to_negative); if (is_overflow) { @@ -387,7 +356,10 @@ static int compareValuesWithOffset(const ColumnType * compared_column, template void WindowTransform::advanceFrameStartRangeOffset() { + // See the comment for advanceFrameEndRangeOffset(). const int direction = window_description.order_by[0].direction; + const bool preceding = window_description.frame.begin_preceding + == (direction > 0); const auto * reference_column = assert_cast( inputAt(current_row)[order_by_indices[0]].get()); for (; frame_start < partition_end; advanceRowNumber(frame_start)) @@ -399,7 +371,7 @@ void WindowTransform::advanceFrameStartRangeOffset() if (compareValuesWithOffset(compared_column, frame_start.row, reference_column, current_row.row, window_description.frame.begin_offset, - window_description.frame.begin_preceding) + preceding) * direction >= 0) { frame_started = true; @@ -410,26 +382,40 @@ void WindowTransform::advanceFrameStartRangeOffset() frame_started = partition_ended; } +// Helper macros to dispatch on type of the ORDER BY column +#define APPLY_FOR_ONE_TYPE(FUNCTION, TYPE) \ +else if (typeid_cast(column)) \ +{ \ + FUNCTION(); \ +} + +#define APPLY_FOR_TYPES(FUNCTION) \ +if (false) /* NOLINT */ \ +{ \ + /* Do nothing, a starter condition. */ \ +} \ +APPLY_FOR_ONE_TYPE(FUNCTION, ColumnVector) \ +APPLY_FOR_ONE_TYPE(FUNCTION, ColumnVector) \ +APPLY_FOR_ONE_TYPE(FUNCTION, ColumnVector) \ +APPLY_FOR_ONE_TYPE(FUNCTION, ColumnVector) \ +APPLY_FOR_ONE_TYPE(FUNCTION, ColumnVector) \ +APPLY_FOR_ONE_TYPE(FUNCTION, ColumnVector) \ +APPLY_FOR_ONE_TYPE(FUNCTION, ColumnVector) \ +APPLY_FOR_ONE_TYPE(FUNCTION, ColumnVector) \ +else \ +{ \ + throw Exception(ErrorCodes::NOT_IMPLEMENTED, \ + "The RANGE OFFSET frame for '{}' ORDER BY column is not implemented", \ + demangle(typeid(*column).name())); \ +} + void WindowTransform::advanceFrameStartRangeOffsetDispatch() { // Dispatch on the type of the ORDER BY column. assert(order_by_indices.size() == 1); const IColumn * column = inputAt(current_row)[order_by_indices[0]].get(); - if (typeid_cast *>(column)) - { - advanceFrameStartRangeOffset>(); - } - else if (typeid_cast *>(column)) - { - advanceFrameStartRangeOffset>(); - } - else - { - throw Exception(ErrorCodes::NOT_IMPLEMENTED, - "The RANGE OFFSET frame start for '{}' ORDER BY column is not implemented", - demangle(typeid(*column).name())); - } + APPLY_FOR_TYPES(advanceFrameStartRangeOffset) } void WindowTransform::advanceFrameStart() @@ -647,7 +633,11 @@ void WindowTransform::advanceFrameEndRowsOffset() template void WindowTransform::advanceFrameEndRangeOffset() { + // PRECEDING/FOLLOWING change direction for DESC order. + // See CD 9075-2:201?(E) 7.14 p. 429. const int direction = window_description.order_by[0].direction; + const bool preceding = window_description.frame.end_preceding + == (direction > 0); const auto * reference_column = assert_cast( inputAt(current_row)[order_by_indices[0]].get()); for (; frame_end < partition_end; advanceRowNumber(frame_end)) @@ -660,7 +650,7 @@ void WindowTransform::advanceFrameEndRangeOffset() if (compareValuesWithOffset(compared_column, frame_end.row, reference_column, current_row.row, window_description.frame.end_offset, - window_description.frame.end_preceding) + preceding) * direction > 0) { frame_ended = true; @@ -677,20 +667,7 @@ void WindowTransform::advanceFrameEndRangeOffsetDispatch() assert(order_by_indices.size() == 1); const IColumn * column = inputAt(current_row)[order_by_indices[0]].get(); - if (typeid_cast *>(column)) - { - advanceFrameEndRangeOffset>(); - } - else if (typeid_cast *>(column)) - { - advanceFrameEndRangeOffset>(); - } - else - { - throw Exception(ErrorCodes::NOT_IMPLEMENTED, - "The RANGE OFFSET frame end for '{}' ORDER BY column is not implemented", - demangle(typeid(*column).name())); - } + APPLY_FOR_TYPES(advanceFrameEndRangeOffset) } void WindowTransform::advanceFrameEnd() diff --git a/tests/queries/0_stateless/01591_window_functions.reference b/tests/queries/0_stateless/01591_window_functions.reference index b8b3ae17830..4ce4503c3ca 100644 --- a/tests/queries/0_stateless/01591_window_functions.reference +++ b/tests/queries/0_stateless/01591_window_functions.reference @@ -771,3 +771,52 @@ order by x; 125 124 127 4 126 125 127 3 127 126 127 2 +-- RANGE OFFSET ORDER BY DESC +select x, min(x) over w, max(x) over w, count(x) over w from ( + select toUInt8(number) x from numbers(11)) t +window w as (order by x desc range between 1 preceding and 2 following) +order by x +settings max_block_size = 1; +0 0 1 2 +1 0 2 3 +2 0 3 4 +3 1 4 4 +4 2 5 4 +5 3 6 4 +6 4 7 4 +7 5 8 4 +8 6 9 4 +9 7 10 4 +10 8 10 3 +select x, min(x) over w, max(x) over w, count(x) over w from ( + select toUInt8(number) x from numbers(11)) t +window w as (order by x desc range between 1 preceding and unbounded following) +order by x +settings max_block_size = 2; +0 0 1 2 +1 0 2 3 +2 0 3 4 +3 0 4 5 +4 0 5 6 +5 0 6 7 +6 0 7 8 +7 0 8 9 +8 0 9 10 +9 0 10 11 +10 0 10 11 +select x, min(x) over w, max(x) over w, count(x) over w from ( + select toUInt8(number) x from numbers(11)) t +window w as (order by x desc range between unbounded preceding and 2 following) +order by x +settings max_block_size = 3; +0 0 10 11 +1 0 10 11 +2 0 10 11 +3 1 10 10 +4 2 10 9 +5 3 10 8 +6 4 10 7 +7 5 10 6 +8 6 10 5 +9 7 10 4 +10 8 10 3 diff --git a/tests/queries/0_stateless/01591_window_functions.sql b/tests/queries/0_stateless/01591_window_functions.sql index cf2dd28a54c..8ac07e2df2a 100644 --- a/tests/queries/0_stateless/01591_window_functions.sql +++ b/tests/queries/0_stateless/01591_window_functions.sql @@ -241,3 +241,22 @@ from ( ) window w as (order by x range between 1 preceding and 2 following) order by x; + +-- RANGE OFFSET ORDER BY DESC +select x, min(x) over w, max(x) over w, count(x) over w from ( + select toUInt8(number) x from numbers(11)) t +window w as (order by x desc range between 1 preceding and 2 following) +order by x +settings max_block_size = 1; + +select x, min(x) over w, max(x) over w, count(x) over w from ( + select toUInt8(number) x from numbers(11)) t +window w as (order by x desc range between 1 preceding and unbounded following) +order by x +settings max_block_size = 2; + +select x, min(x) over w, max(x) over w, count(x) over w from ( + select toUInt8(number) x from numbers(11)) t +window w as (order by x desc range between unbounded preceding and 2 following) +order by x +settings max_block_size = 3;