Fix dynamic filters pushdown from multiple operators on same column #10478
Conversation
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D59814502 |
✅ Deploy Preview for meta-velox canceled.
|
|
This pull request was exported from Phabricator. Differential Revision: D59814502 |
…acebookincubator#10478) Summary: Pull Request resolved: facebookincubator#10478 The data structure we used in table scan operator to keep dynamic filters can only hold one filter per column. When multiple operators pushing dynamic filters to the same column, the later ones would overwrite the previous filters on the same column. Fix this by merging the existing filter with the new filter. Differential Revision: D59814502
|
I understand it's hard to detect such bugs. But wondering if there is any fuzzer improvement done for such cases so that we can gain more confidence on the code. |
Yuhta
changed the title
|
This pull request was exported from Phabricator. Differential Revision: D59814502 |
…acebookincubator#10478) Summary: Pull Request resolved: facebookincubator#10478 The data structure we used in table scan operator to keep dynamic filters can only hold one filter per column. When multiple operators pushing dynamic filters to the same column, the later ones would overwrite the previous filters on the same column. Fix this by merging the existing filter with the new filter. Differential Revision: D59814502
There was a problem hiding this comment.
@Yuhta Jimmy, thank you for the fix. Would you double check that the test fails without the changes before landing?
…acebookincubator#10478) Summary: Pull Request resolved: facebookincubator#10478 The data structure we used in table scan operator to keep dynamic filters can only hold one filter per column. When multiple operators pushing dynamic filters to the same column, the later ones would overwrite the previous filters on the same column. Fix this by merging the existing filter with the new filter. Reviewed By: mbasmanova Differential Revision: D59814502
|
@mbasmanova Sure I checked it failed before pushing this version |
|
This pull request was exported from Phabricator. Differential Revision: D59814502 |
|
@amitkdutta A new full plan fuzzer would be needed for such cases. And we need to fuzz the query plan, in addition to data, and check against some invariance (for example with and without dynamic filter enabled). We need to add more invariances in order to catch more bugs though, but they are hard to find. Another way is to have a full SQL fuzzer and compare the result of Prestissimo with Presto Java. The tricky part is to generate proper SQL and data not always error out or hanging there forever. |
|
This pull request has been merged in 2e3bf33. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
@Yuhta Thanks for the reply. What you are describing (a full SQL fuzzer) is actually a verification system (e.g. Presto Verifier). Basically, a system that takes a set of SQL (either from random sampled or other way), test with a gold version (could be Prestissimo with production velox or Presto java) with new version. The comparsion of end to end SQL is much more useful and direct indicator of correctness. If you want to get a fuzzer like behavior, generating random SQL and feed it through the framework will achieve that. |
Summary:
The data structure we used in table scan operator to keep dynamic
filters can only hold one filter per column. When multiple operators pushing
dynamic filters to the same column, the later ones would overwrite the previous
filters on the same column. Fix this by merging the existing filter with the
new filter.
Differential Revision: D59814502