Fix bug in initializing global aggregations with initialized flag #9396
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
|
This pull request was exported from Phabricator. Differential Revision: D55823098 |
facebook-github-bot
added
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Apr 5, 2024
…cebookincubator#9396) Summary: facebookincubator#9067 introduced an initialized flag that we include alongside the null flag for aggregations in the rows in RowContainer. Global aggregations do not use a RowContainer to create rows, rather a single row is constructed manually inside GroupingSets. When that PR was landed this logic was not properly updated, we did not allocate enough space in the row for the additional bits which could lead to null/initialized flags overwriting the aggregate values when there were a lot of them. This change fixes it so that we allocate the correct amount of space for flags for global aggregations as an immediate fix. Longer term it would be better to centralize the logic for creating rows in RowContainer (I'd like to get us back to a working state before attempting such a refactor). Differential Revision: D55823098
kevinwilfong
force-pushed
the
export-D55823098
branch
from
fef41b5
to
cd6970e
Compare
|
This pull request was exported from Phabricator. Differential Revision: D55823098 |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Apr 5, 2024
…cebookincubator#9396) Summary: facebookincubator#9067 introduced an initialized flag that we include alongside the null flag for aggregations in the rows in RowContainer. Global aggregations do not use a RowContainer to create rows, rather a single row is constructed manually inside GroupingSets. When that PR was landed this logic was not properly updated, we did not allocate enough space in the row for the additional bits which could lead to null/initialized flags overwriting the aggregate values when there were a lot of them. This change fixes it so that we allocate the correct amount of space for flags for global aggregations as an immediate fix. Longer term it would be better to centralize the logic for creating rows in RowContainer (I'd like to get us back to a working state before attempting such a refactor). Differential Revision: D55823098
kevinwilfong
force-pushed
the
export-D55823098
branch
from
cd6970e
to
967a981
Compare
|
This pull request was exported from Phabricator. Differential Revision: D55823098 |
…cebookincubator#9396) Summary: facebookincubator#9067 introduced an initialized flag that we include alongside the null flag for aggregations in the rows in RowContainer. Global aggregations do not use a RowContainer to create rows, rather a single row is constructed manually inside GroupingSets. When that PR was landed this logic was not properly updated, we did not allocate enough space in the row for the additional bits which could lead to null/initialized flags overwriting the aggregate values when there were a lot of them. This change fixes it so that we allocate the correct amount of space for flags for global aggregations as an immediate fix. Longer term it would be better to centralize the logic for creating rows in RowContainer (I'd like to get us back to a working state before attempting such a refactor). Differential Revision: D55823098
kevinwilfong
force-pushed
the
export-D55823098
branch
from
967a981
to
2f53976
Compare
|
This pull request was exported from Phabricator. Differential Revision: D55823098 |
Yuhta
approved these changes
Apr 5, 2024
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
|
This pull request has been merged in e0ef5a2. |
waitinfuture
reviewed
May 8, 2024
|
|
||
| // Allocate space for the null and initialized flags. | ||
| int32_t rowSizeOffset = | ||
| bits::nbytes(aggregates_.size() * RowContainer::kNumAccumulatorFlags); |
There was a problem hiding this comment.
Hi @kevinwilfong , seems the flags don't consider for sortedAggregations_ and distinctAggregations_? cc @Yuhta @mbasmanova
Joe-Abraham
pushed a commit
to Joe-Abraham/velox
that referenced
this pull request
Jun 7, 2024
…cebookincubator#9396) Summary: Pull Request resolved: facebookincubator#9396 facebookincubator#9067 introduced an initialized flag that we include alongside the null flag for aggregations in the rows in RowContainer. Global aggregations do not use a RowContainer to create rows, rather a single row is constructed manually inside GroupingSets. When that PR was landed this logic was not properly updated, we did not allocate enough space in the row for the additional bits which could lead to null/initialized flags overwriting the aggregate values when there were a lot of them. This change fixes it so that we allocate the correct amount of space for flags for global aggregations as an immediate fix. Longer term it would be better to centralize the logic for creating rows in RowContainer (I'd like to get us back to a working state before attempting such a refactor). Reviewed By: Yuhta Differential Revision: D55823098 fbshipit-source-id: 9a15f257669273c64dc0361bf735c4472e45957d
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:
#9067 introduced an initialized flag that we include alongside the null flag
for aggregations in the rows in RowContainer.
Global aggregations do not use a RowContainer to create rows, rather a single row is constructed manually inside
GroupingSets. When that PR was landed this logic was not properly updated, we did not allocate enough space in the row
for the additional bits which could lead to null/initialized flags overwriting the aggregate values when there were a lot of
them.
This change fixes it so that we allocate the correct amount of space for flags for global aggregations as an immediate fix.
Longer term it would be better to centralize the logic for creating rows in RowContainer (I'd like to get us back to a working
state before attempting such a refactor).
Differential Revision: D55823098