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

Gen4Fallback bug fix #9487

Merged
merged 3 commits into from
Jan 10, 2022
Merged

Gen4Fallback bug fix #9487

merged 3 commits into from
Jan 10, 2022

Conversation

systay
Copy link
Collaborator

@systay systay commented Jan 10, 2022

Description

A minor planner fix

When using the Gen4fallback planner, we were not protecting the AST from changes from the Gen4 planner, which lead to the V3 planner getting an already updated AST and failing

Copy link
Member

@harshit-gangal harshit-gangal left a comment

Choose a reason for hiding this comment

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

looks good, need a test for it :)

@systay systay requested a review from GuptaManan100 January 10, 2022 07:58
Comment on lines 61 to 62
semTable.ProjectionErr = vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: cross-shard left join and where clause")
return nil
Copy link
Member

Choose a reason for hiding this comment

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

Why revert this change? Isn't it okay to keep it as a ShardedErr?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, we have to either return an error, or build something that adds the predicate to the operator tree. At the moment we don't have a filter operator, so this will be easy to fix post-remove-query-trees

@systay systay changed the title Small planner fixes Gen4Fallback bug fix Jan 10, 2022
@systay systay merged commit db504fb into vitessio:main Jan 10, 2022
@systay systay deleted the planner-fixes branch January 10, 2022 12:56
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.

3 participants