Copilot commented on code in PR #13202:
URL: https://github.com/apache/gluten/pull/13202#discussion_r4186379948
##########
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));
+ try {
+ return Long.parseLong(raw);
+ } catch (NumberFormatException e) {
+ throw new GlutenNotSupportException(
+ "Window frame bound is not a long offset: "
+ + raw
+ + " ("
+ + boundType.dataType().simpleString()
+ + ")");
+ }
Review Comment:
`parseOffset` relies on `eval(null)` + string parsing to decide support.
This can silently accept unsupported non-integral *types* when their string
happens to parse as a long (e.g., a `decimal(10,0)` bound may stringify as
`"10"`), which is particularly relevant for backends like ClickHouse that don’t
validate bound types in `supportWindowExec`. Consider explicitly rejecting any
non-integral bound *data type* (byte/short/int/long) before attempting
conversion, and convert via the evaluated value’s numeric type (instead of
string parsing) to avoid formatting-dependent behavior.
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxBackend.scala:
##########
@@ -448,6 +448,13 @@ 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(
+ "Only integral literal bounds are supported for a
RANGE frame, " +
+ s"got ${other.simpleString}")
Review Comment:
The new error message only reports the bound data type, but not the
offending bound value/expression. Including the literal’s SQL/value (e.g., via
`e.sql`/`e.toString`) would make debugging much easier when users hit fallback
rejections in production.
--
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]
