optimize: optimize sql parsing logic #7219
Conversation
# Conflicts: # changes/en-us/2.x.md # changes/zh-cn/2.x.md
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.x #7219 +/- ##
============================================
+ Coverage 53.28% 53.45% +0.17%
- Complexity 7069 7108 +39
============================================
Files 1169 1169
Lines 41585 41578 -7
Branches 4871 4873 +2
============================================
+ Hits 22158 22226 +68
+ Misses 17328 17251 -77
- Partials 2099 2101 +2
🚀 New features to boost your workflow:
|
slievrly
changed the title
|
The REPLACE INTO syntax is not standard SQL92/99 syntax, is not supported by all databases, and should be removed from the Base class. |
YongGoose
left a comment
YongGoose
left a comment
There was a problem hiding this comment.
The code seems to be well-written.
However, I have left a few comments.
Have a good day :)
| if (tableSource instanceof SQLSubqueryTableSource) { | ||
| return new SqlServerSelectForUpdateRecognizer(sql, ast); | ||
| } else { | ||
| List<SQLHint> hints = tableSource.getHints(); | ||
| if (CollectionUtils.isNotEmpty(hints)) { | ||
| List<String> hintsTexts = hints | ||
| .stream() | ||
| .map(hint -> { | ||
| if (hint instanceof SQLExprHint) { | ||
| SQLExpr expr = ((SQLExprHint) hint).getExpr(); | ||
| return expr instanceof SQLIdentifierExpr ? ((SQLIdentifierExpr) expr).getName() : ""; | ||
| } else if (hint instanceof SQLCommentHint) { | ||
| return ((SQLCommentHint) hint).getText(); | ||
| } | ||
| return ""; | ||
| }).collect(Collectors.toList()); | ||
| if (hintsTexts.contains("UPDLOCK")) { | ||
| return new SqlServerSelectForUpdateRecognizer(sql, ast); | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
In the given code, the else block is unnecessary because the return statement inside the if block already exits the method. Removing the else will improve readability and reduce nesting.
There was a problem hiding this comment.
Yes, it was an unwarranted mistake, I corrected it.
| // When dbtype are DM and SQLSERVER, druid cannot parse | ||
| // try { | ||
| // recognizerFactory.create(sql3, JdbcConstants.DM); | ||
| // } catch (Exception e) { | ||
| // //e.printStackTrace(); | ||
| // } |
There was a problem hiding this comment.
Is there a reason why you didn’t remove the single-line comments?
There was a problem hiding this comment.
This is a bad habit I developed in the past. I have deleted the comments so that later people can understand them easily
| // try { | ||
| // recognizerFactory.create(sql4, JdbcConstants.MYSQL); | ||
| // } catch (Exception e) { | ||
| // //e.printStackTrace(); | ||
| // } |
There was a problem hiding this comment.
Because merge sql syntax is not supported, the relevant code has been removed
|
@YongGoose Can we merge this code now? |
Sure :) |
5 participants
Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
#7217
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews