Revise row-by-row check

PHILO-HE · Mar 14, 2024 · a2c0760 · a2c0760
1 parent d0c413b
commit a2c0760
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 19 deletions.
diff --git a/velox/exec/Window.cpp b/velox/exec/Window.cpp
@@ -285,9 +285,9 @@ void updateKRowsOffsetsColumn(
   int precedingFactor = isKPreceding ? -1 : 1;
   for (auto i = 0; i < numRows; i++) {
     auto startValue = (int64_t)(startRow + i) + precedingFactor * offsets[i];
-    // Considers integer overflow case.
-    if (startValue != (int32_t)startValue) {
-      // computeValidFrames will handle INT32_MAX to compute a valid index.
+    if (startValue > INT32_MAX || startValue < INT32_MIN) {
+      // Integer overflow. computeValidFrames will replace INT32_MAX set here
+      // with partition's final row index.
       rawFrameBounds[i] = startValue < 0 ? 0 : INT32_MAX;
     } else {
       rawFrameBounds[i] = startValue;
@@ -318,28 +318,25 @@ void Window::updateKRowsFrameBounds(
         (isKPreceding ? -constantOffset : constantOffset) + startRow;
 
     if (isKPreceding) {
-      // Considers a very large int64 constantOffset is used.
+      // Overflow.
       if (startValue < INT32_MIN) {
         std::fill_n(rawFrameBounds, numRows, 0);
         return;
       }
-      // Integer overflow cannot happen.
       std::iota(rawFrameBounds, rawFrameBounds + numRows, startValue);
       return;
     }
     // KFollowing.
     auto overflowStart = getOverflowStart(startValue);
     if (overflowStart >= 0 && overflowStart < numRows) {
       std::iota(rawFrameBounds, rawFrameBounds + overflowStart, startValue);
-      // For remaining, use INT32_MAX, which will be converted to valid index
-      // by computeValidFrames.
+      // For remaining rows that overflow happens, use INT32_MAX.
+      // computeValidFrames will replace it with partition's final row index.
       std::fill_n(
           rawFrameBounds + overflowStart, numRows - overflowStart, INT32_MAX);
       return;
     }
-    // Integer overflow cannot happen.
     std::iota(rawFrameBounds, rawFrameBounds + numRows, startValue);
-    return;
   } else {
     currentPartition_->extractColumn(
         frameArg.index, partitionOffset_, numRows, 0, frameArg.value);

diff --git a/velox/functions/prestosql/window/tests/AggregateWindowTest.cpp b/velox/functions/prestosql/window/tests/AggregateWindowTest.cpp
@@ -226,9 +226,9 @@ TEST_F(AggregateWindowTest, integerOverflowRowsFrame) {
        2147483649,
        2147483648});
   auto input = makeRowVector({c0, c1, c2, c3});
-
   std::string overClause = "partition by c0 order by c1 desc";
-  // Test constant following larger than INT32_MAX (2147483647).
+
+  // Constant following larger than INT32_MAX (2147483647).
   std::string frameClause = "rows between 0 preceding and 2147483650 following";
   auto expected = makeRowVector(
       {c0,
@@ -239,8 +239,7 @@ TEST_F(AggregateWindowTest, integerOverflowRowsFrame) {
   WindowTestBase::testWindowFunction(
       {input}, "count(c1)", overClause, frameClause, expected);
 
-  // Test overflow case that happens during the calculation for the middle
-  // partition.
+  // Overflow starts happening from middle of the partition.
   frameClause = "rows between 0 preceding and 2147483645 following";
   expected = makeRowVector(
       {c0,
@@ -251,7 +250,7 @@ TEST_F(AggregateWindowTest, integerOverflowRowsFrame) {
   WindowTestBase::testWindowFunction(
       {input}, "count(c1)", overClause, frameClause, expected);
 
-  // Test with column-specified following (int32).
+  // Column-specified following (int32).
   frameClause = "rows between 0 preceding and c2 following";
   expected = makeRowVector(
       {c0,
@@ -262,7 +261,7 @@ TEST_F(AggregateWindowTest, integerOverflowRowsFrame) {
   WindowTestBase::testWindowFunction(
       {input}, "count(c1)", overClause, frameClause, expected);
 
-  // Test with column-specified following (int64).
+  // Column-specified following (int64).
   frameClause = "rows between 0 preceding and c3 following";
   expected = makeRowVector(
       {c0,
@@ -273,7 +272,7 @@ TEST_F(AggregateWindowTest, integerOverflowRowsFrame) {
   WindowTestBase::testWindowFunction(
       {input}, "count(c1)", overClause, frameClause, expected);
 
-  // Test constant preceding larger than INT32_MAX.
+  // Constant preceding larger than INT32_MAX.
   frameClause = "rows between 2147483650 preceding and 0 following";
   expected = makeRowVector(
       {c0,
@@ -284,7 +283,7 @@ TEST_F(AggregateWindowTest, integerOverflowRowsFrame) {
   WindowTestBase::testWindowFunction(
       {input}, "count(c1)", overClause, frameClause, expected);
 
-  // Test with column-specified preceding (int32).
+  // Column-specified preceding (int32).
   frameClause = "rows between c2 preceding and 0 following";
   expected = makeRowVector(
       {c0,
@@ -295,7 +294,7 @@ TEST_F(AggregateWindowTest, integerOverflowRowsFrame) {
   WindowTestBase::testWindowFunction(
       {input}, "count(c1)", overClause, frameClause, expected);
 
-  // Test with column-specified preceding (int64).
+  // Column-specified preceding (int64).
   frameClause = "rows between c3 preceding and 0 following";
   expected = makeRowVector(
       {c0,
@@ -306,7 +305,7 @@ TEST_F(AggregateWindowTest, integerOverflowRowsFrame) {
   WindowTestBase::testWindowFunction(
       {input}, "count(c1)", overClause, frameClause, expected);
 
-  // Test constant preceding & following both larger than INT32_MAX.
+  // Constant preceding & following both larger than INT32_MAX.
   frameClause = "rows between 2147483650 preceding and 2147483651 following";
   expected = makeRowVector(
       {c0,