[Refactor] Introduce ColumnId to support Column renaming (part4) #45842
+741
−416
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![starrocks-cr[bot]](https://avatars.githubusercontent.com/in/404334?s=60&v=4){kind=small}
| @@ -800,7 +816,7 @@ public void gsonPostProcess() throws IOException { | |||
| @Override | |||
| public void gsonPreProcess() throws IOException { | |||
| if (generatedColumnExpr != null) { | |||
| generatedColumnExprSerialized = new GsonUtils.ExpressionSerializedObject(generatedColumnExpr.toSql()); | |||
| generatedColumnExprSerialized = ExpressionSerializedObject.create(generatedColumnExpr); | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The most risky bug in this code is:
Inconsistent handling of null return value leading to potential NullPointerException
You can modify the code like this:
public List<SlotRef> getGeneratedColumnRef(Map<ColumnId, Column> idToColumn) {
List<SlotRef> slots = new ArrayList<>();
if (generatedColumnExpr == null) {
return slots; // Return an empty list instead of null to avoid potential NullPointerException.
} else {
generatedColumnExpr.convertToColumnNameExpr(idToColumn).collect(SlotRef.class, slots);
return slots;
}
}This change ensures that getGeneratedColumnRef method returns an empty list instead of null, reducing the risk of a NullPointerException in contexts where the returned list is immediately used or iterated over.
| @@ -1746,7 +1747,7 @@ public Status doAfterRestore(MvRestoreContext mvRestoreContext) throws DdlExcept | |||
| // change ExpressionRangePartitionInfo because mv's db may be changed. | |||
| if (partitionInfo instanceof ExpressionRangePartitionInfo) { | |||
| ExpressionRangePartitionInfo expressionRangePartitionInfo = (ExpressionRangePartitionInfo) partitionInfo; | |||
| Preconditions.checkState(expressionRangePartitionInfo.getPartitionExprs().size() == 1); | |||
| Preconditions.checkState(expressionRangePartitionInfo.getPartitionExprsSize() == 1); | |||
| expressionRangePartitionInfo.renameTableName(db.getFullName(), this.name); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The most risky bug in this code is:
Incorrect handling of ExpressionRangePartitionInfo.getPartitionExprs() method calls without considering the change to require idToColumn as a parameter.
You can modify the code like this:
- partitionRefTableExprs.get(0).setType(expressionRangePartitionInfo.getPartitionExprs().get(0).getType());
+ partitionRefTableExprs.get(0).setType(expressionRangePartitionInfo.getPartitionExprs(idToColumn).get(0).getType());
- Expr partitionExpr = expressionRangePartitionInfo.getPartitionExprs().get(0);
+ Expr partitionExpr = expressionRangePartitionInfo.getPartitionExprs(idToColumn).get(0);
- Preconditions.checkState(expressionRangePartitionInfo.getPartitionExprs().size() == 1);
+ Preconditions.checkState(expressionRangePartitionInfo.getPartitionExprsSize() == 1);This adjustment ensures the updated method signatures that now require an idToColumn map for fetching partition expressions are used correctly, adhering to the changes introduced. Additionally, it corrects the call from .getPartitionExprs().size() to .getPartitionExprsSize() reflecting the intended code behavior while avoiding potential NullPointerExceptions or incorrect behavior due to missing arguments.
| for (Expr expr : partitionExprs) { | ||
| expr.accept(renameVisitor, null); | ||
| for (ColumnIdExpr expr : partitionExprs) { | ||
| expr.getExpr().accept(renameVisitor, null); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The most risky bug in this code is:
Incorrect handling and potential loss of partition expressions during serialization/deserialization
You can modify the code like this:
public static PartitionInfo read(DataInput in) throws IOException {
// Instead of returning null, properly implement the deserialization logic if applicable.
// The provided code snippet does not include the complete implementation for read and write methods,
// which are critical for correct serialization and deserialization of the object.
// Assuming ExpressionSerializedObject and ColumnIdExpr have proper serialize and deserialize methods implemented.
ExpressionRangePartitionInfo info = new ExpressionRangePartitionInfo();
info.readFields(in); // Assuming this reads the basic fields required except partition expressions
// Properly handle reading the serialized form of partition expressions.
String json = Text.readString(in);
List<ExpressionSerializedObject> expressionSerializedObjects = GsonUtils.GSON.fromJson(json,
new TypeToken<List<ExpressionSerializedObject>>() {}.getType());
List<ColumnIdExpr> partitionExprs = new ArrayList<>();
for (ExpressionSerializedObject expressionSerializedObject : expressionSerializedObjects) {
if (expressionSerializedObject != null) {
ColumnIdExpr columnIdExpr = expressionSerializedObject.deserialize(); // Assuming an appropriate deserialize method exists
partitionExprs.add(columnIdExpr);
}
}
info.setPartitionExprs(partitionExprs); // Ensure this method properly updates the object's state
return info;
}
@Override
public void write(DataOutput out) throws IOException {
super.write(out);
// Serialize the partition expressions into JSON string
List<ExpressionSerializedObject> serializedPartitionExprs = new ArrayList<>();
for (ColumnIdExpr columnIdExpr : partitionExprs) {
// Assuming an appropriate method to serialize a ColumnIdExpr to ExpressionSerializedObject
serializedPartitionExprs.add(ExpressionSerializedObject.create(columnIdExpr));
}
// Convert the list of serialized objects to JSON string and write it
String json = GsonUtils.GSON.toJson(serializedPartitionExprs);
Text.writeString(out, json);
}This rectifies the potential issue by ensuring that the read and write methods for serialization/deserialization are appropriately implemented, thus avoiding data loss or inconsistencies.
| @@ -62,7 +63,8 @@ | |||
|
|
|||
| public class SlotRef extends Expr { | |||
| private TableName tblName; | |||
| private String col; | |||
| private String colName; | |||
There was a problem hiding this comment.
this is the logical name , right?
| } | ||
| } | ||
|
|
||
| private static void setColumnIdToColumnName(Expr expr) { |
There was a problem hiding this comment.
Suggested change
| private static void setColumnIdToColumnName(Expr expr) { | |
| private static void setColumnIdByColumnName(Expr expr) { |
better?
| @Override | ||
| public String visitSlot(SlotRef node, Void context) { | ||
| if (node.getTblNameWithoutAnalyzed() != null) { | ||
| return node.getTblNameWithoutAnalyzed().toString() + "." + node.getColumnId().getId(); |
There was a problem hiding this comment.
why use column id here, should use column name?
| @@ -0,0 +1,120 @@ | |||
| diff a/fe/fe-core/src/main/java/com/starrocks/sql/common/MetaUtils.java b/fe/fe-core/src/main/java/com/starrocks/sql/common/MetaUtils.java (rejected hunks) | |||
| return expr; | ||
| } | ||
|
|
||
| public String serialize() { |
There was a problem hiding this comment.
why call it serialize/deserialize? how about toSQL/fromSQL?
| @@ -252,7 +254,7 @@ public ColumnDef toColumnDef() { | |||
| } | |||
| } | |||
| ColumnDef col = new ColumnDef(name, new TypeDef(type), null, isKey, aggregationType, isAllowNull, | |||
| defaultValueDef, isAutoIncrement, generatedColumnExpr, comment); | |||
| defaultValueDef, isAutoIncrement, generatedColumnExpr.convertToColumnNameExpr(table.getIdToColumn()), comment); | |||
There was a problem hiding this comment.
why using column name here?
| sb.append(Joiner.on(", ").join(partitionExprs.stream().map(Expr::toSql).collect(toList()))); | ||
| sb.append(Joiner.on(", ").join(partitionExprs | ||
| .stream() | ||
| .map(physicalExpr -> physicalExpr.convertToColumnNameExpr(table.getIdToColumn()).toSql()) |
There was a problem hiding this comment.
Suggested change
| .map(physicalExpr -> physicalExpr.convertToColumnNameExpr(table.getIdToColumn()).toSql()) | |
| .map(columnIdExpr -> columnIdExpr.convertToColumnNameExpr(table.getIdToColumn()).toSql()) |
gengjun-git
force-pushed
the
rename_col_sql_expr
branch
from
b70efd3
to
ddd16e3
Compare
gengjun-git
force-pushed
the
rename_col_sql_expr
branch
from
4f4c67c
to
f648759
Compare
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]❌ fail : 183 / 301 (60.80%) file detail
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why I'm doing:
We need a unique ID to identify the Column. This ID is used in all places that reference the Column. In this way, to change the attributes of the Column, such as name, we only need to change the attributes in the Column object. There is a uniqueId in the current Column, but it is only available in newly created tables. For compatibility reasons, we need to introduce another Id: ColumnID. The ColumnID of the historical table is the name of the column, because the name was previously immutable.
What I'm doing:
There are currently three ways to reference Column: 1: direct copy of Column object, 2: reference to Column name, 3: sql expression reference.
This PR changes the sql expression reference to use ColumnId reference.
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: