Add ability to verify expression fuzzer runs on a subset of rows #11267
Conversation
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D64366745 |
✅ Deploy Preview for meta-velox canceled.
|
| RowVectorPtr reducedVector = std::dynamic_pointer_cast<RowVector>( | ||
| BaseVector::create(rowVector->type(), cnt, rowVector->pool())); | ||
| SelectivityVector rowsToCopy(cnt); | ||
| reducedVector->copy(rowVector.get(), rowsToCopy, rawIndices); |
There was a problem hiding this comment.
nit: This piece of code can be simply replaced with BaseVector::wrapInDictionary(nullptr, indices, cnt, rowVector)?
There was a problem hiding this comment.
True, the only reason I did this was to avoid creating a top level RowVector which is dictionary encoded which we do not expect in velox. This basically reduces some steps if we did (wrapInDictionary + flattenVector) instead.
This however will only be used by the presto runner, where it will probably write this to disk first. I have not had a chance to run it with PQR yet, but if that codepath supports an encoded top level row vector then we should be good and I can replace this with your suggestion.
| @@ -102,7 +102,8 @@ TEST_F(ExpressionVerifierUnitTest, persistReproInfo) { | |||
| auto plan = parseExpression("always_throws(c0)", asRowType(data->type())); | |||
|
|
|||
| removeDirecrtoryIfExist(localFs, reproPath); | |||
| VELOX_ASSERT_THROW(verifier.verify({plan}, data, nullptr, false), ""); | |||
| VELOX_ASSERT_THROW( | |||
| verifier.verify({plan}, data, std::nullopt, nullptr, false), ""); | |||
There was a problem hiding this comment.
Could you also add a test case of verify() applying only on a subset of rows? (E.g., maybe make the unselected rows throw if they are evaluated, then assert verify() doesn't throw.)
bikramSingh91
force-pushed
the
export-D64366745
branch
from
09e2315 to
730d5cc
Compare
…ebookincubator#11267) Summary: Currently, the expression fuzzer has a phase where it re-runs rows that did not throw an error to ensure evaluation is consistent for them. To achieve this, it currently wraps the inputs with a dictionary that only points to the subset of those rows. This results a change in the encoding of inputs which can cause differences in eval paths taken between phases. To address this and ensure the same paths are taken for each evaluation phase, this change introduces the ability for the expression verifier to only verify a subset of the input rows. The aforementioned fuzzer run phase can only specify the non error rows and maintain the original input row. Follow up: After this change, it would be relevant to also store the input selectivity vector. A subsequent change will be added that would add this ability and make corresponding changes to the ExpressionRunner Differential Revision: D64366745
|
This pull request was exported from Phabricator. Differential Revision: D64366745 |
…ebookincubator#11267) Summary: Currently, the expression fuzzer has a phase where it re-runs rows that did not throw an error to ensure evaluation is consistent for them. To achieve this, it currently wraps the inputs with a dictionary that only points to the subset of those rows. This results a change in the encoding of inputs which can cause differences in eval paths taken between phases. To address this and ensure the same paths are taken for each evaluation phase, this change introduces the ability for the expression verifier to only verify a subset of the input rows. The aforementioned fuzzer run phase can only specify the non error rows and maintain the original input row. Follow up: After this change, it would be relevant to also store the input selectivity vector. A subsequent change will be added that would add this ability and make corresponding changes to the ExpressionRunner Differential Revision: D64366745
bikramSingh91
force-pushed
the
export-D64366745
branch
from
730d5cc to
50d636f
Compare
|
This pull request was exported from Phabricator. Differential Revision: D64366745 |
…ebookincubator#11267) Summary: Currently, the expression fuzzer has a phase where it re-runs rows that did not throw an error to ensure evaluation is consistent for them. To achieve this, it currently wraps the inputs with a dictionary that only points to the subset of those rows. This results a change in the encoding of inputs which can cause differences in eval paths taken between phases. To address this and ensure the same paths are taken for each evaluation phase, this change introduces the ability for the expression verifier to only verify a subset of the input rows. The aforementioned fuzzer run phase can only specify the non error rows and maintain the original input row. Follow up: After this change, it would be relevant to also store the input selectivity vector. A subsequent change will be added that would add this ability and make corresponding changes to the ExpressionRunner Differential Revision: D64366745
Thank you @kagamiori for verifying using PrestoQueryRunner. |
|
This pull request has been merged in 7c93eba. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Summary:
Currently, the expression fuzzer has a phase where it re-runs rows
that did not throw an error to ensure evaluation is consistent for
them. To achieve this, it currently wraps the inputs with a dictionary
that only points to the subset of those rows. This results a change in
the encoding of inputs which can cause differences in eval paths taken
between phases. To address this and ensure the same paths are taken
for each evaluation phase, this change introduces the ability for the
expression verifier to only verify a subset of the input rows. The
aforementioned fuzzer run phase can only specify the non error rows
and maintain the original input row.
Follow up: After this change, it would be relevant to also store the
input selectivity vector. A subsequent change will be added that would
add this ability and make corresponding changes to the ExpressionRunner
Differential Revision: D64366745