-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BugFix] Fix union isnullable error #32426
Conversation
Expr childExpr = ScalarOperatorToExpr.buildExecExpression(childRef, formatterContext); | ||
isNullable |= childExpr.isNullable(); | ||
} | ||
slotDesc.setIsNullable(isNullable); | ||
setOutputList.add(new SlotRef(String.valueOf(columnRefOperator.getId()), slotDesc)); | ||
} | ||
setOperationTuple.computeMemLayout(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some improvement suggestions for the code patch:
Bug Risks:
- The code does not seem to have any evident bug risks.
Naming:
- Consider using more descriptive variable names instead of abbreviations like
optExpr
,optExpr
, andsetOutputList
.
Code Style:
- Utilize consistent indentation throughout the code.
- Avoid mixing different styles of iteration. Stick to one style, either using a foreach loop or an indexed loop.
Best Practices:
- Instead of using
Lists.newArrayList()
, consider using the diamond operator to initialize thesetOutputList
asnew ArrayList<>()
. - Extract the logic inside the
for
loop into a separate method to improve the code's readability and maintainability.
Compatibility:
- No specific compatibility concerns identified in the provided code snippet.
Simplicity:
- Consider splitting the functionality in the
for
loop into separate methods or functions to improve code modularity and simplify the logic.
Optimization Points:
- If performance is a concern, cache the size of
setOperation.getOutputColumnRefOp()
before the loop condition check to avoid repetitive method calls.
Overall, the code appears to be functional, but there is room for improvement in terms of code style, clarity, and naming conventions.
Signed-off-by: Seaven <[email protected]>
Expr childExpr = ScalarOperatorToExpr.buildExecExpression(childRef, formatterContext); | ||
isNullable |= childExpr.isNullable(); | ||
} | ||
slotDesc.setIsNullable(isNullable); | ||
setOutputList.add(new SlotRef(String.valueOf(columnRefOperator.getId()), slotDesc)); | ||
} | ||
setOperationTuple.computeMemLayout(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review Feedback:
- Bug Risk: The code appears to be fine from a bug risk perspective.
- Naming: The variable names are generally meaningful and descriptive.
- Code Style: The code follows indentation and formatting conventions.
- Best Practices: There are some opportunities for improvement in terms of best practices.
- Compatibility: The code doesn't seem to have any compatibility issues.
- Simplicity: The code could be simplified and made more readable.
- Optimization Points: Some performance optimization points can be addressed.
Code Improvement Suggestions:
-
Use enhanced for loop: Instead of using a traditional for loop with an index, you can use an enhanced for loop to iterate over the
setOperation.getOutputColumnRefOp()
list. This will simplify the loop and make it more readable.for (ColumnRefOperator columnRefOperator : setOperation.getOutputColumnRefOp()) { // Rest of the loop body }
-
Combine bitwise OR operations: In the line
boolean isNullable = slotDesc.getIsNullable() | setOperationNode.isHasNullableGenerateChild();
, you can combine the bitwise OR operation with the assignment in a single statement. This will make the code more concise.boolean isNullable = slotDesc.getIsNullable() || setOperationNode.isHasNullableGenerateChild();
-
Simplify nullable check inside the loop: Instead of calculating the
isNullable
value in a separate loop, you can move that logic inside the main loop by accessing the child output columns directly. This will eliminate the need for an additional loop.for (ColumnRefOperator columnRefOperator : setOperation.getOutputColumnRefOp()) { // Existing code // ... for (List<ColumnRefOperator> childOutputColumn : setOperation.getChildOutputColumns()) { ColumnRefOperator childRef = childOutputColumn.get(index); Expr childExpr = ScalarOperatorToExpr.buildExecExpression(childRef, formatterContext); isNullable |= childExpr.isNullable(); } // Existing code // ... }
-
Use StringBuilder for SlotId conversion: Instead of creating a new
String
object every time innew SlotRef(String.valueOf(columnRefOperator.getId()), slotDesc)
, you can use aStringBuilder
to improve efficiency.new SlotRef(new StringBuilder().append(columnRefOperator.getId()).toString(), slotDesc)
-
Optimize computeMemLayout(): Consider optimizing the
computeMemLayout()
call after adding elements tosetOutputList
. IfcomputeMemLayout()
is an expensive operation, you can delay the call until after the loop, reducing redundant calculations if applicable.
Note: These suggestions focus on the provided code patch and may not capture potential issues or optimizations in the larger context of the codebase.
Kudos, SonarCloud Quality Gate passed! |
[FE Incremental Coverage Report]😍 pass : 9 / 9 (100.00%) file detail
|
[BE Incremental Coverage Report]😍 pass : 0 / 0 (0%) |
@Mergifyio backport branch-3.2 |
✅ Backports have been created
|
Signed-off-by: Seaven <[email protected]> (cherry picked from commit c39da42)
Signed-off-by: Seaven <[email protected]> (cherry picked from commit c39da42)
@mergify backport branch-3.1 branch-3.0 branch-2.5 |
✅ Backports have been created
|
Signed-off-by: Seaven <[email protected]> (cherry picked from commit c39da42) # Conflicts: # fe/fe-core/src/test/java/com/starrocks/sql/plan/EmptyValueTest.java
Signed-off-by: Seaven <[email protected]> (cherry picked from commit c39da42) # Conflicts: # fe/fe-core/src/test/java/com/starrocks/sql/plan/EmptyValueTest.java
Signed-off-by: Seaven <[email protected]> (cherry picked from commit c39da42) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/sql/plan/PlanFragmentBuilder.java # fe/fe-core/src/test/java/com/starrocks/sql/plan/EmptyValueTest.java
Signed-off-by: Seaven <[email protected]>
Signed-off-by: Seaven <[email protected]>
Signed-off-by: Seaven <[email protected]>
Signed-off-by: Seaven <[email protected]>
Signed-off-by: Seaven <[email protected]>
Signed-off-by: Seaven <[email protected]>
Fixes #issue
For SQL:
t1
must empty table, the optimizer will trans SQL tothe
union
set output expr's isnullable errorWhat 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: