[Refactor] Refactor create table analyzer process #45498
Conversation
![starrocks-cr[bot]](https://avatars.githubusercontent.com/in/404334?s=60&v=4){kind=small}
| * @param partitionColumnExpr : the materialized view's partition expr | ||
| * @param connectContext : connect context of the current session. | ||
| * @param queryStatement : the sub query statment that contains the partition column slot ref | ||
| * @return : return the resolved partition expr. | ||
| * @return : return the resolved partition expr. | ||
| */ | ||
| private Expr resolvePartitionExpr(Expr partitionColumnExpr, | ||
| ConnectContext connectContext, |
There was a problem hiding this comment.
The most risky bug in this code is:
The removal and re-addition of IndexDef import without clear context on why it's been moved.
You can modify the code like this to ensure clarity on IndexDef usage or if the relocation was necessary, justify or comment on the reason behind such an action:
// Original location with a comment explaining why it might need to be relocated or kept here.
import com.starrocks.analysis.IndexDef;
// Further down if moved
// Moved to com.starrocks.sql.ast for reasons X, Y, Z.
import com.starrocks.sql.ast.IndexDef;
| return pos; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The most risky bug in this code is:
Improper construction of KeysDesc object with potentially mutable List reference.
You can modify the code like this:
public KeysDesc(KeysType type, List<String> keysColumnNames, NodePosition pos) {
this.pos = pos;
this.type = type;
// Create a new ArrayList from the provided list to ensure immutability of internal state
this.keysColumnNames = new ArrayList<>(keysColumnNames);
}
| public boolean isHasGeneratedColumn() { | ||
| return hasGeneratedColumn; | ||
| } | ||
|
|
||
| public static CreateTableStmt read(DataInput in) throws IOException { | ||
| throw new RuntimeException("CreateTableStmt serialization is not supported anymore."); | ||
| } |
There was a problem hiding this comment.
The most risky bug in this code is:
The columns and indexes fields in the CreateTableStmt class are not initialized, which can lead to a NullPointerException when their getter methods are called before they are explicitly set.
You can modify the code like this:
@@ -48,14 +45,19 @@ public class CreateTableStmt extends DdlStmt {
private List<AlterClause> rollupAlterClauseList;
// set in analyze
- private List<Column> columns;
+ private List<Column> columns = Lists.newArrayList();
private List<String> sortKeys = Lists.newArrayList();
- private List<Index> indexes;
+ private List<Index> indexes = Lists.newArrayList();
// for backup. set to -1 for normal use
private int tableSignature;
boolean hasHll = false;
boolean hasBitmap = false;
boolean hasReplace = false;
boolean hasGeneratedColumn = false;This ensures that both columns and indexes are immediately usable after an instance of CreateTableStmt is created, avoiding the risk of encountering a NullPointerException if getters are called before these fields are explicitly set using their respective setter methods.
HangyuanLiu
force-pushed
the
0511-refactor-create-table
branch
from
f4e6304
to
5110837
Compare
HangyuanLiu
force-pushed
the
0511-refactor-create-table
branch
2 times, most recently
from
03f884b
to
7fbdec2
Compare
Signed-off-by: HangyuanLiu <[email protected]>
HangyuanLiu
force-pushed
the
0511-refactor-create-table
branch
from
7fbdec2
to
108e934
Compare
|
[FE Incremental Coverage Report]✅ pass : 389 / 448 (86.83%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
Signed-off-by: HangyuanLiu <[email protected]>
Why I'm doing:
What I'm doing:
No logic modification, just tidying up the code logic
Fixes #issue
fix #44543
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: