more fuzzing and less bugs

programs/client/QueryFuzzer.cpp

@@ -366,6 +366,8 @@ void QueryFuzzer::fuzzWindowFrame(WindowFrame & frame)
         default:
             break;
     }
+
+    frame.is_default = (frame == WindowFrame{});
 }
 
 void QueryFuzzer::fuzz(ASTs & asts)
@@ -465,6 +467,23 @@ void QueryFuzzer::fuzz(ASTPtr & ast)
 
         fuzz(select->children);
     }
+    /*
+     * The time to fuzz the settings has not yet come.
+     * Apparently we don't have any infractructure to validate the values of
+     * the settings, and the first query with max_block_size = -1 breaks
+     * because of overflows here and there.
+     *//*
+     * else if (auto * set = typeid_cast<ASTSetQuery *>(ast.get()))
+     * {
+     *      for (auto & c : set->changes)
+     *      {
+     *          if (fuzz_rand() % 50 == 0)
+     *          {
+     *              c.value = fuzzField(c.value);
+     *          }
+     *      }
+     * }
+     */
     else if (auto * literal = typeid_cast<ASTLiteral *>(ast.get()))
     {
         // There is a caveat with fuzzing the children: many ASTs also keep the

src/Interpreters/WindowDescription.cpp

@@ -33,4 +33,72 @@ std::string WindowDescription::dump() const
     return ss.str();
 }
 
+std::string WindowFrame::toString() const
+{
+    WriteBufferFromOwnString buf;
+    toString(buf);
+    return buf.str();
+}
+
+void WindowFrame::toString(WriteBuffer & buf) const
+{
+    buf << toString(type) << " BETWEEN ";
+    if (begin_type == BoundaryType::Current)
+    {
+        buf << "CURRENT ROW";
+    }
+    else if (begin_type == BoundaryType::Unbounded)
+    {
+        buf << "UNBOUNDED PRECEDING";
+    }
+    else
+    {
+        buf << abs(begin_offset);
+        buf << " "
+            << (begin_offset > 0 ? "FOLLOWING" : "PRECEDING");
+    }
+    buf << " AND ";
+    if (end_type == BoundaryType::Current)
+    {
+        buf << "CURRENT ROW";
+    }
+    else if (end_type == BoundaryType::Unbounded)
+    {
+        buf << "UNBOUNDED PRECEDING";
+    }
+    else
+    {
+        buf << abs(end_offset);
+        buf << " "
+            << (end_offset > 0 ? "FOLLOWING" : "PRECEDING");
+    }
+}
+
+void WindowFrame::checkValid() const
+{
+    if (begin_type == BoundaryType::Unbounded
+        || end_type == BoundaryType::Unbounded)
+    {
+        return;
+    }
+
+    if (begin_type == BoundaryType::Current
+        && end_type == BoundaryType::Offset
+        && end_offset > 0)
+    {
+        return;
+    }
+
+    if (end_type == BoundaryType::Current
+        && begin_type == BoundaryType::Offset
+        && begin_offset < 0)
+    {
+        return;
+    }
+
+    throw Exception(ErrorCodes::BAD_ARGUMENTS,
+        "Window frame '{}' is invalid",
+        toString());
+}
+
 }

src/Interpreters/WindowDescription.h

@@ -53,6 +53,13 @@ struct WindowFrame
     int64_t end_offset = 0;
 
 
+    // Throws BAD_ARGUMENTS exception if the frame definition is incorrect, e.g.
+    // the frame start comes later than the frame end.
+    void checkValid() const;
+
+    std::string toString() const;
+    void toString(WriteBuffer & buf) const;
+
     bool operator == (const WindowFrame & other) const
     {
         // We don't compare is_default because it's not a real property of the

src/Parsers/ExpressionElementParsers.cpp

@@ -640,6 +640,7 @@ static bool tryParseFrameDefinition(ASTWindowDefinition * node, IParser::Pos & p
             }
             else if (keyword_following.ignore(pos, expected))
             {
+                // Positive offset or UNBOUNDED FOLLOWING.
             }
             else
             {

src/Processors/QueryPlan/WindowStep.cpp

@@ -57,6 +57,7 @@ WindowStep::WindowStep(const DataStream & input_stream_,
 {
     // We don't remove any columns, only add, so probably we don't have to update
     // the output DataStream::distinct_columns.
+    window_description.frame.checkValid();
 }
 
 void WindowStep::transformPipeline(QueryPipeline & pipeline)

src/Processors/Transforms/WindowTransform.cpp

@@ -22,6 +22,8 @@ WindowTransform::WindowTransform(const Block & input_header_,
     , input_header(input_header_)
     , window_description(window_description_)
 {
+    window_description.frame.checkValid();
+
     workspaces.reserve(functions.size());
     for (const auto & f : functions)
     {
@@ -210,14 +212,21 @@ auto WindowTransform::moveRowNumberNoCheck(const RowNumber & _x, int offset) con
                 break;
             }
 
+            // Move to the first row in current block. Note that the offset is
+            // negative.
+            offset += x.row;
+            x.row = 0;
+
+            // Move to the last row of the previous block, if we are not at the
+            // first one. Offset also is incremented by one, because we pass over
+            // the first row of this block.
             if (x.block == first_block_number)
             {
                 break;
             }
 
-            // offset is negative
-            offset += (x.row + 1);
             --x.block;
+            offset += 1;
             x.row = blockRowsNumber(x) - 1;
         }
     }
@@ -253,10 +262,10 @@ void WindowTransform::advanceFrameStartRowsOffset()
 
     assertValid(frame_start);
 
-//    fmt::print(stderr, "frame start {} partition start {}\n", frame_start,
-//        partition_start);
+//    fmt::print(stderr, "frame start {} left {} partition start {}\n",
+//        frame_start, offset_left, partition_start);
 
-    if (moved_row <= partition_start)
+    if (frame_start <= partition_start)
     {
         // Got to the beginning of partition and can't go further back.
         frame_start = partition_start;
@@ -269,10 +278,10 @@ void WindowTransform::advanceFrameStartRowsOffset()
     {
         // A FOLLOWING frame start ran into the end of partition.
         frame_started = true;
+        return;
     }
 
     assert(partition_start < frame_start);
-    frame_start = moved_row;
     frame_started = offset_left == 0;
 }
 

tests/queries/0_stateless/01591_window_functions.reference

@@ -558,5 +558,10 @@ settings max_block_size = 2;
 28	5	3	2	1
 29	5	2	1	0
 30	6	1	1	0
+SELECT count(*) OVER (ROWS BETWEEN 2 PRECEDING AND CURRENT ROW) FROM numbers(4);
+1
+2
+3
+3
 -- seen a use-after-free under MSan in this query once
 SELECT number, max(number) OVER (PARTITION BY intDiv(number, 7) ORDER BY number ASC NULLS LAST ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) FROM numbers(1024) SETTINGS max_block_size = 2 FORMAT Null;

tests/queries/0_stateless/01591_window_functions.sql

@@ -175,6 +175,8 @@ from (select number, intDiv(number, 5) p from numbers(31))
 order by p, number
 settings max_block_size = 2;
 
+SELECT count(*) OVER (ROWS BETWEEN 2 PRECEDING AND CURRENT ROW) FROM numbers(4);
+
 -- seen a use-after-free under MSan in this query once
 SELECT number, max(number) OVER (PARTITION BY intDiv(number, 7) ORDER BY number ASC NULLS LAST ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) FROM numbers(1024) SETTINGS max_block_size = 2 FORMAT Null;