[Refactor] Add DML Statement Analyzer to unify some dml analyze logical #53974
Conversation
![starrocks-cr[bot]](https://avatars.githubusercontent.com/in/404334?s=60&v=4){kind=small}
| DMLStmtAnalyzer.analyze(statement, context); | ||
| return null; | ||
| } | ||
|
|
||
| // ------------------------------------------- Cluster Management Statement ---------------------------------------- | ||
|
|
||
| @Override |
There was a problem hiding this comment.
The most risky bug in this code is:
The duplicate handling of the visitInsertStatement, visitUpdateStatement, and visitDeleteStatement methods using two different analyzers (InsertAnalyzer, UpdateAnalyzer, DeleteAnalyzer vs. DMLStmtAnalyzer) might lead to inconsistent behavior or the incorrect analyzer being used due to potential conflicts.
You can modify the code like this:
// Remove duplicates:
// The old insert statement handler is outdated; ensure only one consistent handler is retained.
@Override
public Void visitSubmitTaskStatement(SubmitTaskStmt statement, ConnectContext context) {
taskStmt = queryStatement;
} else if (statement.getInsertStmt() != null) {
InsertStmt insertStmt = statement.getInsertStmt();
DMLStmtAnalyzer.analyze(insertStmt, context); // Adjusted for consistency
taskStmt = insertStmt;
}
// Keep DML Statement section using DMLStmtAnalyzer as intended.
// Remove previous overridden handlers to avoid conflicts:
// No change needed; duplicates already appear removed in current version.
| return null; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The most risky bug in this code is:
The DMLStmtAnalyzerVisitor class doesn't properly implement all methods from the AstVisitor interface, which might lead to the visit method not being called as expected for DmlStmt.
You can modify the code like this:
static class DMLStmtAnalyzerVisitor implements AstVisitor<Void, ConnectContext> {
public void analyze(DmlStmt dmlStmt, ConnectContext context) {
dmlStmt.getTableName().normalization(context);
visit(dmlStmt, context);
}
@Override
public Void visitInsertStatement(InsertStmt stmt, ConnectContext context) {
InsertAnalyzer.analyze(stmt, context);
return null;
}
@Override
public Void visitUpdateStatement(UpdateStmt stmt, ConnectContext context) {
UpdateAnalyzer.analyze(stmt, context);
return null;
}
@Override
public Void visitDeleteStatement(DeleteStmt stmt, ConnectContext context) {
DeleteAnalyzer.analyze(stmt, context);
return null;
}
@Override
public Void visit(DmlStmt dmlStmt, ConnectContext context) {
// Handle default visit behavior or throw an error if needed.
throw new UnsupportedOperationException("Visit method not implemented for this statement type");
}
}This modification adds a generic visit method to handle cases where specific visitor methods for other DmlStmt types are missing. Adjust it according to your actual use case or specification.
|
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 20 / 21 (95.24%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
Why I'm doing:
There is some analyze logic that exists in all DML Statements. Here is a common abstracted visitor
What I'm doing:
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: