Test side-effect-free of Aggregate::extractXxx #8399
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for meta-velox canceled.
|
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D52755566 |
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 17, 2024
Summary: Add a test in testAggregations to check that Aggregate::extractAccumulators and Aggregate::extractValues don't have side effects of changing the accumulators. Differential Revision: D52755566
kagamiori
force-pushed
the
export-D52755566
branch
from
fb36050
to
28a8035
Compare
|
This pull request was exported from Phabricator. Differential Revision: D52755566 |
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 17, 2024
Summary: Add a test in testAggregations to check that Aggregate::extractAccumulators and Aggregate::extractValues don't have side effects of changing the accumulators. Differential Revision: D52755566
kagamiori
force-pushed
the
export-D52755566
branch
from
28a8035
to
6577c67
Compare
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 17, 2024
Summary: Add a test in testAggregations to check that Aggregate::extractAccumulators and Aggregate::extractValues don't have side effects of changing the accumulators. Differential Revision: D52755566
|
This pull request was exported from Phabricator. Differential Revision: D52755566 |
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 17, 2024
Summary: Add a test in testAggregations to check that Aggregate::extractAccumulators and Aggregate::extractValues don't have side effects of changing the accumulators. Differential Revision: D52755566
mbasmanova
approved these changes
Jan 17, 2024
There was a problem hiding this comment.
@kagamiori Thanks. It would be nice to mention that min/max/min_by/max_by are currently having side effects and therefore excluded from testing.
velox/exec/Aggregate.h
Outdated
| @@ -208,6 +208,8 @@ class Aggregate { | |||
| // 'result' and its parts are expected to be singly referenced. If | |||
| // other threads or operators hold references that they would use | |||
| // after 'result' has been updated by this, effects will be unpredictable. | |||
| // This method should be side-effect free, i.e., calling this method | |||
There was a problem hiding this comment.
nit: should be side-effect free -> should not have side effects.
velox/exec/Aggregate.h
Outdated
| @@ -208,6 +208,8 @@ class Aggregate { | |||
| // 'result' and its parts are expected to be singly referenced. If | |||
| // other threads or operators hold references that they would use | |||
| // after 'result' has been updated by this, effects will be unpredictable. | |||
| // This method should be side-effect free, i.e., calling this method | |||
| // doesn't change the content of the accumnulators. | |||
There was a problem hiding this comment.
accumnulators -> accumulators
It would be nice to explain why we have this requirement.
| auto constant = core::TypedExprs::asConstant(arg); | ||
| columns.push_back(constant->toConstantVector(pool)); | ||
| } | ||
| if (auto lambda = dynamic_cast<const core::LambdaTypedExpr*>(arg.get())) { |
There was a problem hiding this comment.
Do we not have core::TypedExprs::asLambda? Let's add.
| @@ -265,6 +272,15 @@ class AggregationTestBase : public exec::test::OperatorTestBase { | |||
| vector_size_t rawInput2Size, | |||
| const std::unordered_map<std::string, std::string>& config = {}); | |||
|
|
|||
| // Test to ensure that when extractValues() or extractAccumulators() is called | |||
| // twice on the same accumulator, the extracted results are the same. This | |||
| // ensure that extractValues() and extractAccumulators() are free of side | |||
| func->extractValues(groups.data(), 1, &result2); | ||
| velox::test::assertEqualVectors(result1, result2); | ||
|
|
||
| // Destroy accumulators to avoid memory leak. |
There was a problem hiding this comment.
Do we need to do this if there is an exception?
kagamiori
force-pushed
the
export-D52755566
branch
from
6577c67
to
33aa0ed
Compare
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 19, 2024
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, so their tests are excluded from the added testing. Reviewed By: mbasmanova Differential Revision: D52755566
|
This pull request was exported from Phabricator. Differential Revision: D52755566 |
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 19, 2024
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, so their tests are excluded from the added testing. Reviewed By: mbasmanova Differential Revision: D52755566
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 19, 2024
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, so their tests are excluded from the added testing. Reviewed By: mbasmanova Differential Revision: D52755566
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 19, 2024
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, so their tests are excluded from the added testing. Reviewed By: mbasmanova Differential Revision: D52755566
kagamiori
force-pushed
the
export-D52755566
branch
from
33aa0ed
to
8414713
Compare
|
This pull request was exported from Phabricator. Differential Revision: D52755566 |
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 19, 2024
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, so their tests are excluded from the added testing. Reviewed By: mbasmanova Differential Revision: D52755566
kagamiori
force-pushed
the
export-D52755566
branch
from
8414713
to
a6590c1
Compare
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 19, 2024
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, so their tests are excluded from the added testing. Reviewed By: mbasmanova Differential Revision: D52755566
|
This pull request was exported from Phabricator. Differential Revision: D52755566 |
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 19, 2024
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, so their tests are excluded from the added testing. Reviewed By: mbasmanova Differential Revision: D52755566
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 19, 2024
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, so their tests are excluded from the added testing. Reviewed By: mbasmanova Differential Revision: D52755566
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 19, 2024
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
kagamiori
force-pushed
the
export-D52755566
branch
from
a6590c1
to
26f805e
Compare
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 19, 2024
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
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 22, 2024
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
kagamiori
force-pushed
the
export-D52755566
branch
from
26f805e
to
4616e3b
Compare
|
This pull request was exported from Phabricator. Differential Revision: D52755566 |
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 22, 2024
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
kagamiori
force-pushed
the
export-D52755566
branch
from
4616e3b
to
ae3b332
Compare
|
This pull request was exported from Phabricator. Differential Revision: D52755566 |
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 22, 2024
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
kagamiori
force-pushed
the
export-D52755566
branch
from
ae3b332
to
843026b
Compare
|
This pull request was exported from Phabricator. Differential Revision: D52755566 |
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
kagamiori
force-pushed
the
export-D52755566
branch
from
843026b
to
8b5b401
Compare
|
This pull request was exported from Phabricator. Differential Revision: D52755566 |
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 22, 2024
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
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Jan 22, 2024
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
|
This pull request has been merged in ce18fe4. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Add a test in testAggregations to check that Aggregate::extractAccumulators and
Aggregate::extractValues don't have side effects of changing the accumulators.
Differential Revision: D52755566