Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug with MetricFlowQueryRequest.sql_optimization_level handling #1524

Merged
merged 3 commits into from
Nov 14, 2024

Conversation

plypaul
Copy link
Contributor

@plypaul plypaul commented Nov 13, 2024

MetricFlowQueryRequest has the sql_optimization_level field which is supposed to control the SQL optimization level. However, it was not getting handled correctly, so this PR fixes that issue.

@cla-bot cla-bot bot added the cla:yes label Nov 13, 2024
@plypaul plypaul changed the title Fix bug MetricFlowQueryRequest.sql_optimization_level handling. Fix bug with MetricFlowQueryRequest.sql_optimization_level handling Nov 13, 2024
@plypaul plypaul marked this pull request as ready for review November 13, 2024 06:34
Copy link
Contributor

@courtneyholcomb courtneyholcomb left a comment

Choose a reason for hiding this comment

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

LGTM!

def test_sql_optimization_level(it_helpers: IntegrationTestHelpers) -> None:
"""Check that different SQL optimization levels produce different SQL."""
assert (
SqlQueryOptimizationLevel.default_level() != SqlQueryOptimizationLevel.O0
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker for this PR, but I'm curious - do you know why the 00 enum option uses two different 0 characters? I've never seen that before!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Borrows from the syntax from gcc: https://www.rapidtables.com/code/linux/gcc/gcc-o.html

Base automatically changed from p--cte--16 to main November 14, 2024 17:20
@plypaul plypaul merged commit b5b6d3f into main Nov 14, 2024
15 checks passed
@plypaul plypaul deleted the p--cte--17 branch November 14, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants