[Feature] Support drop partitions with common expressions #53118
+772
−60
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
Signed-off-by: shuming.li <[email protected]>
| } catch (Exception e) { | ||
| throw new SemanticException("Failed to prune partitions with where expression: " + e.getMessage()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The most risky bug in this code is:
In the putValueMapItem method, there is a risk of a concurrent modification exception because the partitionIdSet, once retrieved from the map, can be modified outside of the synchronization context before it is put back.
You can modify the code like this:
private static void putValueMapItem(ConcurrentNavigableMap<LiteralExpr, Set<Long>> partitionValueToIds,
Long partitionId,
LiteralExpr value) {
partitionValueToIds.compute(value, (k, partitionIdSet) -> {
if (partitionIdSet == null) {
partitionIdSet = new HashSet<>();
}
partitionIdSet.add(partitionId);
return partitionIdSet;
});
}This change uses compute which safely adds partitionId to the set associated with value without risking concurrent modification issues.
| } | ||
|
|
||
| public Expr getDropWhereExpr() { | ||
| return dropWhereExpr; | ||
| } | ||
|
|
||
| public List<String> getResolvedPartitionNames() { |
There was a problem hiding this comment.
The most risky bug in this code is:
Potential null pointer dereference due to assumptions about partition lists.
You can modify the code like this:
// Before accessing any fields that may be null, check for null references.
public DropPartitionClause(boolean ifExists, Expr whereExpr, boolean isTempPartition,
boolean forceDrop, NodePosition pos) {
super(AlterOpType.DROP_PARTITION, pos);
this.ifExists = ifExists;
this.partitionName = null;
this.isTempPartition = isTempPartition;
this.forceDrop = forceDrop;
this.multiRangePartitionDesc = null;
this.partitionNames = new ArrayList<>(); // Ensure partitionNames is initialized to prevent null access.
this.dropWhereExpr = whereExpr;
}Make sure the partitionNames or any potentially nullable field is either checked before use or given a safe default (like an empty list). This helps avoid null pointer exceptions when the object is used elsewhere in the system.
| .getDb(context.getCurrentCatalog(), context.getDatabase()); | ||
| TableName tableName = new TableName(db.getFullName(), table.getName()); | ||
| List<String> dropPartitionNames = ListPartitionPruner.getPartitionNamesByExpr(olapTable, tableName, expr, context); | ||
| clause.setResolvedPartitionNames(dropPartitionNames); | ||
| } | ||
|
|
||
| if (table instanceof OlapTable) { |
There was a problem hiding this comment.
The most risky bug in this code is:
Incorrect conditions in if-else statements that can lead to semantic exceptions for unsupported table types.
You can modify the code like this:
@Override
public Void visitDropPartitionClause(DropPartitionClause clause, ConnectContext context) {
if (clause.getMultiRangePartitionDesc() != null) {
if (!(table instanceof OlapTable)) {
throw new SemanticException("Can't drop partitions with multi-range since its table type is not olap");
}
MultiRangePartitionDesc multiRangePartitionDesc = clause.getMultiRangePartitionDesc();
PartitionDescAnalyzer.analyze(multiRangePartitionDesc);
OlapTable olapTable = (OlapTable) table;
PartitionDescAnalyzer.analyzePartitionDescWithExistsTable(multiRangePartitionDesc, olapTable);
PartitionInfo partitionInfo = olapTable.getPartitionInfo();
List<String> dropPartitionNames = multiRangePartitionDesc.convertToSingleRangePartitions(partitionInfo);
clause.setResolvedPartitionNames(dropPartitionNames);
} else if (clause.getPartitionNames() != null) {
clause.setResolvedPartitionNames(clause.getPartitionNames());
} else if (clause.getDropWhereExpr() != null) {
if (!(table instanceof OlapTable)) {
throw new SemanticException("Can't drop partitions with where expression since its table type is not olap");
}
OlapTable olapTable = (OlapTable) table;
Expr expr = clause.getDropWhereExpr();
Database db = context.getGlobalStateMgr().getMetadataMgr()
.getDb(context.getCurrentCatalog(), context.getDatabase());
TableName tableName = new TableName(db.getFullName(), table.getName());
List<String> dropPartitionNames = ListPartitionPruner.getPartitionNamesByExpr(olapTable, tableName, expr, context);
clause.setResolvedPartitionNames(dropPartitionNames);
}
if (table instanceof OlapTable) {
// any additional logic for OlapTable
}
}Explanation:
- Placement and order of
MultiRangePartitionDesc multiRangePartitionDescinitialization: In the original code, there's a risk because initialization should happen only after confirming the table type asOlapTable. - Ensuring the
ifconditions align logically with their use case to prevent invalid operations on non-Olap tables. - It ensures that both types of partition dropping verifications are correctly placed and don't leak erroneous behaviors on different table types.
Signed-off-by: shuming.li <[email protected]>
a0e136e
to
3ade038
Compare
|
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 141 / 148 (95.27%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
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:
What I'm doing:
Fixes #53117
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: