Skip to content

Add query configs to turn off expression evaluation optimizations#10902

Closed
bikramSingh91 wants to merge 1 commit intofacebookincubator:mainfrom
bikramSingh91:export-D61943875
Closed

Add query configs to turn off expression evaluation optimizations#10902
bikramSingh91 wants to merge 1 commit intofacebookincubator:mainfrom
bikramSingh91:export-D61943875

Conversation

@bikramSingh91
Copy link
Copy Markdown
Contributor

@bikramSingh91 bikramSingh91 commented Aug 30, 2024

Summary:
This change adds query configs to individually turn off expression
evaluation optimizations like dictionary peeling, dictionary
memoization, reusing shared subexpression results and deferring
lazy vector loading.
The goal is to streamline debugging in production and enable prompt
mitigation of bugs or regressions caused by the optimization or
surfaced due to it.

Note: When peeling is turned off, we still ensure that single arg
functions recieve a flat input.

Test Plan:
Added unit tests for query configs and when each optimization is
individually turned off.

Differential Revision: D61943875

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 30, 2024
@netlify
Copy link
Copy Markdown

netlify bot commented Aug 30, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit c052d6a
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/66df67fe00a11900085178bf

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D61943875

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D61943875

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you send a PR to update contributing guidelines / coding style to describe when to use "debug_xxx" configs?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbasmanova Created a seperate PR that I will merge after this since it references one of the configs added here: #10934

Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@bikramSingh91
Copy link
Copy Markdown
Contributor Author

This change cause performance regressions in benchmarks, namely in SimpleArithmetic, SimpleCastExpr, ComparisonConjunct and Preproc. This was due to the added cost of creating an EvalCtx, particularly due to the calls to queryConfig for fetching the configs during construction. This is because on every iteration of the benchmark a new EvalCtx is created and each call to the query ctx contains a map lookup and a string to bool/int conversion. Even though its a small cost it shows up on simple benchmarks as they are designed to be very targeted on a single simple operation.

To avoid this, I will move the params up into ExecCtx instead which is only created once per driver and once per benchmark.

…cebookincubator#10902)

Summary:
Pull Request resolved: facebookincubator#10902

This change adds query configs to individually turn off expression
evaluation optimizations like dictionary peeling, dictionary
memoization, reusing shared subexpression results and deferring
lazy vector loading.
The goal is to streamline debugging in production and enable prompt
mitigation of bugs or regressions caused by the optimization or
surfaced due to it.

Note: When peeling is turned off, we still ensure that single arg
functions recieve a flat input

Reviewed By: mbasmanova

Differential Revision: D61943875
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D61943875

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request has been merged in 55931e7.

@conbench-facebook
Copy link
Copy Markdown

Conbench analyzed the 1 benchmark run on commit 55931e79.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants