Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Array/Map/RowVector::copyRanges for UNKNOWN source #6607

Closed

Conversation

mbasmanova
Copy link
Contributor

@mbasmanova mbasmanova commented Sep 16, 2023

Fix crashes when copying from TypeKind::UNKNOWN vector in the following methods:

  • Map/Array/RowVector::copyRanges
  • FlatVector::copyRanges
  • FlatVector::copy(source, rows, toSourceRow)
  • RowVector::copy(source, rows, toSourceRow)

Fixes #6606

@netlify
Copy link

netlify bot commented Sep 16, 2023

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 415ad04
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/65059b7da8832b00085dc400

@facebook-github-bot 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 Sep 16, 2023
@facebook-github-bot
Copy link
Contributor

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

if (leafVector->isConstantEncoding()) {
// A null constant does not have a value vector, so wrappedVector
// returns the constant.
VELOX_CHECK(leafVector->isNullAt(0));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: VELOX_USER_CHECK?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is not a user error, but rather a bug inside Velox as it should not allow creating vectors that would trigger this check.

Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbasmanova LGTM. Thanks!

@facebook-github-bot
Copy link
Contributor

@mbasmanova merged this pull request in 63fa93a.

@conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit 63fa93ae.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

codyschierbeck pushed a commit to codyschierbeck/velox that referenced this pull request Sep 27, 2023
…ator#6607)

Summary:
Fix crashes when copying from TypeKind::UNKNOWN vector in the following methods:
- Map/Array/RowVector::copyRanges
- FlatVector<StringView>::copyRanges
- FlatVector<bool>::copy(source, rows, toSourceRow)
- RowVector::copy(source, rows, toSourceRow)

Fixes facebookincubator#6606

Pull Request resolved: facebookincubator#6607

Reviewed By: xiaoxmeng

Differential Revision: D49348224

Pulled By: mbasmanova

fbshipit-source-id: 57598428a36787ee3c57a1b7c9f6cffd11206bf6
codyschierbeck pushed a commit to codyschierbeck/velox that referenced this pull request Sep 27, 2023
…ator#6607)

Summary:
Fix crashes when copying from TypeKind::UNKNOWN vector in the following methods:
- Map/Array/RowVector::copyRanges
- FlatVector<StringView>::copyRanges
- FlatVector<bool>::copy(source, rows, toSourceRow)
- RowVector::copy(source, rows, toSourceRow)

Fixes facebookincubator#6606

Pull Request resolved: facebookincubator#6607

Reviewed By: xiaoxmeng

Differential Revision: D49348224

Pulled By: mbasmanova

fbshipit-source-id: 57598428a36787ee3c57a1b7c9f6cffd11206bf6
codyschierbeck pushed a commit to codyschierbeck/velox that referenced this pull request Sep 27, 2023
…ator#6607)

Summary:
Fix crashes when copying from TypeKind::UNKNOWN vector in the following methods:
- Map/Array/RowVector::copyRanges
- FlatVector<StringView>::copyRanges
- FlatVector<bool>::copy(source, rows, toSourceRow)
- RowVector::copy(source, rows, toSourceRow)

Fixes facebookincubator#6606

Pull Request resolved: facebookincubator#6607

Reviewed By: xiaoxmeng

Differential Revision: D49348224

Pulled By: mbasmanova

fbshipit-source-id: 57598428a36787ee3c57a1b7c9f6cffd11206bf6
ericyuliu pushed a commit to ericyuliu/velox that referenced this pull request Oct 12, 2023
…ator#6607)

Summary:
Fix crashes when copying from TypeKind::UNKNOWN vector in the following methods:
- Map/Array/RowVector::copyRanges
- FlatVector<StringView>::copyRanges
- FlatVector<bool>::copy(source, rows, toSourceRow)
- RowVector::copy(source, rows, toSourceRow)

Fixes facebookincubator#6606

Pull Request resolved: facebookincubator#6607

Reviewed By: xiaoxmeng

Differential Revision: D49348224

Pulled By: mbasmanova

fbshipit-source-id: 57598428a36787ee3c57a1b7c9f6cffd11206bf6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map/Array/RowVector::copyRanges crashes when source is an UNKNOWN vector
4 participants