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.

Sign up for GitHub

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

Jump to bottom

Fix Vtexplains StripComments #5984

Merged
merged 4 commits into from
Apr 2, 2020
Merged

Fix Vtexplains StripComments #5984

merged 4 commits into from
Apr 2, 2020

Conversation

saifalharthi
Copy link
Contributor

@saifalharthi saifalharthi commented Mar 27, 2020 •
edited
Loading

Closes #5981
Signed-off-by: Saif Alharthi saif@saifalharthi.me

@harshit-gangal
Copy link
Member

Issue related #5981

@saifalharthi
Copy link
Contributor Author

saifalharthi commented Mar 28, 2020 •
edited
Loading

Thanks @harshit-gangal. Updated the PR description

@saifalharthi
Fix Vtexplains StripComments
1ddeee0 
Signed-off-by: Saif Alharthi <saif@saifalharthi.me>

Modify for better readability

Signed-off-by: Saif Alharthi <saif@saifalharthi.me>

Make sure anything inside qoutations will not be removed

Signed-off-by: Saif Alharthi <saif@saifalharthi.me>

Modity test for readablity

Signed-off-by: Saif Alharthi <saif@saifalharthi.me>
@saifalharthi saifalharthi force-pushed the sa-fix-vtexplain-strip-comments branch from ba91c1f to 1ddeee0 Compare March 28, 2020 16:56
@harshit-gangal
Copy link
Member

harshit-gangal commented Mar 29, 2020 •
edited
Loading

I tried below case and got this
test input: "select * from customer where name like 'abc*/la/*xyz'"
got SQL -> select * from customer where name like 'abcxyz'
want -> select * from customer where name like 'abc*/la/*xyz'
is my expectation correct?

@saifalharthi
Use Parser to handle comments if parseable
e907bdb 
Signed-off-by: Saif Alharthi <saif@saifalharthi.me>
@saifalharthi
Copy link
Contributor Author

@harshit-gangal Yes, your expectation is correct. I changed the code so it can use the parser. And the previous code if it is not. I am not sure if we should change the tests to display an error the string cannot be parsed. But this seems like a good compromise.

@harshit-gangal
Copy link
Member

Let's switch this function to use splitMarginComments and also fix the current issues with that function.

systay and others added 2 commits April 1, 2020 09:41
@systay
Try to use SplitMarginComments instead of having StripComments() sepa…
9b2f210 
…rately

Signed-off-by: Andres Taylor <andres@planetscale.com>
@saifalharthi
Remove Partition comment from test to match current behaviour for Vitess
309c888 
Signed-off-by: Saif Alharthi <saif@saifalharthi.me>
@saifalharthi
Copy link
Contributor Author

I discussed with @sougou about this issue and decided to remove PARTITION so vtexplain can match vitess's current behavior.

@harshit-gangal harshit-gangal merged commit cde37b0 into vitessio:master Apr 2, 2020
@ajm188 ajm188 mentioned this pull request Jun 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harshit-gangal harshit-gangal harshit-gangal approved these changes

@sougou sougou Awaiting requested review from sougou

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sql like select * from t where name like '*//*' in StripComments will lead to a deadloop and a cpu usage raise
3 participants
@saifalharthi @harshit-gangal @systay