Fix Expr::evalWithMemo #48
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
facebook-github-bot
added
the
CLA Signed
|
@mbasmanova has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Fix BaseVector::ensureWritable(*uncached,... &dictionaryCache_); call. dictionaryCache_ is valid only for some rows (cachedDictionaryIndices_), hence, a safe call to BaseVector::ensureWritable must include all the rows not covered by cachedDictionaryIndices_. If BaseVector::ensureWritable is called only for a subset of rows not covered by cachedDictionaryIndices_, it will attempt to copy rows that are not valid leading to a crash. Also, set EvalCtx::isFinalSelection to false when evaluating "uncached" rows to ensure that values in rows copies from the dictionaryCache_ don't get lost. These fixes are band-aids. They are workarounds for the fact that BaseVector::ensureWritable primitive is not safe. It takes a set of rows to make writable and copies all other rows. Since it has no way of knowing which of the other rows are uninitialized it may cause a crash. A proper fix could be to (1) introduce a way to specify that a particular row in a vector is uninitilized and use this information when copying the rows to skip uninitialized rows or (2) change BaseVector::ensureWritable to take a set of rows to preserve and change all the call sites accordingly.
mbasmanova
force-pushed
the
eval-with-memo-crash
branch
from
be480a4 to
3f7bcb8
Compare
|
@mbasmanova has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
kgpai
approved these changes
Aug 16, 2021
| @@ -618,6 +636,16 @@ class ExprTest : public testing::Test { | |||
| return vectorMaker_->arrayVector(size, sizeAt, valueAt, isNullAt); | |||
| } | |||
|
|
|||
| template <typename T> | |||
| ArrayVectorPtr makeArrayVector( | |||
There was a problem hiding this comment.
I am pretty sure I have seen an equivalent function in ExprTest.cpp..
|
@mbasmanova merged this pull request in f7601bf. |
rui-mo
added a commit
to rui-mo/velox
that referenced
this pull request
Sep 21, 2022
rui-mo
added a commit
to rui-mo/velox
that referenced
this pull request
Sep 26, 2022
rui-mo
added a commit
to rui-mo/velox
that referenced
this pull request
Oct 26, 2022
rui-mo
added a commit
to rui-mo/velox
that referenced
this pull request
Nov 8, 2022
rui-mo
added a commit
to rui-mo/velox
that referenced
this pull request
Nov 8, 2022
rui-mo
added a commit
to rui-mo/velox
that referenced
this pull request
Nov 22, 2022
rui-mo
added a commit
to rui-mo/velox
that referenced
this pull request
Dec 15, 2022
rui-mo
added a commit
to rui-mo/velox
that referenced
this pull request
Jan 6, 2023
rui-mo
added a commit
to rui-mo/velox
that referenced
this pull request
Jan 12, 2023
PHILO-HE
pushed a commit
to PHILO-HE/velox
that referenced
this pull request
Feb 3, 2023
rui-mo
added a commit
to rui-mo/velox
that referenced
this pull request
Feb 24, 2023
liujiayi771
pushed a commit
to liujiayi771/velox
that referenced
this pull request
Mar 3, 2023
liujiayi771
pushed a commit
to liujiayi771/velox
that referenced
this pull request
Mar 9, 2023
liujiayi771
pushed a commit
to liujiayi771/velox
that referenced
this pull request
Apr 1, 2023
rui-mo
added a commit
to rui-mo/velox
that referenced
this pull request
Apr 23, 2023
zhouyuan
pushed a commit
to zhouyuan/velox
that referenced
this pull request
Jun 7, 2023
relative pr: Check a fallback case in validation: using literal partition key in window function facebookincubator#148 Fix might_contain validate fallback and support struct literal facebookincubator#137 Implement datetime functions in velox/sparksql. facebookincubator#81 Parse options in SingularOrList correctly facebookincubator#48 Add SingularOrList support facebookincubator#45 Support if then in filter facebookincubator#74 Fix semi join output type and support existence join facebookincubator#67 Support decimal as partition column facebookincubator#167 Add the window support facebookincubator#61 Add expand operator facebookincubator#65 Support more cases of filter and its pushdown facebookincubator#14
Yohahaha
pushed a commit
to Yohahaha/velox
that referenced
this pull request
Jul 4, 2023
relative pr: Check a fallback case in validation: using literal partition key in window function facebookincubator#148 Fix might_contain validate fallback and support struct literal facebookincubator#137 Implement datetime functions in velox/sparksql. facebookincubator#81 Parse options in SingularOrList correctly facebookincubator#48 Add SingularOrList support facebookincubator#45 Support if then in filter facebookincubator#74 Fix semi join output type and support existence join facebookincubator#67 Support decimal as partition column facebookincubator#167 Add the window support facebookincubator#61 Add expand operator facebookincubator#65 Support more cases of filter and its pushdown facebookincubator#14
chenxu14
pushed a commit
to chenxu14/velox
that referenced
this pull request
Jul 5, 2023
relative pr: Check a fallback case in validation: using literal partition key in window function facebookincubator#148 Fix might_contain validate fallback and support struct literal facebookincubator#137 Implement datetime functions in velox/sparksql. facebookincubator#81 Parse options in SingularOrList correctly facebookincubator#48 Add SingularOrList support facebookincubator#45 Support if then in filter facebookincubator#74 Fix semi join output type and support existence join facebookincubator#67 Support decimal as partition column facebookincubator#167 Add the window support facebookincubator#61 Add expand operator facebookincubator#65 Support more cases of filter and its pushdown facebookincubator#14
PHILO-HE
pushed a commit
to PHILO-HE/velox
that referenced
this pull request
Jul 17, 2023
relative pr: Check a fallback case in validation: using literal partition key in window function facebookincubator#148 Fix might_contain validate fallback and support struct literal facebookincubator#137 Implement datetime functions in velox/sparksql. facebookincubator#81 Parse options in SingularOrList correctly facebookincubator#48 Add SingularOrList support facebookincubator#45 Support if then in filter facebookincubator#74 Fix semi join output type and support existence join facebookincubator#67 Support decimal as partition column facebookincubator#167 Add the window support facebookincubator#61 Add expand operator facebookincubator#65 Support more cases of filter and its pushdown facebookincubator#14
rui-mo
pushed a commit
to rui-mo/velox
that referenced
this pull request
Jul 21, 2023
relative pr: Check a fallback case in validation: using literal partition key in window function facebookincubator#148 Fix might_contain validate fallback and support struct literal facebookincubator#137 Implement datetime functions in velox/sparksql. facebookincubator#81 Parse options in SingularOrList correctly facebookincubator#48 Add SingularOrList support facebookincubator#45 Support if then in filter facebookincubator#74 Fix semi join output type and support existence join facebookincubator#67 Support decimal as partition column facebookincubator#167 Add the window support facebookincubator#61 Add expand operator facebookincubator#65 Support more cases of filter and its pushdown facebookincubator#14
rui-mo
pushed a commit
to rui-mo/velox
that referenced
this pull request
Jul 24, 2023
relative pr: Check a fallback case in validation: using literal partition key in window function facebookincubator#148 Fix might_contain validate fallback and support struct literal facebookincubator#137 Implement datetime functions in velox/sparksql. facebookincubator#81 Parse options in SingularOrList correctly facebookincubator#48 Add SingularOrList support facebookincubator#45 Support if then in filter facebookincubator#74 Fix semi join output type and support existence join facebookincubator#67 Support decimal as partition column facebookincubator#167 Add the window support facebookincubator#61 Add expand operator facebookincubator#65 Support more cases of filter and its pushdown facebookincubator#14
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.
Fix BaseVector::ensureWritable(*uncached,... &dictionaryCache_); call.
dictionaryCache_ is valid only for some rows
(cachedDictionaryIndices_), hence, a safe call to BaseVector::ensureWritable
must include all the rows not covered by cachedDictionaryIndices_. If
BaseVector::ensureWritable is called only for a subset of rows not covered by
cachedDictionaryIndices_, it will attempt to copy rows that are not valid
leading to a crash.
Also, set EvalCtx::isFinalSelection to false when evaluating "uncached" rows
to ensure that values in rows copies from the dictionaryCache_ don't get
lost.
These fixes are band-aids. They are workarounds for the fact that
BaseVector::ensureWritable primitive is not safe. It takes a set of rows to
make writable and copies all other rows. Since it has no way of knowing which
of the other rows are uninitialized it may cause a crash. A proper fix could
be to (1) introduce a way to specify that a particular row in a vector is
uninitilized and use this information when copying the rows to skip
uninitialized rows or (2) change BaseVector::ensureWritable to take a set of
rows to preserve and change all the call sites accordingly.