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 select star, col1 orderby col1 bug. #7743

Merged
merged 10 commits into from
Apr 8, 2021

Conversation

yangxuanjia
Copy link
Contributor

shard env.
fix select star, col1 orderby col1 bug.
The problem is col1 parse index is 1. But before this column, there have a star(*), this will be some column.
so finally orderby from multi shards, use order index 1 will be make a bug.
This is a company inner fix, base on Vitess 6.0.

So I not really sure, Vitess9.0 whether is fix this bug or doesn't support this typical sql.

select t.*, t.customer_id from customer t where email = 'aaa11' order by customer_id desc limit 0, 3;

@systay
Copy link
Collaborator

systay commented Mar 30, 2021

Hi @yangxuanjia!

Thank you so much for sharing this fix with us.
We need two things to make progress on this PR. First, we need all commits to have a signoff signature to be acceptable.

  1. You will need to amend your commits with git commit --amend --signoff so they can be legally accepted. Let me know if this is an issue and I can guide you through it.

  2. We need tests to be added, showing what you are trying to accomplish. I think adding something to the postprocess.txt could work.

@systay
Copy link
Collaborator

systay commented Mar 30, 2021

Oh, and I see that you will also need to do a make sizegen to update the CachedSize methods

AdamKorcz and others added 7 commits April 2, 2021 16:42
Signed-off-by: AdamKorcz <adam@adalogics.com>
Signed-off-by: yangxuanjia <yangxuanjia@jd.com>
Signed-off-by: Hormoz K <hkheradm@gmail.com>
Signed-off-by: yangxuanjia <yangxuanjia@jd.com>
Signed-off-by: Hormoz K <hkheradm@gmail.com>
Signed-off-by: yangxuanjia <yangxuanjia@jd.com>
Signed-off-by: Hormoz K <hkheradm@gmail.com>
Signed-off-by: yangxuanjia <yangxuanjia@jd.com>
Signed-off-by: Hormoz K <hkheradm@gmail.com>
Signed-off-by: yangxuanjia <yangxuanjia@jd.com>
… by customer_id desc limit 0, 3;

Signed-off-by: yangxuanjia <yangxuanjia@jd.com>
Signed-off-by: yangxuanjia <yangxuanjia@jd.com>
Signed-off-by: yangxuanjia <yangxuanjia@jd.com>
Signed-off-by: yangxuanjia <yangxuanjia@jd.com>
Signed-off-by: yangxuanjia <yangxuanjia@jd.com>
@yangxuanjia
Copy link
Contributor Author

Hi @systay,
I done the work, please review it at your earliest convenience.

@frouioui
Copy link
Member

frouioui commented Apr 7, 2021

Hello 👋 Looked at the changes earlier with @systay, it looks like a great fix. However, we are still trying to understand if the use of starColFixedIndex can be avoided by using only orderBy.

I personally think we should add an error message in planRouteOrdering where we break (go/vt/vtgate/planbuilder/ordering.go:308). For instance, if the query below was allowed one day, it would break and starColFixedIndex would be incorrect.

select *, t.id from user t, user_extra where t.email = 'aaa11' order by t.id desc limit 0, 3;

Today's code returns the error "unsupported: '*' expression in cross-shard query", a similar message can be added in planRouteOrdering.

Copy link
Collaborator

@systay systay left a comment

Choose a reason for hiding this comment

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

Thank you @yangxuanjia for this contribution.

Long term, I think we should rewrite queries so that when the query gets to the planner, star expressions have been expanded, but until then, this is solving a real problem. Much appreciated.

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.

5 participants