Copilot commented on code in PR #59017:
URL: https://github.com/apache/doris/pull/59017#discussion_r2618312309
##########
fe/fe-core/src/main/antlr4/org/apache/doris/nereids/DorisParser.g4:
##########
@@ -1512,7 +1512,7 @@ rollupDef
;
aggTypeDef
- : MAX | MIN | SUM | REPLACE | REPLACE_IF_NOT_NULL | HLL_UNION |
BITMAP_UNION | QUANTILE_UNION | GENERIC
+ : MAX | MIN | SUM | REPLACE | REPLACE_IF_NOT_NULL | HLL_UNION |
BITMAP_UNION | QUANTILE_UNION | GENERIC | FIRST
Review Comment:
There is trailing whitespace at the end of line 1515 after "FIRST". This
should be removed for code cleanliness.
```suggestion
: MAX | MIN | SUM | REPLACE | REPLACE_IF_NOT_NULL | HLL_UNION |
BITMAP_UNION | QUANTILE_UNION | GENERIC | FIRST
```
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -347,7 +347,8 @@ private boolean processDropColumn(DropColumnOp
dropColumnOp, OlapTable olapTable
isKey = true;
lightSchemaChange = false;
} else if (AggregateType.REPLACE ==
column.getAggregationType()
- || AggregateType.REPLACE_IF_NOT_NULL ==
column.getAggregationType()) {
+ || AggregateType.REPLACE_IF_NOT_NULL ==
column.getAggregationType()
+ || AggregateType.FIRST ==
column.getAggregationType()) {
Review Comment:
Consider using the `isReplaceFamily()` method instead of explicitly checking
each aggregate type. This would make the code more maintainable and consistent
with how the replace family is identified elsewhere. Replace the condition
with: `column.getAggregationType().isReplaceFamily()`
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -366,7 +367,8 @@ private boolean processDropColumn(DropColumnOp
dropColumnOp, OlapTable olapTable
isKey = true;
lightSchemaChange = false;
} else if (AggregateType.REPLACE ==
column.getAggregationType()
- || AggregateType.REPLACE_IF_NOT_NULL ==
column.getAggregationType()) {
+ || AggregateType.REPLACE_IF_NOT_NULL ==
column.getAggregationType()
+ || AggregateType.FIRST ==
column.getAggregationType()) {
Review Comment:
Consider using the `isReplaceFamily()` method instead of explicitly checking
each aggregate type. This would make the code more maintainable and consistent
with how the replace family is identified elsewhere. Replace the condition
with: `column.getAggregationType().isReplaceFamily()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
