Fix the extract values function sigature when used with filter process in selective reader#10956
Fix the extract values function sigature when used with filter process in selective reader#10956xiaoxmeng wants to merge 1 commit intofacebookincubator:mainfrom
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
This pull request was exported from Phabricator. Differential Revision: D62394606 |
Yuhta
left a comment
Yuhta
left a comment
There was a problem hiding this comment.
You need to fix the format before merging
|
This pull request was exported from Phabricator. Differential Revision: D62394606 |
xiaoxmeng
force-pushed
the
export-D62394606
branch
from
f5b47d7 to
ce7e91b
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62394606 |
xiaoxmeng
force-pushed
the
export-D62394606
branch
from
ce7e91b to
ab643d2
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62394606 |
xiaoxmeng
force-pushed
the
export-D62394606
branch
from
ab643d2 to
defe585
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62394606 |
xiaoxmeng
force-pushed
the
export-D62394606
branch
from
defe585 to
13c41bf
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62394606 |
xiaoxmeng
force-pushed
the
export-D62394606
branch
from
13c41bf to
5aa5972
Compare
xiaoxmeng
force-pushed
the
export-D62394606
branch
from
5aa5972 to
9afe77d
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62394606 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D62394606 |
xiaoxmeng
force-pushed
the
export-D62394606
branch
from
9afe77d to
3135b47
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62394606 |
xiaoxmeng
force-pushed
the
export-D62394606
branch
from
3135b47 to
935292c
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62394606 |
xiaoxmeng
force-pushed
the
export-D62394606
branch
from
935292c to
9e97eb6
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62394606 |
xiaoxmeng
force-pushed
the
export-D62394606
branch
from
9e97eb6 to
16f3717
Compare
…s in selective reader (facebookincubator#10956) Summary: Pull Request resolved: facebookincubator#10956 The processFilter depends on the exact value callback type match to detect if the filter to drop value or not. If not drop value, it will add value to the result buffer of the selective reader. For null filter type, it add nulls to the null result buffer of the selective reader. The recent refactor changes the extract value callback from T to const T& which breaks the comparison. It tries to add value to the null result buffer which is not initialized in the prepare read of the selective reader as the reader is a filter only reader. This PR reverts the signature change and verified with both failed query shadow and a new unit test to repro this. We will refactor this later to avoid a copy extract value callback later Reviewed By: Yuhta Differential Revision: D62394606
|
This pull request was exported from Phabricator. Differential Revision: D62394606 |
xiaoxmeng
force-pushed
the
export-D62394606
branch
from
16f3717 to
cabe3b1
Compare
|
This pull request has been merged in ef251d4. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
3 participants
Summary:
The processFilter depends on the exact value callback type match to detect if the filter to drop value or not.
If not drop value, it will add value to the result buffer of the selective reader. For null filter type, it add nulls to
the null result buffer of the selective reader. The recent refactor changes the extract value callback from
T to const T& which breaks the comparison. It tries to add value to the null result buffer which is not initialized
in the prepare read of the selective reader as the reader is a filter only reader.
This PR reverts the signature change and verified with both failed query shadow and a new unit test to repro this.
We will refactor this later to avoid a copy extract value callback later
Differential Revision: D62394606