Fix min/max(x, n) #8311

kagamiori · 2024-01-09T22:04:33Z

Summary:
Velox has an optimization for Window operation with incremental aggregation when there are peer
rows with the same frame. In this situation, the aggregation result is only computed at the first row
of the peer and the rest rows simply copy this result. This optimization assumes that results of
incremental aggregation in Window operation on peer rows should be the same. However, min/max(x, n)
in Velox breaks this assumption because their extractValues() method causes the accumulator to
be cleared, making the peer rows after the first row expect a different result. This diff fixes min/max(x, n)
to make the extraction method not clear the accumulator. The behavior after the fix also align with
Presto's.

This diff fixes #8138.

Differential Revision: D52638334

netlify · 2024-01-09T22:04:39Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`3e560d6`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65aea4a0b7780a000865116a

facebook-github-bot · 2024-01-09T22:04:43Z

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

Summary: In Presto, accumulators of min/max(x, n) do not clear the heap when values are extracted from accumulator. But in Velox they do. Fix this bug to make Velox behavior align with Presto. This diff fixes facebookincubator#8138. Differential Revision: D52638334

Summary: Velox has an optimization for Window operation with incremental aggregation when there are peer rows with the same frame. In this situation, the aggregation result is only computed at the first row of the peer and the rest rows simply copy this result. This optimization assumes that results of incremental aggregation in Window operation on peer rows should be the same. However, min/max(x, n) in Velox breaks this assumption because their extractValues() method causes the accumulator to be cleared, making the peer rows after the first row expect a different result. This diff fixes min/max(x, n) to make the extraction method not clear the accumulator. The behavior after the fix also align with Presto's. This diff also adds a method testIncrementalAggregation in testAggregations to check that extractValues() doesn't change accumulator afterwards for all aggregation functions. After this fix, only min_by/max_by(x, y, n) doesn't pass testIncrementalAggregation. This diff fixes facebookincubator#8138. Differential Revision: D52638334

facebook-github-bot · 2024-01-11T18:10:26Z

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

mbasmanova · 2024-01-12T00:42:00Z

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

+          .window({"max(c0, c1) over (partition by c2 order by c3 asc)"})
+          .planNode();
+
+  auto result = AssertQueryBuilder(plan).copyResults(pool());


copyResults -> assertResults(expected)

mbasmanova · 2024-01-12T00:46:11Z

velox/functions/lib/aggregates/tests/utils/AggregationTestBase.cpp

+    if (auto field =
+            dynamic_cast<const core::FieldAccessTypedExpr*>(arg.get())) {
+      auto channel = type.getChildIdx(field->name());
+      columns.push_back(input->childAt(channel));


input->childAt(field->name());

/// Returns child vector for the specified field name. Throws if field with /// specified name doesn't exist. VectorPtr& childAt(const std::string& name) { return children_[type_->asRow().getChildIdx(name)]; }

mbasmanova · 2024-01-12T00:46:17Z

velox/functions/lib/aggregates/tests/utils/AggregationTestBase.cpp

+    const core::CallTypedExprPtr& aggregateExpr,
+    const RowVectorPtr& input,
+    memory::MemoryPool* pool) {
+  auto type = input->type()->as<TypeKind::ROW>();


mbasmanova · 2024-01-12T00:47:28Z

velox/functions/lib/aggregates/tests/utils/AggregationTestBase.cpp

+  std::vector<VectorPtr> columns;
+  for (const auto& arg : aggregateExpr->inputs()) {
+    if (auto field =
+            dynamic_cast<const core::FieldAccessTypedExpr*>(arg.get())) {


TypedExprs::asFieldAccess(arg)

mbasmanova · 2024-01-12T00:47:47Z

velox/functions/lib/aggregates/tests/utils/AggregationTestBase.cpp

+      auto channel = type.getChildIdx(field->name());
+      columns.push_back(input->childAt(channel));
+    }
+    if (dynamic_cast<const core::ConstantTypedExpr*>(arg.get())) {


isConstant(arg)

Summary: Velox has an optimization for Window operation with incremental aggregation when there are peer rows with the same frame. In this situation, the aggregation result is only computed at the first row of the peer and the rest rows simply copy this result. This optimization assumes that results of incremental aggregation in Window operation on peer rows should be the same. However, min/max(x, n) in Velox breaks this assumption because their extractValues() method causes the accumulator to be cleared, making the peer rows after the first row expect a different result. This diff fixes min/max(x, n) to make the extraction method not clear the accumulator. The behavior after the fix also align with Presto's. This diff also adds a method testIncrementalAggregation in testAggregations to check that extractValues() doesn't change accumulator afterwards for all aggregation functions. After this fix, only min_by/max_by(x, y, n) doesn't pass testIncrementalAggregation. This diff fixes facebookincubator#8138. Differential Revision: D52638334

facebook-github-bot · 2024-01-13T01:56:36Z

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

Summary: Velox has an optimization for Window operation with incremental aggregation when there are peer rows with the same frame. In this situation, the aggregation result is only computed at the first row of the peer and the rest rows simply copy this result. This optimization assumes that results of incremental aggregation in Window operation on peer rows should be the same. However, min/max(x, n) in Velox breaks this assumption because their extractValues() method causes the accumulator to be cleared, making the peer rows after the first row expect a different result. This diff fixes min/max(x, n) to make the extraction method not clear the accumulator. The behavior after the fix also align with Presto's. This diff also adds a method testIncrementalAggregation in testAggregations to check that extractValues() doesn't change accumulator afterwards for all aggregation functions. After this fix, only min_by/max_by(x, y, n) doesn't pass testIncrementalAggregation. This diff fixes facebookincubator#8138. Differential Revision: D52638334

facebook-github-bot · 2024-01-17T01:25:18Z

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

Summary: Velox has an optimization for Window operation with incremental aggregation when there are peer rows with the same frame. In this situation, the aggregation result is only computed at the first row of the peer and the rest rows simply copy this result. This optimization assumes that results of incremental aggregation in Window operation on peer rows should be the same. However, min/max(x, n) in Velox breaks this assumption because their extractValues() method causes the accumulator to be cleared, making the peer rows after the first row expect a different result. This diff fixes min/max(x, n) to make the extraction method not clear the accumulator. The behavior after the fix also align with Presto's. This diff also adds a method testIncrementalAggregation in testAggregations to check that extractValues() doesn't change accumulator afterwards for all aggregation functions. After this fix, only min_by/max_by(x, y, n) doesn't pass testIncrementalAggregation. This diff fixes facebookincubator#8138. Differential Revision: D52638334

facebook-github-bot · 2024-01-17T01:25:44Z

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

Summary: Velox has an optimization for Window operation with incremental aggregation when there are peer rows with the same frame. In this situation, the aggregation result is only computed at the first row of the peer and the rest rows simply copy this result. This optimization assumes that results of incremental aggregation in Window operation on peer rows should be the same. However, min/max(x, n) in Velox breaks this assumption because their extractValues() method causes the accumulator to be cleared, making the peer rows after the first row expect a different result. This diff fixes min/max(x, n) to make the extraction method not clear the accumulator. The behavior after the fix also align with Presto's. This diff also adds a method testIncrementalAggregation in testAggregations to check that extractValues() doesn't change accumulator afterwards for all aggregation functions. After this fix, only min_by/max_by(x, y, n) doesn't pass testIncrementalAggregation. This diff fixes facebookincubator#8138. Differential Revision: D52638334

facebook-github-bot · 2024-01-19T01:04:34Z

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

facebook-github-bot · 2024-01-19T01:04:52Z

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

facebook-github-bot · 2024-01-19T01:18:54Z

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

Summary: Velox has an optimization for Window operation with incremental aggregation when there are peer rows with the same frame. In this situation, the aggregation result is only computed at the first row of the peer and the rest rows simply copy this result. This optimization assumes that results of incremental aggregation in Window operation on peer rows should be the same. However, min/max(x, n) in Velox breaks this assumption because their extractValues() method causes the accumulator to be cleared, making the peer rows after the first row expect a different result. This diff fixes min/max(x, n) to make the extraction method not clear the accumulator. The behavior after the fix also align with Presto's. This diff also adds a method testIncrementalAggregation in testAggregations to check that extractValues() doesn't change accumulator afterwards for all aggregation functions. After this fix, only min_by/max_by(x, y, n) doesn't pass testIncrementalAggregation. This diff fixes facebookincubator#8138. Reviewed By: mbasmanova Differential Revision: D52638334

facebook-github-bot · 2024-01-19T01:19:13Z

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

Summary: Velox has an optimization for Window operation with incremental aggregation when there are peer rows with the same frame. In this situation, the aggregation result is only computed at the first row of the peer and the rest rows simply copy this result. This optimization assumes that results of incremental aggregation in Window operation on peer rows should be the same. However, min/max(x, n) in Velox breaks this assumption because their extractValues() method causes the accumulator to be cleared, making the peer rows after the first row expect a different result. This diff fixes min/max(x, n) to make the extraction method not clear the accumulator. The behavior after the fix also align with Presto's. This diff also adds a method testIncrementalAggregation in testAggregations to check that extractValues() doesn't change accumulator afterwards for all aggregation functions. After this fix, only min_by/max_by(x, y, n) doesn't pass testIncrementalAggregation. This diff fixes facebookincubator#8138. Reviewed By: mbasmanova Differential Revision: D52638334

facebook-github-bot · 2024-01-19T23:46:37Z

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

Summary: Velox has an optimization for Window operation with incremental aggregation when there are peer rows with the same frame. In this situation, the aggregation result is only computed at the first row of the peer and the rest rows simply copy this result. This optimization assumes that results of incremental aggregation in Window operation on peer rows should be the same. However, min/max(x, n) in Velox breaks this assumption because their extractValues() method causes the accumulator to be cleared, making the peer rows after the first row expect a different result. This diff fixes min/max(x, n) to make the extraction method not clear the accumulator. The behavior after the fix also align with Presto's. This diff also adds a method testIncrementalAggregation in testAggregations to check that extractValues() doesn't change accumulator afterwards for all aggregation functions. After this fix, only min_by/max_by(x, y, n) doesn't pass testIncrementalAggregation. This diff fixes facebookincubator#8138. Reviewed By: mbasmanova Differential Revision: D52638334

facebook-github-bot · 2024-01-19T23:48:06Z

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

Summary: Add a test in testAggregations to check that Aggregate::extractAccumulators and Aggregate::extractValues don't have side effects of changing the accumulators. min, max, min_by, and max_by currently have side effects. avg and sum for decimal inputs have undefined values in intermediate results. So they are excluded from the added testing. Reviewed By: mbasmanova Differential Revision: D52755566

Summary: Velox has an optimization for Window operation with incremental aggregation when there are peer rows with the same frame. In this situation, the aggregation result is only computed at the first row of the peer and the rest rows simply copy this result. This optimization assumes that results of incremental aggregation in Window operation on peer rows should be the same. However, min/max(x, n) in Velox breaks this assumption because their extractValues() method causes the accumulator to be cleared, making the peer rows after the first row expect a different result. This diff fixes min/max(x, n) to make the extraction method not clear the accumulator. The behavior after the fix also align with Presto's. This diff also adds a method testIncrementalAggregation in testAggregations to check that extractValues() doesn't change accumulator afterwards for all aggregation functions. After this fix, only min_by/max_by(x, y, n) doesn't pass testIncrementalAggregation. This diff fixes facebookincubator#8138. Reviewed By: mbasmanova Differential Revision: D52638334

facebook-github-bot · 2024-01-22T17:23:36Z

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

facebook-github-bot · 2024-01-22T17:23:55Z

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

facebook-github-bot · 2024-01-22T21:02:02Z

This pull request has been merged in d747ac3.

conbench-facebook · 2024-01-22T21:39:25Z

Conbench analyzed the 1 benchmark run on commit d747ac33.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: Same as bug in min/max(x, n) fixed in facebookincubator#8311, min_by/max_by(x, y, n) also breaks the assumption of incremental window aggregation because their extractValues() methods has a side effect of clearing the accumulator. This diff fixes this issue by making the extractValues() methods of min_by/max_by(x, y, n) not clear the accumulators. Since Presto's min_by/max_by have the same bug (prestodb/presto#21653). This fix will make Velox's min_by/max_by behave differently from Presto when used in Window operation, until prestodb/presto#21653 is fixed. Differential Revision: D53139892

Summary: Same as bug in min/max(x, n) fixed in facebookincubator#8311, min_by/max_by(x, y, n) also breaks the assumption of incremental window aggregation because their extractValues() methods has a side effect of clearing the accumulator. This diff fixes this issue by making the extractValues() methods of min_by/max_by(x, y, n) not clear the accumulators. Since Presto's min_by/max_by have the same bug (prestodb/presto#21653). This fix will make Velox's min_by/max_by behave differently from Presto when used in Window operation, until prestodb/presto#21653 is fixed. This diff fixes facebookincubator#8138. Differential Revision: D53139892

Summary: Pull Request resolved: #8566 Same as bug in min/max(x, n) fixed in #8311, min_by/max_by(x, y, n) also breaks the assumption of incremental window aggregation because their extractValues() methods has a side effect of clearing the accumulator. This diff fixes this issue by making the extractValues() methods of min_by/max_by(x, y, n) not clear the accumulators. Since Presto's min_by/max_by have the same bug (prestodb/presto#21653). This fix will make Velox's min_by/max_by behave differently from Presto when used in Window operation, until prestodb/presto#21653 is fixed. This diff fixes #8138. Reviewed By: bikramSingh91 Differential Revision: D53139892 fbshipit-source-id: 1323f22196e22554c0d880d20584a4ee4059b64c

Summary: Pull Request resolved: facebookincubator#8566 Same as bug in min/max(x, n) fixed in facebookincubator#8311, min_by/max_by(x, y, n) also breaks the assumption of incremental window aggregation because their extractValues() methods has a side effect of clearing the accumulator. This diff fixes this issue by making the extractValues() methods of min_by/max_by(x, y, n) not clear the accumulators. Since Presto's min_by/max_by have the same bug (prestodb/presto#21653). This fix will make Velox's min_by/max_by behave differently from Presto when used in Window operation, until prestodb/presto#21653 is fixed. This diff fixes facebookincubator#8138. Reviewed By: bikramSingh91 Differential Revision: D53139892 fbshipit-source-id: 1323f22196e22554c0d880d20584a4ee4059b64c

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 Jan 9, 2024

facebook-github-bot added the fb-exported label Jan 9, 2024

kagamiori force-pushed the export-D52638334 branch from 2114635 to bf002e6 Compare January 11, 2024 18:10

mbasmanova reviewed Jan 12, 2024

View reviewed changes

kagamiori force-pushed the export-D52638334 branch from bf002e6 to ba4bd00 Compare January 13, 2024 01:56

kagamiori force-pushed the export-D52638334 branch from ba4bd00 to 170a048 Compare January 17, 2024 01:25

kagamiori force-pushed the export-D52638334 branch from 170a048 to 6940296 Compare January 17, 2024 01:25

kagamiori requested a review from mbasmanova January 17, 2024 01:26

kagamiori force-pushed the export-D52638334 branch from 6940296 to 42ac073 Compare January 19, 2024 01:04

kagamiori force-pushed the export-D52638334 branch from 42ac073 to 191b1b1 Compare January 19, 2024 01:04

kagamiori force-pushed the export-D52638334 branch from b76d946 to c19795c Compare January 19, 2024 01:19

kagamiori force-pushed the export-D52638334 branch from c19795c to 1b9c66c Compare January 19, 2024 23:46

kagamiori force-pushed the export-D52638334 branch from 1b9c66c to a708dc4 Compare January 19, 2024 23:47

kagamiori added 2 commits January 22, 2024 09:23

kagamiori force-pushed the export-D52638334 branch from a708dc4 to 48f2bf1 Compare January 22, 2024 17:23

kagamiori force-pushed the export-D52638334 branch from 48f2bf1 to 3e560d6 Compare January 22, 2024 17:23

facebook-github-bot closed this in d747ac3 Jan 22, 2024

facebook-github-bot added the Merged label Jan 22, 2024

kagamiori mentioned this pull request Jan 26, 2024

Fix min_by/max_by(x, y, n) #8566

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix min/max(x, n) #8311

Fix min/max(x, n) #8311

kagamiori commented Jan 9, 2024 •

edited

netlify bot commented Jan 9, 2024 •

edited

facebook-github-bot commented Jan 9, 2024

facebook-github-bot commented Jan 11, 2024

mbasmanova Jan 12, 2024

mbasmanova Jan 12, 2024

mbasmanova Jan 12, 2024

mbasmanova Jan 12, 2024

mbasmanova Jan 12, 2024

facebook-github-bot commented Jan 13, 2024

facebook-github-bot commented Jan 17, 2024

facebook-github-bot commented Jan 17, 2024

facebook-github-bot commented Jan 19, 2024

facebook-github-bot commented Jan 19, 2024

facebook-github-bot commented Jan 19, 2024

facebook-github-bot commented Jan 19, 2024

facebook-github-bot commented Jan 19, 2024

facebook-github-bot commented Jan 19, 2024

facebook-github-bot commented Jan 22, 2024

facebook-github-bot commented Jan 22, 2024

facebook-github-bot commented Jan 22, 2024

conbench-facebook bot commented Jan 22, 2024

Fix min/max(x, n) #8311

Fix min/max(x, n) #8311

Conversation

kagamiori commented Jan 9, 2024 • edited

netlify bot commented Jan 9, 2024 • edited

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Jan 9, 2024

facebook-github-bot commented Jan 11, 2024

mbasmanova Jan 12, 2024

Choose a reason for hiding this comment

mbasmanova Jan 12, 2024

Choose a reason for hiding this comment

mbasmanova Jan 12, 2024

Choose a reason for hiding this comment

mbasmanova Jan 12, 2024

Choose a reason for hiding this comment

mbasmanova Jan 12, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Jan 13, 2024

facebook-github-bot commented Jan 17, 2024

facebook-github-bot commented Jan 17, 2024

facebook-github-bot commented Jan 19, 2024

facebook-github-bot commented Jan 19, 2024

facebook-github-bot commented Jan 19, 2024

facebook-github-bot commented Jan 19, 2024

facebook-github-bot commented Jan 19, 2024

facebook-github-bot commented Jan 19, 2024

facebook-github-bot commented Jan 22, 2024

facebook-github-bot commented Jan 22, 2024

facebook-github-bot commented Jan 22, 2024

conbench-facebook bot commented Jan 22, 2024

kagamiori commented Jan 9, 2024 •

edited

netlify bot commented Jan 9, 2024 •

edited