Copilot commented on code in PR #12821:
URL: https://github.com/apache/gluten/pull/12821#discussion_r3844366891
##########
cpp/velox/substrait/SubstraitToVeloxExpr.cc:
##########
@@ -651,7 +700,7 @@ core::TypedExprPtr SubstraitVeloxExprConverter::toVeloxExpr(
case ::substrait::Expression::RexTypeCase::kScalarFunction:
return toVeloxExpr(substraitExpr.scalar_function(), inputType);
case ::substrait::Expression::RexTypeCase::kSelection:
- return toVeloxExpr(substraitExpr.selection(), inputType);
+ return toVeloxOrdinalFieldReferenceExpr(substraitExpr.selection(),
inputType);
Review Comment:
This dispatch now returns a `DereferenceTypedExpr` for every nested
selection, but several existing consumers of the full `Expression` API still
require a `FieldAccessTypedExpr` (for example `processSortField`, window
partition/sort keys, and Generate's replicated/unnest fields). Those paths
previously received the nested `FieldAccessTypedExpr` chain and therefore
worked for nested references; they now reject the same valid plans even though
only aggregate masks are documented as intentionally unsupported. Please either
adapt these field-only operators to preserve ordinal nested access (for example
by projecting the dereference) or explicitly handle these cases before
conversion so they do not regress.
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -596,6 +596,7 @@ core::PlanNodePtr
SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
if (substraitAggMask.ByteSizeLong() > 0) {
mask = std::dynamic_pointer_cast<const core::FieldAccessTypedExpr>(
exprConverter_->toVeloxExpr(substraitAggMask, inputType));
+ VELOX_USER_CHECK_NOT_NULL(mask, "Aggregation Operator only supports a
top-level field mask.");
Review Comment:
This check is only reached while constructing the Velox plan, but
`SubstraitToVeloxPlanValidator::validate(AggregateRel)` currently accepts any
`selection` filter and never converts the mask or verifies that it is
top-level. Consequently a nested-mask plan is reported as supported by native
validation and only fails later at this `VELOX_USER_CHECK`, rather than being
rejected during the validation/fallback decision. Add the same top-level check
to validation (or validate the constructed mask there) so the support result
matches the converter.
##########
cpp/velox/substrait/SubstraitToVeloxExpr.cc:
##########
@@ -651,7 +700,7 @@ core::TypedExprPtr SubstraitVeloxExprConverter::toVeloxExpr(
case ::substrait::Expression::RexTypeCase::kScalarFunction:
return toVeloxExpr(substraitExpr.scalar_function(), inputType);
case ::substrait::Expression::RexTypeCase::kSelection:
- return toVeloxExpr(substraitExpr.selection(), inputType);
+ return toVeloxOrdinalFieldReferenceExpr(substraitExpr.selection(),
inputType);
Review Comment:
This ordinal path is only used when callers pass the whole `Expression`, but
`SubstraitToVeloxPlanConverter::toVeloxPlan(ExpandRel)` still passes
`projectExpr.selection()` for selection projections. The validator already
converts the full expression, so a nested duplicate/unnamed field can validate
successfully while the runtime Expand plan resolves it by name and selects the
wrong child. The Expand conversion should pass `projectExpr` itself, as
ProjectRel does.
--
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]
