morrySnow commented on code in PR #60091:
URL: https://github.com/apache/doris/pull/60091#discussion_r2735108706
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/algebra/Repeat.java:
##########
@@ -140,8 +162,8 @@ default GroupingSetShapes toShapes() {
*
* return: [(4, 3), (3)]
*/
- default List<Set<Integer>> computeRepeatSlotIdList(List<Integer>
slotIdList) {
- List<Set<Integer>> groupingSetsIndexesInOutput =
getGroupingSetsIndexesInOutput();
+ default List<Set<Integer>> computeRepeatSlotIdList(List<Integer>
slotIdList, List<Slot> outputSlots) {
Review Comment:
add a ut for this function
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalRepeat.java:
##########
@@ -140,6 +140,11 @@ public List<NamedExpression>
getGroupingScalarFunctionAlias() {
return functionList;
}
+ public Set<GroupingScalarFunction> getGroupingScalarFunctions() {
+ return new HashSet<>(
Review Comment:
return immutable set
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/algebra/Repeat.java:
##########
@@ -160,8 +182,8 @@ default List<Set<Integer>>
computeRepeatSlotIdList(List<Integer> slotIdList) {
* e.g. groupingSets=((b, a), (a)), output=[a, b]
* return ((1, 0), (1))
*/
- default List<Set<Integer>> getGroupingSetsIndexesInOutput() {
- Map<Expression, Integer> indexMap = indexesOfOutput();
+ default List<Set<Integer>> getGroupingSetsIndexesInOutput(List<Slot>
outputSlots) {
Review Comment:
add a ut for this function
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/algebra/Repeat.java:
##########
@@ -117,18 +117,40 @@ default List<List<Long>> computeGroupingFunctionsValues()
{
/**
* flatten the grouping sets and build to a GroupingSetShapes.
+ * This method ensures that all expressions referenced by grouping
functions are included
+ * in the flattenGroupingSetExpression, even if they are not in any
grouping set.
+ * This is necessary for optimization scenarios where some expressions may
only exist
+ * in the maximum grouping set that was removed during optimization.
*/
default GroupingSetShapes toShapes() {
- Set<Expression> flattenGroupingSet =
ImmutableSet.copyOf(ExpressionUtils.flatExpressions(getGroupingSets()));
+ Set<Expression> flattenGroupingSet =
ImmutableSet.copyOf(getGroupByExpressions());
Review Comment:
if flattenGroupingSet only used in L138 to construct allExpresssions, why
copy twice in this function?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/algebra/Repeat.java:
##########
@@ -117,18 +117,40 @@ default List<List<Long>> computeGroupingFunctionsValues()
{
/**
* flatten the grouping sets and build to a GroupingSetShapes.
+ * This method ensures that all expressions referenced by grouping
functions are included
+ * in the flattenGroupingSetExpression, even if they are not in any
grouping set.
+ * This is necessary for optimization scenarios where some expressions may
only exist
+ * in the maximum grouping set that was removed during optimization.
*/
default GroupingSetShapes toShapes() {
Review Comment:
add a ut for this function
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/algebra/Repeat.java:
##########
@@ -192,15 +214,11 @@ default List<Set<Integer>>
getGroupingSetsIndexesInOutput() {
* `c`: 2
* )
*/
- default Map<Expression, Integer> indexesOfOutput() {
+ default Map<Expression, Integer> indexesOfOutput(List<Slot> outputSlots) {
Review Comment:
should add comment to explain why need pass outputSlots, not use
outputExpression
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/algebra/Repeat.java:
##########
@@ -192,15 +214,11 @@ default List<Set<Integer>>
getGroupingSetsIndexesInOutput() {
* `c`: 2
* )
*/
- default Map<Expression, Integer> indexesOfOutput() {
+ default Map<Expression, Integer> indexesOfOutput(List<Slot> outputSlots) {
Review Comment:
add a ut for this function
--
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]
