JkSelf commented on PR #12009:
URL: https://github.com/apache/gluten/pull/12009#issuecomment-4705956509
@jianzhenwu Thanks for your fix.
1. The scope of this fix seems to be the Expand type alignment issue on the
Velox backend. Given that both the problem statement and the added validation
focus on Velox, do we really need to apply `AlignExpandOutputTypes` to the CH
backend as well?
2. The current fix feels more complicated than necessary. This PR adds a
rewrite rule to align projection types with Expand output types, but then also
performs another mismatch check inside `ExpandExecTransformer`. For the Velox
path, `ExpandExecTransformer` is created from the rewritten `ExpandExec`, so if
the rewrite already guarantees type alignment, the second check looks redundant.
3. The core of the fix seems quite simple: check whether each projection
type matches the corresponding output type, and if not, convert it to the
expected type. However, the current implementation introduces several helper
layers and special-case checks, which makes the code harder to follow and the
intent less clear. I wonder if we can simplify this into a more direct
implementation with better readability as below.
```
for each projectionRow in projections:
for each column i:
val expr = projectionRow(i)
val targetType = output(i).dataType
if expr is null literal:
projectionRow(i) = typedNull(targetType)
else if expr.dataType != targetType:
projectionRow(i) = Cast(expr, targetType)
```
--
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]
