Fix wrapping of 0-length vector in element_at and subscript functions #3284
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.
|
|
This pull request was exported from Phabricator. Differential Revision: D41357753 |
kagamiori
added a commit
to kagamiori/velox
that referenced
this pull request
Nov 21, 2022
…facebookincubator#3284) Summary: Pull Request resolved: facebookincubator#3284 Expression fuzzer found a bug in element_at() and subscript() functions when the input vector has a 0-length element vector and there are unselected rows of this evaluation: facebookincubator#3216. In this situation, SubscriptImpl builds up an `indices` where unselected rows point to 0 (i.e., default value) and use it to wrap the 0-length element vector via BaseVector::wrapInDictionary. DictionaryVector<T>::setInternalState() checks that the index pointed to by `indices` is less than the length of the vector being wrapped, which are both 0 in this case. Hence the error happens. This diff fixes this bug by directly creating a null vector when the input element vector has 0 length, instead of wrapping it in a dictionary. Differential Revision: D41357753 fbshipit-source-id: e7c8d11b0cf84d7b3a72ccc79ec4234908a6b2ce
e60f1d6 to
7b86c51
Compare
|
This pull request was exported from Phabricator. Differential Revision: D41357753 |
…facebookincubator#3284) Summary: Pull Request resolved: facebookincubator#3284 Expression fuzzer found a bug in element_at() and subscript() functions when the input vector has a 0-length element vector and there are unselected rows of this evaluation: facebookincubator#3216. In this situation, SubscriptImpl builds up an `indices` where unselected rows point to 0 (i.e., default value) and use it to wrap the 0-length element vector via BaseVector::wrapInDictionary. DictionaryVector<T>::setInternalState() checks that the index pointed to by `indices` is less than the length of the vector being wrapped, which are both 0 in this case. Hence the error happens. This diff fixes this bug by directly creating a null vector when the input element vector has 0 length, instead of wrapping it in a dictionary. Reviewed By: bikramSingh91 Differential Revision: D41357753 fbshipit-source-id: aa84b67dfbcace5a12e97fc61163e92ec7f21663
7b86c51 to
7e42770
Compare
|
This pull request was exported from Phabricator. Differential Revision: D41357753 |
kevinwilfong
pushed a commit
to kevinwilfong/velox
that referenced
this pull request
Dec 14, 2022
Summary: facebookincubator#3284 changed the behavior of subscript operators so if an array is empty any subscript access (regardless of value) returns NULL. The way it was written, this only applies if all the arrays in a batch are empty. This change makes it so it applies consistently if only a subset of arrays are empty. This inconsistency in behavior was identified in facebookincubator#3490 Differential Revision: D42021583 fbshipit-source-id: b94746aeb4748249b11ffd0b1fffbf5ce4c227b7
facebook-github-bot
pushed a commit
that referenced
this pull request
Dec 14, 2022
Summary: Pull Request resolved: #3509 #3284 changed the behavior of subscript operators so if an array is empty any subscript access (regardless of value) returns NULL. The way it was written, this only applies if all the arrays in a batch are empty. This change makes it so it applies consistently if only a subset of arrays are empty. This inconsistency in behavior was identified in #3490 Reviewed By: mbasmanova Differential Revision: D42021583 fbshipit-source-id: e49e6a2bd8714a4e617b7e201b1ce2722551077f
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:
Expression fuzzer found a bug in element_at() and subscript() functions when
the input vector has a 0-length element vector and there are unselected rows
of this evaluation: #3216. In this situation, SubscriptImpl builds up an
indiceswhere unselected rows point to 0 (i.e., default value) and use it to wrap the 0-
length element vector via BaseVector::wrapInDictionary. DictionaryVector::setInternalState()
checks that the index pointed to by
indicesis less than the length of thevector being wrapped, which are both 0 in this case. Hence the error happens.
This diff fixes this bug by directly creating a null vector when the input element
vector has 0 length, instead of wrapping it in a dictionary.
Differential Revision: D41357753