fix(tests): Keep an explicit ROWS window frame (facebookincubator#18639)

Differential Revision: D117099096

Pull Request resolved: facebookincubator#18639

Author: mbasmanova

velox/duckdb/conversion/DuckParser.cpp

@@ -1079,8 +1079,13 @@ core::WindowCallExprPtr buildWindowCallExpr(
   }
 
   auto endType = parseBoundType(windowExpr.end);
+  // Without ORDER BY every row is a peer, so a RANGE frame ending at the
+  // current row covers the whole partition. That is the frame a window with no
+  // frame clause gets. A ROWS frame counts rows rather than peers and ends
+  // where it says, so read the DuckDB boundary: `parseBoundType` maps both
+  // spellings of CURRENT ROW to the same bound.
   if (options.correctWindowFrameDefault && orderByKeys.empty() &&
-      endType == core::WindowCallExpr::BoundType::kCurrentRow) {
+      windowExpr.end == WindowBoundary::CURRENT_ROW_RANGE) {
     endType = core::WindowCallExpr::BoundType::kUnboundedFollowing;
   }
 

velox/duckdb/conversion/DuckParser.h

@@ -31,8 +31,9 @@ struct ParseOptions {
 
   // DuckDB defaults the window frame end bound to CURRENT ROW even when ORDER
   // BY is absent. The SQL standard requires UNBOUNDED FOLLOWING in that case.
-  // When true, corrects this default. Cannot distinguish defaulted from
-  // explicit frames, so an explicit CURRENT ROW may be incorrectly overridden.
+  // When true, corrects this default. An explicit RANGE frame ending at the
+  // current row is corrected as well, since unordered it covers the whole
+  // partition either way. An explicit ROWS frame ends where it says.
   bool correctWindowFrameDefault = false;
 
   /// SQL functions could be registered with different prefixes by the user.

velox/duckdb/conversion/tests/DuckParserTest.cpp

@@ -624,6 +624,44 @@ TEST(DuckParserTest, window) {
       parseWindow("nth_value(x, 3) over ()"));
 }
 
+TEST(DuckParserTest, correctWindowFrameDefault) {
+  auto parse = [](const std::string& expr) {
+    ParseOptions options;
+    options.correctWindowFrameDefault = true;
+    return parseWindowExpr(expr, options)->toString();
+  };
+
+  // Without ORDER BY every row is a peer, so a RANGE frame ending at the
+  // current row covers the whole partition. A window with no frame clause
+  // gets that frame.
+  EXPECT_EQ(
+      "row_number() OVER (PARTITION BY \"a\" "
+      "RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING)",
+      parse("row_number() over (partition by a)"));
+  EXPECT_EQ(
+      "row_number() OVER (PARTITION BY \"a\" "
+      "RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING)",
+      parse(
+          "row_number() over (partition by a "
+          "range between unbounded preceding and current row)"));
+
+  // A ROWS frame counts rows rather than peers, so it ends at the current row
+  // even with nothing ordered.
+  EXPECT_EQ(
+      "row_number() OVER (PARTITION BY \"a\" "
+      "ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW)",
+      parse(
+          "row_number() over (partition by a "
+          "rows between unbounded preceding and current row)"));
+
+  // With ORDER BY the peers are the rows that tie, so the default frame ends
+  // at the current row as written.
+  EXPECT_EQ(
+      "row_number() OVER (PARTITION BY \"a\" ORDER BY \"b\" ASC NULLS LAST "
+      "RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW)",
+      parse("row_number() over (partition by a order by b)"));
+}
+
 TEST(DuckParserTest, windowWithIntegerConstant) {
   ParseOptions options;
   options.parseIntegerAsBigint = false;