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

*: improve syntax error code & message compatibility #9103

Merged
merged 7 commits into from
Jan 23, 2019
Merged

*: improve syntax error code & message compatibility #9103

merged 7 commits into from
Jan 23, 2019

Conversation

lysu
Copy link
Contributor

@lysu lysu commented Jan 17, 2019

What problem does this PR solve?

ref pingcap/parser#175

and pre-append user-friendly error message, and use right error code.

What is changed and how it works?

  • ref parser
  • use SyntaxError, SyntaxWarn convert error/warns returned by parser

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change

Side effects

  • N/A

Related changes

  • N/A

This change is Reviewable

@lysu
Copy link
Contributor Author

lysu commented Jan 17, 2019

/run-all-tests tidb-test=pr/723

@lysu
Copy link
Contributor Author

lysu commented Jan 17, 2019

/run-common-test tidb-test=pr/723
/run-integration-common-test tidb-test=pr/723

@lysu
Copy link
Contributor Author

lysu commented Jan 21, 2019

/run-all-tests tidb-test=pr/723

@lysu
Copy link
Contributor Author

lysu commented Jan 21, 2019

/run-unit-test tidb-test=pr/723

@lysu
Copy link
Contributor Author

lysu commented Jan 21, 2019

/run-all-tests tidb-test=pr/723

@lysu lysu requested review from tiancaiamao and jackysp January 21, 2019 07:50
@lysu
Copy link
Contributor Author

lysu commented Jan 21, 2019

/run-unit-test tidb-test=pr/723

1 similar comment
@lysu
Copy link
Contributor Author

lysu commented Jan 21, 2019

/run-unit-test tidb-test=pr/723

@lysu
Copy link
Contributor Author

lysu commented Jan 21, 2019

/run-unit-test tidb-test=pr/723

@lysu
Copy link
Contributor Author

lysu commented Jan 21, 2019

/run-all-tests tidb-test=pr/723

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@lysu
Copy link
Contributor Author

lysu commented Jan 21, 2019

/run-common-test tidb-test=pr/723
/run-unit-test tidb-test=pr/723

util/misc.go Outdated Show resolved Hide resolved
@lysu
Copy link
Contributor Author

lysu commented Jan 23, 2019

@tiancaiamao PTAL

@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 23, 2019
@lysu lysu merged commit 36176be into pingcap:master Jan 23, 2019
@codecov-io
Copy link

codecov-io commented Jan 23, 2019

Codecov Report

Merging #9103 into master will decrease coverage by <.01%.
The diff coverage is 5.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9103      +/-   ##
==========================================
- Coverage   67.21%   67.21%   -0.01%     
==========================================
  Files         371      371              
  Lines       76933    76942       +9     
==========================================
+ Hits        51714    51718       +4     
- Misses      20602    20608       +6     
+ Partials     4617     4616       -1
Impacted Files Coverage Δ
session/session.go 72.35% <0%> (ø) ⬆️
expression/simple_rewriter.go 14.83% <0%> (ø) ⬆️
executor/prepared.go 63.5% <0%> (ø) ⬆️
util/misc.go 42.85% <0%> (-11.69%) ⬇️
table/tables/gen_expr.go 76.92% <100%> (ø) ⬆️
executor/join.go 79.42% <0%> (+0.52%) ⬆️
ddl/delete_range.go 80.42% <0%> (+1.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ee643e...11fd6c1. Read the comment docs.

@AndrewDi
Copy link
Contributor

@tiancaiamao It seems like this PR has broken Integration Test:

FAIL: integration_test.go:3735: testIntegrationSuite.TestUnknowHintIgnore

integration_test.go:3740:
    tk.MustQuery("show warnings").Check(testkit.Rows("Warning 1064 You have an error in your SQL syntax; check the manual that corresponds to your TiDB version for the right syntax to use line 1 column 29 near \"unknown_hint(c1)*/ 1\" "))
/home/andrew/Developer/Go/src/github.com/pingcap/tidb/util/testkit/testkit.go:55:
    res.c.Assert(resBuff.String(), check.Equals, needBuff.String(), res.comment)
... obtained string = "[Warning 1064 You have an error in your SQL syntax; check the manual that corresponds to your TiDB version for the right syntax to use near 'line 1 column 29 near \"select /*+ unknown_hint(c1)*/ 1\" (total length 31)' at line %!d(MISSING)]\n"
... expected string = "[Warning 1064 You have an error in your SQL syntax; check the manual that corresponds to your TiDB version for the right syntax to use line 1 column 29 near \"unknown_hint(c1)*/ 1\" ]\n"
... sql:show warnings, args:[]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants