Add kRange preceding/following frames in window fuzzer #10006

pramodsatya · 2024-06-02T03:57:15Z

Adds support for kRange preceding/following frames in window fuzzer
Adds reference query runner context for PrestoSql frame clause for kRange
preceding/following frames.

Resolves #9572 .

netlify · 2024-06-02T03:57:38Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`906d532`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6700320b91067500082ab351

pramodsatya · 2024-06-11T18:29:01Z

Hi @aditi-pandit, could you please help review these changes? The second commit contains changes specific to query runner context.

velox/exec/fuzzer/ReferenceQueryRunner.h

kagamiori

Hi @pramodsatya, thank you for putting together this draft! I left some comments. Have you tried whether the current code already work properly with PrestoQueryRunner?

velox/exec/fuzzer/AggregationFuzzerBase.cpp

velox/exec/tests/utils/QueryAssertions.cpp

velox/exec/fuzzer/WindowFuzzer.cpp

velox/exec/fuzzer/AggregationFuzzerBase.cpp

kagamiori · 2024-06-19T00:55:20Z

velox/exec/fuzzer/WindowFuzzer.cpp

+    if constexpr (std::is_same_v<T, double> || std::is_same_v<T, float>) {
+      return offsetCol[idx] - offsetValue;
+    } else {
+      return checkedMinus<T>(offsetCol[idx], offsetValue);
    }


This piece of code is similar to lines 139-143. Could we reuse the code?

velox/exec/fuzzer/WindowFuzzer.cpp

pramodsatya

Thanks for the feedback @kagamiori. We found some result mismatches with Presto while testing with these changes and are investigating them further.

velox/exec/fuzzer/AggregationFuzzerBase.cpp

velox/exec/fuzzer/ReferenceQueryRunner.h

velox/exec/fuzzer/WindowFuzzer.cpp

velox/exec/tests/utils/QueryAssertions.cpp

aditi-pandit

Thanks @pramodsatya. This is beginning to look solid.

velox/exec/fuzzer/AggregationFuzzerBase.cpp

velox/exec/fuzzer/ReferenceQueryRunner.h

velox/exec/fuzzer/WindowFuzzer.cpp

aditi-pandit · 2024-06-28T20:47:14Z

velox/exec/fuzzer/WindowFuzzer.cpp

+                  std::is_same_v<T, Timestamp> ||
+                  std::is_same_v<T, UnknownValue>)) {
+    auto size = vectorFuzzer_.getOptions().vectorSize;
+    velox::test::VectorMaker vectorMaker{pool_.get()};


Do we really need vectorMaker for the rowVector at the end ? It seems un-necessary.

aditi-pandit · 2024-06-28T20:49:03Z

velox/exec/fuzzer/WindowFuzzer.cpp

+      newNames.push_back(columnName);
+      auto newChildren = input[i]->children();
+      newChildren.push_back(offsetColumn);
+      input[i] = vectorMaker.rowVector(newNames, newChildren);


We can use makeRowVector here. Do we really need vectorMaker ?

This function is in VectorTestBase and we don't derive from this class, vectorMaker is also used in AggregationFuzzerBase::generateInputData. Creating a RowVectorPtr with std::make_shared requires more changes and vectorMaker looks more convenient here. Could you please share if this is fine?

velox/exec/fuzzer/WindowFuzzer.cpp

pramodsatya · 2024-07-17T01:18:23Z

Hi @aditi-pandit, @kagamiori, could you please help review this PR?

aditi-pandit

Thanks @pramodsatya.Only few minor comments left.

velox/exec/fuzzer/AggregationFuzzerBase.cpp

velox/exec/fuzzer/WindowFuzzer.h

velox/exec/fuzzer/WindowFuzzer.cpp

pramodsatya · 2024-07-25T23:57:28Z

Thanks @pramodsatya.Only few minor comments left.

Thanks @aditi-pandit, addressed the comments.

aditi-pandit · 2024-07-31T18:27:39Z

@pramodsatya : Please rebase your code. There is a conflict.

aditi-pandit

Thanks @pramodsatya

pramodsatya · 2024-09-10T23:45:55Z

Hi @pramodsatya, I saw there is a window fuzzer failure in CI. Could you please take a look? https://github.com/facebookincubator/velox/actions/runs/10730400789/job/29760243003?pr=10006

Hi @kagamiori @aditi-pandit, it looks like the failure was because of the check for null values in the frameColumn in function WindowPartition::updateKRangeFrameBounds. The order by column index obtained from sortKeyInfo_, in the variable orderByColumn, already contains the order by column index in the reordered data (since WindowBuild constructs std::unique_ptr<RowContainer> data_; by placing partitionKeys and orderByKeys first). So the orderByRowColumn should be obtained from the inverse map of inputMapping_, so the right index (which maps to sortKeyInfo_.first in the inverse map) is queried in std::vector<exec::RowColumn> columns_; for the null check.

Eg: Input

-- Window[1][partition by [p0, p1] order by [s0 ASC NULLS LAST] w0 := skewness(ROW["c0"]) RANGE between UNBOUNDED PRECEDING and off1 PRECEDING] -> c0:BIGINT, p0:ROW<f0:REAL,f1:INTERVAL DAY TO SECOND,f2:REAL,f3:DATE,f4:INTERVAL DAY TO SECOND,f5:VARCHAR>, p1:ARRAY<BIGINT>, s0:HUGEINT, row_number:BIGINT, off1:HUGEINT, w0:DOUBLE
  -- Values[0][1000 rows in 10 vectors] -> c0:BIGINT, p0:ROW<f0:REAL,f1:INTERVAL DAY TO SECOND,f2:REAL,f3:DATE,f4:INTERVAL DAY TO SECOND,f5:VARCHAR>, p1:ARRAY<BIGINT>, s0:HUGEINT, row_number:BIGINT, off1:HUGEINT

Issue

column_index_t orderByColumn = sortKeyInfo_[0].first;    ==> 2
inputMapping_ : 3 0 1 2 4 5
inputMapping_[orderByColumn] = 1
RowColumn corresponding to orderByColumn is at index 3 in columns_

Could you please take another look?

Update: The recent fuzzer failure is unrelated to this change and the same error is seen on other PRs as well.

kagamiori · 2024-09-19T03:38:00Z

Hi @pramodsatya, is that failure still reproducible? If so, could you share which command you used to reproduce it? Thanks!

pramodsatya · 2024-09-19T03:40:41Z

Hi @pramodsatya, is that failure still reproducible? If so, could you share which command you used to reproduce it? Thanks!

Hi @kagamiori, the failure is not reproducible currently. It can be reproduced by reverting the changes to WindowPartition.cpp in this PR, and with the following command:

velox_window_fuzzer_test --enable_window_reference_verification --presto_url="http://127.0.0.1:8080" --duration_sec=3600 --logtostderr=1 --minloglevel=0 --seed=2483741532 --req_timeout_ms=5000

kagamiori

Hi @pramodsatya, thank you for investigating the fuzzer failure! I can reproduce that bug, and as you explained, it was due to an incorrect use of inputMapping_ in WindowPartition::updateKRangeFrameBounds(). It's a great demonstration that this fuzzer enhancement enables us to catch a real bug.

Since this PR is already big, I wonder if we could separate the bug fix into another PR and add a unit test with that?

velox/exec/WindowPartition.cpp

facebook-github-bot · 2024-09-23T18:18:35Z

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

pramodsatya · 2024-09-23T20:55:43Z

Hi @pramodsatya, thank you for investigating the fuzzer failure! I can reproduce that bug, and as you explained, it was due to an incorrect use of inputMapping_ in WindowPartition::updateKRangeFrameBounds(). It's a great demonstration that this fuzzer enhancement enables us to catch a real bug.

Since this PR is already big, I wonder if we could separate the bug fix into another PR and add a unit test with that?

Thanks for the feedback @kagamiori, addressed the comments and opened another PR for the null check fix: #11075 . Could you please take another look?

facebook-github-bot · 2024-09-30T16:55:22Z

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

pramodsatya · 2024-09-30T18:15:09Z

Hi @kagamiori, after the rebase, window fuzzer is failing with kRange frames when alternative plans are tested, I'm looking into it:

-- Window[2][STREAMING partition by [p0, p1] order by [s0 DESC NULLS FIRST] w0 := checksum(ROW["c0"]) RANGE between off0 FOLLOWING and off1 FOLLOWING] -> c0:INTERVAL DAY TO SECOND, p0:INTERVAL DAY TO SECOND, p1:BOOLEAN, s0:REAL, row_number:BIGINT, off0:REAL, k1:REAL, off1:REAL, w0:VARBINARY
  -- OrderBy[1][p0 ASC NULLS FIRST, p1 ASC NULLS FIRST, s0 DESC NULLS FIRST] -> c0:INTERVAL DAY TO SECOND, p0:INTERVAL DAY TO SECOND, p1:BOOLEAN, s0:REAL, row_number:BIGINT, off0:REAL, k1:REAL, off1:REAL
    -- Values[0][1000 rows in 10 vectors] -> c0:INTERVAL DAY TO SECOND, p0:INTERVAL DAY TO SECOND, p1:BOOLEAN, s0:REAL, row_number:BIGINT, off0:REAL, k1:REAL, off1:REAL

Expected 1000, got 1000
1 extra rows, 1 missing rows
1 of extra rows:
        2933652544787757131 | 4798976694194643045 | true | "Infinity" | 420 | "Infinity" | "NaN" | "NaN" | "Zw90+g6vltk="

1 of missing rows:
        2933652544787757131 | 4798976694194643045 | true | "Infinity" | 420 | "Infinity" | "NaN" | "NaN" | null

kagamiori

LGTM. Thank you for adding this support, iterating on this PR, and fixing the fuzzer-found bug!

velox/vector/fuzzer/VectorFuzzer.h

pramodsatya · 2024-10-04T17:58:45Z

Hi @kagamiori, after the rebase, window fuzzer is failing with kRange frames when alternative plans are tested, I'm looking into it:

-- Window[2][STREAMING partition by [p0, p1] order by [s0 DESC NULLS FIRST] w0 := checksum(ROW["c0"]) RANGE between off0 FOLLOWING and off1 FOLLOWING] -> c0:INTERVAL DAY TO SECOND, p0:INTERVAL DAY TO SECOND, p1:BOOLEAN, s0:REAL, row_number:BIGINT, off0:REAL, k1:REAL, off1:REAL, w0:VARBINARY
  -- OrderBy[1][p0 ASC NULLS FIRST, p1 ASC NULLS FIRST, s0 DESC NULLS FIRST] -> c0:INTERVAL DAY TO SECOND, p0:INTERVAL DAY TO SECOND, p1:BOOLEAN, s0:REAL, row_number:BIGINT, off0:REAL, k1:REAL, off1:REAL
    -- Values[0][1000 rows in 10 vectors] -> c0:INTERVAL DAY TO SECOND, p0:INTERVAL DAY TO SECOND, p1:BOOLEAN, s0:REAL, row_number:BIGINT, off0:REAL, k1:REAL, off1:REAL

Expected 1000, got 1000
1 extra rows, 1 missing rows
1 of extra rows:
        2933652544787757131 | 4798976694194643045 | true | "Infinity" | 420 | "Infinity" | "NaN" | "NaN" | "Zw90+g6vltk="

1 of missing rows:
        2933652544787757131 | 4798976694194643045 | true | "Infinity" | 420 | "Infinity" | "NaN" | "NaN" | null

Hi @kagamiori, we investigated this error and it seems to be because of the recently added changes which enable the vector fuzzer to generate Nan and Infinity values. Nan and Infinity values are now accounted for when constructing the frame offset column and the error is fixed now.
Could you please help review this fix? will squash to a single commit if the change looks good. Thanks!
cc: @minhancao

kagamiori

Hi @pramodsatya, I left one comment. The other part of this fix looks good to me. Thanks!

velox/exec/fuzzer/WindowFuzzer.cpp

Co-authored-by: Minhan Cao <[email protected]>

pramodsatya · 2024-10-04T18:22:53Z

Hi @pramodsatya, I left one comment. The other part of this fix looks good to me. Thanks!

Thanks for the suggestion @kagamiori, updated accordingly. Could you please help merge this change?

facebook-github-bot · 2024-10-04T21:37:59Z

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

kagamiori · 2024-10-08T00:02:33Z

Hi @pramodsatya, FYI, there was an internal linter complaint about defining a static variable in the header file VectorFuzzer.h. So I reverted the change in VectorFuzzer.h and VectorFuzzer.cpp, but instead declare the function const std::vector<TypePtr>& defaultScalarTypes() in VectorFuzzer.h so that AggregationFuzzerBase.cpp can use defaultScalarTypes(). I have checked there was no build issue on my Mac and Linux.

I'm going to start the merging process now.

pramodsatya · 2024-10-08T00:06:10Z

Hi @pramodsatya, FYI, there was an internal linter complaint about defining a static variable in the header file VectorFuzzer.h. So I reverted the change in VectorFuzzer.h and VectorFuzzer.cpp, but instead declare the function const std::vector<TypePtr>& defaultScalarTypes() in VectorFuzzer.h so that AggregationFuzzerBase.cpp can use defaultScalarTypes(). I have checked there was no build issue on my Mac and Linux.

I'm going to start the merging process now.

Thanks for the update @kagamiori, sounds good. Please let me know if any other changes are needed.

facebook-github-bot · 2024-10-08T00:22:59Z

@kagamiori merged this pull request in ce035c8.

conbench-facebook · 2024-10-08T00:53:25Z

Conbench analyzed the 1 benchmark run on commit ce035c87.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Yuhta · 2024-10-10T19:13:06Z

@pramodsatya Window fuzzer has been broken since this change, can you take a look?
https://github.com/facebookincubator/velox/actions/runs/11226206459/job/31207444517

kagamiori · 2024-10-10T21:19:06Z

@pramodsatya Window fuzzer has been broken since this change, can you take a look? https://github.com/facebookincubator/velox/actions/runs/11226206459/job/31207444517

@Yuhta, I'm taking a look at this failure: #11213.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 2, 2024

pramodsatya force-pushed the query_runner_ctx branch from b39549d to 8b23836 Compare June 2, 2024 23:08

minhancao mentioned this pull request Jun 3, 2024

Implementing windows fuzzer k-range frames #9626

Closed

pramodsatya mentioned this pull request Jun 7, 2024

Window functions with kRange frames returns incorrect results #10093

Closed

pramodsatya force-pushed the query_runner_ctx branch from 8b23836 to bf215b9 Compare June 11, 2024 18:26

pramodsatya changed the title ~~[WIP] Add reference query runner context to window fuzzer~~ Add reference query runner context to window fuzzer Jun 11, 2024

aditi-pandit reviewed Jun 11, 2024

View reviewed changes

velox/exec/fuzzer/ReferenceQueryRunner.h Outdated Show resolved Hide resolved

pramodsatya force-pushed the query_runner_ctx branch from bf215b9 to 2f7e509 Compare June 13, 2024 16:24

kagamiori reviewed Jun 19, 2024

View reviewed changes

pramodsatya force-pushed the query_runner_ctx branch from 2f7e509 to d3b207d Compare June 19, 2024 02:45

pramodsatya commented Jun 19, 2024

View reviewed changes

pramodsatya force-pushed the query_runner_ctx branch 3 times, most recently from 93ddd89 to 09e547b Compare June 28, 2024 03:01

pramodsatya requested a review from aditi-pandit June 28, 2024 03:01

pramodsatya marked this pull request as ready for review June 28, 2024 03:04

pramodsatya force-pushed the query_runner_ctx branch from 09e547b to 86393e2 Compare June 28, 2024 03:08

aditi-pandit reviewed Jun 28, 2024

View reviewed changes

pramodsatya force-pushed the query_runner_ctx branch from 86393e2 to 46fd27a Compare July 3, 2024 03:25

pramodsatya force-pushed the query_runner_ctx branch from 46fd27a to 2779a99 Compare July 16, 2024 23:28

aditi-pandit reviewed Jul 17, 2024

View reviewed changes

velox/exec/fuzzer/AggregationFuzzerBase.cpp Outdated Show resolved Hide resolved

velox/exec/fuzzer/WindowFuzzer.h Outdated Show resolved Hide resolved

velox/exec/fuzzer/WindowFuzzer.cpp Outdated Show resolved Hide resolved

velox/exec/fuzzer/WindowFuzzer.cpp Outdated Show resolved Hide resolved

aditi-pandit changed the title ~~Add reference query runner context to window fuzzer~~ Add kRange preceding/following frames in window fuzzer Jul 23, 2024

pramodsatya force-pushed the query_runner_ctx branch from 2779a99 to bb039c8 Compare July 25, 2024 23:57

aditi-pandit approved these changes Jul 31, 2024

View reviewed changes

pramodsatya force-pushed the query_runner_ctx branch from bb039c8 to 9ebf74f Compare July 31, 2024 19:18

kagamiori reviewed Sep 23, 2024

View reviewed changes

velox/exec/WindowPartition.cpp Outdated Show resolved Hide resolved

pramodsatya force-pushed the query_runner_ctx branch from ad8764e to b836009 Compare September 23, 2024 20:54

pramodsatya force-pushed the query_runner_ctx branch from b836009 to 0f8dafd Compare September 26, 2024 22:13

kagamiori reviewed Sep 30, 2024

View reviewed changes

velox/vector/fuzzer/VectorFuzzer.h Outdated Show resolved Hide resolved

kagamiori approved these changes Sep 30, 2024

View reviewed changes

pramodsatya force-pushed the query_runner_ctx branch from 0f8dafd to b962ca9 Compare October 4, 2024 16:31

kagamiori reviewed Oct 4, 2024

View reviewed changes

velox/exec/fuzzer/WindowFuzzer.cpp Outdated Show resolved Hide resolved

Add kRange preceding/following frames in window fuzzer

906d532

Co-authored-by: Minhan Cao <[email protected]>

pramodsatya force-pushed the query_runner_ctx branch from b962ca9 to 906d532 Compare October 4, 2024 18:20

facebook-github-bot closed this in ce035c8 Oct 8, 2024

facebook-github-bot added the Merged label Oct 8, 2024

pramodsatya deleted the query_runner_ctx branch October 8, 2024 00:57

kevinwilfong mentioned this pull request Oct 10, 2024

Window fuzzer test is flaky in nightly runs #11213

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add kRange preceding/following frames in window fuzzer #10006

Add kRange preceding/following frames in window fuzzer #10006

pramodsatya commented Jun 2, 2024 •

edited

Loading

netlify bot commented Jun 2, 2024 •

edited

Loading

pramodsatya commented Jun 11, 2024

kagamiori left a comment

kagamiori Jun 19, 2024

pramodsatya left a comment

aditi-pandit left a comment

aditi-pandit Jun 28, 2024

aditi-pandit Jun 28, 2024

pramodsatya Jul 2, 2024

pramodsatya commented Jul 17, 2024

aditi-pandit left a comment

pramodsatya commented Jul 25, 2024

aditi-pandit commented Jul 31, 2024

aditi-pandit left a comment

pramodsatya commented Sep 10, 2024 •

edited

Loading

kagamiori commented Sep 19, 2024

pramodsatya commented Sep 19, 2024

kagamiori left a comment

facebook-github-bot commented Sep 23, 2024

pramodsatya commented Sep 23, 2024

facebook-github-bot commented Sep 30, 2024

pramodsatya commented Sep 30, 2024

kagamiori left a comment

pramodsatya commented Oct 4, 2024

kagamiori left a comment

pramodsatya commented Oct 4, 2024

facebook-github-bot commented Oct 4, 2024

kagamiori commented Oct 8, 2024 •

edited

Loading

pramodsatya commented Oct 8, 2024

facebook-github-bot commented Oct 8, 2024

conbench-facebook bot commented Oct 8, 2024

Yuhta commented Oct 10, 2024

kagamiori commented Oct 10, 2024

Add kRange preceding/following frames in window fuzzer #10006

Add kRange preceding/following frames in window fuzzer #10006

Conversation

pramodsatya commented Jun 2, 2024 • edited Loading

netlify bot commented Jun 2, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

pramodsatya commented Jun 11, 2024

kagamiori left a comment

Choose a reason for hiding this comment

kagamiori Jun 19, 2024

Choose a reason for hiding this comment

pramodsatya left a comment

Choose a reason for hiding this comment

aditi-pandit left a comment

Choose a reason for hiding this comment

aditi-pandit Jun 28, 2024

Choose a reason for hiding this comment

aditi-pandit Jun 28, 2024

Choose a reason for hiding this comment

pramodsatya Jul 2, 2024

Choose a reason for hiding this comment

pramodsatya commented Jul 17, 2024

aditi-pandit left a comment

Choose a reason for hiding this comment

pramodsatya commented Jul 25, 2024

aditi-pandit commented Jul 31, 2024

aditi-pandit left a comment

Choose a reason for hiding this comment

pramodsatya commented Sep 10, 2024 • edited Loading

kagamiori commented Sep 19, 2024

pramodsatya commented Sep 19, 2024

kagamiori left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 23, 2024

pramodsatya commented Sep 23, 2024

facebook-github-bot commented Sep 30, 2024

pramodsatya commented Sep 30, 2024

kagamiori left a comment

Choose a reason for hiding this comment

pramodsatya commented Oct 4, 2024

kagamiori left a comment

Choose a reason for hiding this comment

pramodsatya commented Oct 4, 2024

facebook-github-bot commented Oct 4, 2024

kagamiori commented Oct 8, 2024 • edited Loading

pramodsatya commented Oct 8, 2024

facebook-github-bot commented Oct 8, 2024

conbench-facebook bot commented Oct 8, 2024

Yuhta commented Oct 10, 2024

kagamiori commented Oct 10, 2024

pramodsatya commented Jun 2, 2024 •

edited

Loading

netlify bot commented Jun 2, 2024 •

edited

Loading

pramodsatya commented Sep 10, 2024 •

edited

Loading

kagamiori commented Oct 8, 2024 •

edited

Loading