Skip to content

[GLUTEN-9178][CH] Fix cse in aggregate operator not working#9301

Merged
taiyang-li merged 2 commits intoapache:mainfrom
loneylee:9178
Apr 14, 2025
Merged

[GLUTEN-9178][CH] Fix cse in aggregate operator not working#9301
taiyang-li merged 2 commits intoapache:mainfrom
loneylee:9178

Conversation

@loneylee
Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

(Please fill in changes proposed in this fix)

(Fixes: #9178)

How was this patch tested?

Test by ut

@github-actions
Copy link
Copy Markdown

#9178

@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

@loneylee loneylee requested a review from Copilot April 12, 2025 13:08
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

Files not reviewed (2)
  • backends-clickhouse/src/main/scala/org/apache/gluten/extension/CommonSubexpressionEliminateRule.scala: Language not supported
  • backends-clickhouse/src/test/scala/org/apache/gluten/execution/GlutenFunctionValidateSuite.scala: Language not supported

@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

@loneylee
Copy link
Copy Markdown
Member Author

@CodiumAI-Agent /improve

@QodoAI-Agent
Copy link
Copy Markdown

QodoAI-Agent commented Apr 12, 2025

PR Code Suggestions ✨

Latest suggestions up to 3f9799e

CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate children list length

Before accessing the first element of aggFunc.children, ensure the list is non-empty
to avoid runtime exceptions.

backends-clickhouse/src/test/scala/org/apache/gluten/execution/GlutenFunctionValidateSuite.scala [841-843]

 .map(expr => expr.asInstanceOf[AggregateExpression].aggregateFunction)
-.filter(aggFunc => aggFunc.children.head.isInstanceOf[AttributeReference])
+.filter(aggFunc => aggFunc.children.nonEmpty && aggFunc.children.head.isInstanceOf[AttributeReference])
 .map(aggFunc => aggFunc.children.head.asInstanceOf[AttributeReference].name)
Suggestion importance[1-10]: 6

__

Why: The suggestion improves safety by checking that aggFunc.children is non-empty before accessing head, which can prevent potential runtime exceptions; however, its impact is moderate as it only adds a defensive check in a specific context.

Low

Previous suggestions

Suggestions up to commit 3f9799e
CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard head access safely

Safely access the first child by checking its existence to avoid potential
exceptions when the children list is empty.

backends-clickhouse/src/test/scala/org/apache/gluten/execution/GlutenFunctionValidateSuite.scala [844]

-.map(aggFunc => aggFunc.children.head.asInstanceOf[AttributeReference].name)
+.flatMap { aggFunc =>
+  aggFunc.children.headOption match {
+    case Some(attr: AttributeReference) => Some(attr.name)
+    case _ => None
+  }
+}
Suggestion importance[1-10]: 5

__

Why: While adding a safety check for head access can prevent potential runtime exceptions, the context already filters for valid AttributeReference instances, making the improvement only moderately beneficial.

Low

@loneylee loneylee requested a review from taiyang-li April 14, 2025 01:27
@loneylee
Copy link
Copy Markdown
Member Author

@taiyang-li cc

Copy link
Copy Markdown
Contributor

@taiyang-li taiyang-li left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@taiyang-li taiyang-li merged commit d93128c into apache:main Apr 14, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CH] CSE in aggregate operator not working

4 participants