Fix window operation with kRange frames with column boundary #9257

kagamiori · 2024-03-26T14:38:50Z

Summary:
When Window operator is initialized, WindowBuild reorders the input columns to
place partition keys and sorting keys first before adding input rows to
RowContainer. However, for window operations that use kRange frame with
column boundary, e.g., range between c1 preceding and current row, the original
column index of the frame column before reordering is passed to
computeKRangeFrameBounds(), which leads to incorrect result. This diff fixes
this bug by passing the actual index of the frame column after the reordering to
computeKRangeFrameBounds().

This diff fixes #9229.

Differential Revision: D55351848

facebook-github-bot · 2024-03-26T14:39:00Z

This pull request was exported from Phabricator. Differential Revision: D55351848

netlify · 2024-03-26T14:39:06Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`f2e70fb`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/660b0b2e897c8a000856cfb5

…kincubator#9257) Summary: When Window operator is initialized, WindowBuild reorders the input columns to place partition keys and sorting keys first before adding input rows to RowContainer. However, for window operations that use kRange frame with column boundary, e.g., range between c1 preceding and current row, the original column index of the frame column before reordering is passed to computeKRangeFrameBounds(), which leads to incorrect result. This diff fixes this bug by passing the actual index of the frame column after the reordering to computeKRangeFrameBounds(). This diff fixes facebookincubator#9229. Differential Revision: D55351848

facebook-github-bot · 2024-03-26T16:54:29Z

This pull request was exported from Phabricator. Differential Revision: D55351848

kagamiori · 2024-03-26T18:27:29Z

Hi @aditi-pandit, could you help review this fix? Thanks!

aditi-pandit · 2024-03-26T19:10:32Z

@kagamiori : So your original idea to pass windowBuild_.inputType_ to createWindowFrame is actually a more correct fix imo.

The reason you had other issues is because the wrong type is used to figure indices in multiple places...in sending the function parameter indices to the WindowFunction, in mapping output columns to input columns for getOutput().. There might be other places as well.

We likely didn't encounter these issues because none of our testing flipped order of columns in this way :( So we should incorporate this in the fuzzer as well.

Let me try your test with some fix logic. I'll send you a sample PR in mind in a few hours.

aditi-pandit · 2024-03-26T20:56:22Z

@kagamiori : I debugged this a bit more. Your fix is in the right direction.

The WindowBuild code had constructed WindowPartitions (used for function and output indices) providing a column mapping using the input type order. So the function and column indexes were accessed correctly.

The range frame code in WindowPartition needed to use that mapping as well. Since the Window operator code uses the input type indices everywhere its consistent to send the original index from Window->WindowPartition.

See 355e13f for changes over some of the code in this fix.

velox/exec/WindowBuild.cpp

aditi-pandit · 2024-03-26T21:02:40Z

velox/exec/WindowBuild.h

@@ -76,6 +76,10 @@ class WindowBuild {
    return data_->estimateRowSize();
  }

+  column_index_t inputChannelOf(column_index_t originalIndex) const {


I don't think this function is needed after the changes in #9266

aditi-pandit · 2024-03-26T21:02:52Z

velox/functions/prestosql/aggregates/tests/SumTest.cpp

@@ -462,5 +462,44 @@ TEST_F(SumTest, distinct) {
      .assertResults("SELECT c0, sum(distinct c1) FROM tmp GROUP BY 1");
 }

+TEST_F(SumTest, aaa) {


Change the test name.

…kincubator#9257) Summary: When Window operator is initialized, WindowBuild reorders the input columns to place partition keys and sorting keys first before adding input rows to RowContainer. However, for window operations that use kRange frame with column boundary, e.g., range between c1 preceding and current row, the original column index of the frame column before reordering is passed to computeKRangeFrameBounds(), which leads to incorrect result. This diff fixes this bug by passing the actual index of the frame column after the reordering to computeKRangeFrameBounds(). This diff fixes facebookincubator#9229. Differential Revision: D55351848

facebook-github-bot · 2024-03-30T17:48:01Z

This pull request was exported from Phabricator. Differential Revision: D55351848

kagamiori · 2024-03-31T00:36:19Z

Hi @aditi-pandit, thanks a lot for the suggestions in #9266! I've updated the PR and addressed your comments. I've also added addition comments about the expected order of columns in WindowBuild::data_ and WindowBuild::inputColumns_ for clarity. I've combined the computation of inversedInputChannels_ with the computation of inputChannels_ and inputType_ together. Please let me know if you would like a different refactoring or have further suggestions. Thanks!

aditi-pandit

Thanks @kagamiori. This is looking very good. One minor comment.

aditi-pandit · 2024-03-31T19:44:16Z

velox/exec/WindowBuild.h

  std::unique_ptr<RowContainer> data_;

  // The decodedInputVectors_ are reused across addInput() calls to decode
  // the partition and sort keys for the above RowContainer.
  std::vector<DecodedVector> decodedInputVectors_;

-  // RowColumns for window build used to construct WindowPartition.
+  // RowColumns for window build used to construct WindowPartition. Columns are
+  // in the same order as the original input before column reordering.
  std::vector<exec::RowColumn> inputColumns_;


Is this variable needed anymore ? We can remove it if so.

aditi-pandit · 2024-04-01T15:36:38Z

velox/functions/prestosql/aggregates/tests/SumTest.cpp

@@ -462,5 +462,66 @@ TEST_F(SumTest, distinct) {
      .assertResults("SELECT c0, sum(distinct c1) FROM tmp GROUP BY 1");
 }

+TEST_F(SumTest, kRangeAndKRowsWithUnorderedKeyColumns) {


Can this test be moved to AggregateWindowTest.cpp ?

…kincubator#9257) Summary: When Window operator is initialized, WindowBuild reorders the input columns to place partition keys and sorting keys first before adding input rows to RowContainer. However, for window operations that use kRange frame with column boundary, e.g., range between c1 preceding and current row, the original column index of the frame column before reordering is passed to computeKRangeFrameBounds(), which leads to incorrect result. This diff fixes this bug by passing the actual index of the frame column after the reordering to computeKRangeFrameBounds(). This diff fixes facebookincubator#9229. Differential Revision: D55351848

facebook-github-bot · 2024-04-01T18:30:36Z

This pull request was exported from Phabricator. Differential Revision: D55351848

…kincubator#9257) Summary: When Window operator is initialized, WindowBuild reorders the input columns to place partition keys and sorting keys first before adding input rows to RowContainer. However, for window operations that use kRange frame with column boundary, e.g., range between c1 preceding and current row, the original column index of the frame column before reordering is passed to computeKRangeFrameBounds(), which leads to incorrect result. This diff fixes this bug by passing the actual index of the frame column after the reordering to computeKRangeFrameBounds(). This diff fixes facebookincubator#9229. Differential Revision: D55351848

facebook-github-bot · 2024-04-01T18:33:06Z

This pull request was exported from Phabricator. Differential Revision: D55351848

…kincubator#9257) Summary: When Window operator is initialized, WindowBuild reorders the input columns to place partition keys and sorting keys first before adding input rows to RowContainer. However, for window operations that use kRange frame with column boundary, e.g., range between c1 preceding and current row, the original column index of the frame column before reordering is passed to computeKRangeFrameBounds(), which leads to incorrect result. This diff fixes this bug by passing the actual index of the frame column after the reordering to computeKRangeFrameBounds(). This diff fixes facebookincubator#9229. Differential Revision: D55351848

facebook-github-bot · 2024-04-01T18:48:10Z

This pull request was exported from Phabricator. Differential Revision: D55351848

…kincubator#9257) Summary: When Window operator is initialized, WindowBuild reorders the input columns to place partition keys and sorting keys first before adding input rows to RowContainer. However, for window operations that use kRange frame with column boundary, e.g., range between c1 preceding and current row, the original column index of the frame column before reordering is passed to computeKRangeFrameBounds(), which leads to incorrect result. This diff fixes this bug by passing the actual index of the frame column after the reordering to computeKRangeFrameBounds(). This diff fixes facebookincubator#9229. Differential Revision: D55351848

facebook-github-bot · 2024-04-01T18:48:41Z

This pull request was exported from Phabricator. Differential Revision: D55351848

kagamiori · 2024-04-01T18:49:36Z

Hi @aditi-pandit, I've updated the PR to address your comments. Please take a look again. Thanks!

aditi-pandit

Thanks @kagamiori

…kincubator#9257) Summary: When Window operator is initialized, WindowBuild reorders the input columns to place partition keys and sorting keys first before adding input rows to RowContainer. However, for window operations that use kRange frame with column boundary, e.g., range between c1 preceding and current row, the original column index of the frame column before reordering is passed to computeKRangeFrameBounds(), which leads to incorrect result. This diff fixes this bug by passing the actual index of the frame column after the reordering to computeKRangeFrameBounds(). This diff fixes facebookincubator#9229. Differential Revision: D55351848

facebook-github-bot · 2024-04-01T19:28:07Z

This pull request was exported from Phabricator. Differential Revision: D55351848

…kincubator#9257) Summary: When Window operator is initialized, WindowBuild reorders the input columns to place partition keys and sorting keys first before adding input rows to RowContainer. However, for window operations that use kRange frame with column boundary, e.g., range between c1 preceding and current row, the original column index of the frame column before reordering is passed to computeKRangeFrameBounds(), which leads to incorrect result. This diff fixes this bug by passing the actual index of the frame column after the reordering to computeKRangeFrameBounds(). This diff fixes facebookincubator#9229. Differential Revision: D55351848

facebook-github-bot · 2024-04-01T19:29:53Z

This pull request was exported from Phabricator. Differential Revision: D55351848

facebook-github-bot · 2024-04-02T17:07:56Z

This pull request has been merged in 0fdf608.

conbench-facebook · 2024-04-02T17:55:42Z

Conbench analyzed the 1 benchmark run on commit 0fdf6082.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…kincubator#9257) Summary: Pull Request resolved: facebookincubator#9257 When Window operator is initialized, WindowBuild reorders the input columns to place partition keys and sorting keys first before adding input rows to RowContainer. However, for window operations that use kRange frame with column boundary, e.g., range between c1 preceding and current row, the original column index of the frame column before reordering is passed to computeKRangeFrameBounds(), which leads to incorrect result. This diff fixes this bug by passing the actual index of the frame column after the reordering to computeKRangeFrameBounds(). This diff fixes facebookincubator#9229. Reviewed By: kgpai Differential Revision: D55351848 fbshipit-source-id: c5512700327becf44aee8b29e798455ed184ac87

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 Mar 26, 2024

facebook-github-bot added the fb-exported label Mar 26, 2024

kagamiori force-pushed the export-D55351848 branch from 17e2754 to 9c8a27f Compare March 26, 2024 16:54

kagamiori requested a review from aditi-pandit March 26, 2024 16:54

aditi-pandit reviewed Mar 26, 2024

View reviewed changes

kagamiori force-pushed the export-D55351848 branch from 9c8a27f to a44be03 Compare March 30, 2024 17:47

aditi-pandit reviewed Mar 31, 2024

View reviewed changes

aditi-pandit reviewed Apr 1, 2024

View reviewed changes

kagamiori force-pushed the export-D55351848 branch from a44be03 to 4348a91 Compare April 1, 2024 18:30

kagamiori force-pushed the export-D55351848 branch from 4348a91 to b9228df Compare April 1, 2024 18:32

kagamiori requested a review from aditi-pandit April 1, 2024 18:41

kagamiori force-pushed the export-D55351848 branch from b9228df to 24a9fe0 Compare April 1, 2024 18:48

kagamiori force-pushed the export-D55351848 branch from 24a9fe0 to 180d9d9 Compare April 1, 2024 18:48

aditi-pandit approved these changes Apr 1, 2024

View reviewed changes

kagamiori force-pushed the export-D55351848 branch from 180d9d9 to 5a279a4 Compare April 1, 2024 19:28

kagamiori force-pushed the export-D55351848 branch from 5a279a4 to f2e70fb Compare April 1, 2024 19:29

facebook-github-bot closed this in 0fdf608 Apr 2, 2024

facebook-github-bot added the Merged label Apr 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix window operation with kRange frames with column boundary #9257

Fix window operation with kRange frames with column boundary #9257

kagamiori commented Mar 26, 2024

facebook-github-bot commented Mar 26, 2024

netlify bot commented Mar 26, 2024 •

edited

facebook-github-bot commented Mar 26, 2024

kagamiori commented Mar 26, 2024

aditi-pandit commented Mar 26, 2024

aditi-pandit commented Mar 26, 2024 •

edited

aditi-pandit Mar 26, 2024

aditi-pandit Mar 26, 2024

facebook-github-bot commented Mar 30, 2024

kagamiori commented Mar 31, 2024

aditi-pandit left a comment

aditi-pandit Mar 31, 2024

aditi-pandit Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

kagamiori commented Apr 1, 2024

aditi-pandit left a comment

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 2, 2024

conbench-facebook bot commented Apr 2, 2024

Fix window operation with kRange frames with column boundary #9257

Fix window operation with kRange frames with column boundary #9257

Conversation

kagamiori commented Mar 26, 2024

facebook-github-bot commented Mar 26, 2024

netlify bot commented Mar 26, 2024 • edited

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Mar 26, 2024

kagamiori commented Mar 26, 2024

aditi-pandit commented Mar 26, 2024

aditi-pandit commented Mar 26, 2024 • edited

aditi-pandit Mar 26, 2024

Choose a reason for hiding this comment

aditi-pandit Mar 26, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Mar 30, 2024

kagamiori commented Mar 31, 2024

aditi-pandit left a comment

Choose a reason for hiding this comment

aditi-pandit Mar 31, 2024

Choose a reason for hiding this comment

aditi-pandit Apr 1, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

kagamiori commented Apr 1, 2024

aditi-pandit left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 2, 2024

conbench-facebook bot commented Apr 2, 2024

netlify bot commented Mar 26, 2024 •

edited

aditi-pandit commented Mar 26, 2024 •

edited