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

[datafusion] prepend physical_optimizer_rule before other rules #2114

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

igalshilman
Copy link
Contributor

@igalshilman igalshilman commented Oct 16, 2024

Why the order is important, from our source code (just few lines above in the diff)

 //
        // Prepend the join rewrite optimizer to the list of physical optimizers.
        //
        // It is important that the join rewrite optimizer will run before ProjectionPushdown::try_embed_to_hash_join.
        // because the SymmetricHashJoin doesn't support embedded projections out of the box
        //
        // If we don't do that, then when translating a HashJoinExc to SymmetricHashJoinExc we will lose the embedded projection
        // and the query will fail.
        // For example try the following query without prepending but rather appending:
        //
        // 'SELECT  b.service_key FROM sys_invocation_status a JOIN state b on a.target_service_key = b.service_key'
        //
        // A far more involved but potentially more robust solution would be wrap the SymmetricHashJoin in a ProjectionExec
        // If this would become an issue for any reason, then we can explore that alternative.
        //

This is a bug fix that was introduced in restatedev#2004
Which change the order that optimizers are registered.
The reason for this is described as a comment few lines above.
Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this issue @igalshilman. Looking at the size of the comment above this snippet, I probably need to see my ophthalmologist. Otherwise I can't explain how this slipped my sight during the review. +1 for merging :-)

@igalshilman
Copy link
Contributor Author

fyi: I've pushed another commit to use the state builder and removed the deprecated method usage.
Verified locally that these query work again.

@igalshilman igalshilman merged commit cb74485 into restatedev:main Oct 17, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants