Copilot commented on code in PR #13202:
URL: https://github.com/apache/gluten/pull/13202#discussion_r4186042523
##########
gluten-substrait/src/main/scala/org/apache/gluten/utils/PullOutProjectHelper.scala:
##########
@@ -209,16 +207,26 @@ trait PullOutProjectHelper {
(needPreComputeRangeFrameBoundary(swf.lower) ||
needPreComputeRangeFrameBoundary(swf.upper))
}
- protected def supportPreComputeRangeFrame(sortOrders: Seq[SortOrder]):
Boolean = {
- sortOrders.forall {
- _.dataType match {
- case ByteType | ShortType | IntegerType | LongType | DateType => true
- // Only integral type & date type are supported for sort key with
Range Frame
- case _ => false
- }
- }
+ // The pre-computed boundary is the order key plus the bound, so the bound
has to be an integral
+ // value too. A date key with an interval bound, for one, stays as it is and
falls back.
+ private def canPreComputeRangeFrame(
+ swf: SpecifiedWindowFrame,
+ sortOrders: Seq[SortOrder]): Boolean = {
+ def integralBound(bound: Expression): Boolean =
+ !needPreComputeRangeFrameBoundary(bound) ||
isIntegralType(bound.dataType)
Review Comment:
The comment says the bound must be an \"integral value\", but the
implementation enforces an integral *data type*
(`isIntegralType(bound.dataType)`). If the intent is type-based (as the new
backend checks imply), update the comment to avoid implying value-based
acceptance. If the intent is truly value-based, adjust `integralBound` to
accept foldable decimals/doubles whose fractional part is zero (while still
rejecting intervals).
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxBackend.scala:
##########
@@ -448,6 +448,12 @@ object VeloxBackendSettings extends BackendSettingsApi {
"Only integral type & date type are" +
" supported for sort key when literal bound type
is used!")
})
+ e.dataType match {
+ case ByteType | ShortType | IntegerType | LongType =>
+ case other =>
+ throw new GlutenNotSupportException(
+ s"Only integral literal bounds are supported for a
RANGE frame, got $other")
Review Comment:
The error message relies on `DataType.toString` via `$other`, which can be
verbose/inconsistent across Spark versions. Consider using `other.simpleString`
for a more stable message, and (since `e` is foldable here) optionally include
the literal value to speed up diagnosis of fallbacks.
##########
backends-bolt/src/main/scala/org/apache/gluten/backendsapi/bolt/BoltBackend.scala:
##########
@@ -462,6 +462,12 @@ object BoltBackendSettings extends BackendSettingsApi {
"Only integral type & date type are" +
" supported for sort key when literal bound type
is used!")
})
+ e.dataType match {
+ case ByteType | ShortType | IntegerType | LongType =>
+ case other =>
+ throw new GlutenNotSupportException(
+ s"Only integral literal bounds are supported for a
RANGE frame, got $other")
Review Comment:
Same as the Velox backend: prefer `other.simpleString` (and optionally the
evaluated literal value) over `$other` to keep fallback reasons clearer and
more consistent in logs.
##########
gluten-substrait/src/main/java/org/apache/gluten/substrait/expression/WindowFunctionNode.java:
##########
@@ -72,6 +73,23 @@ public class WindowFunctionNode implements Serializable {
this.originalInputAttributes = originalInputAttributes;
}
+ // A bound that does not parse as a long, such as the -1.50 of RANGE BETWEEN
1.5 PRECEDING on a
+ // decimal key, throws GlutenNotSupportException so the window falls back; a
+ // NumberFormatException would fail the query instead.
+ private static Long
parseOffset(org.apache.spark.sql.catalyst.expressions.Expression boundType) {
+ String raw = String.valueOf(boundType.eval(null));
Review Comment:
`parseOffset` takes an expression, but the parameter name `boundType` reads
like a data type rather than a bound expression. Renaming it to something like
`boundExpr` (and updating usages) would reduce ambiguity when reading stack
traces and code paths involving bounds vs. types.
--
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]
