PHILO-HE commented on code in PR #5626:
URL: https://github.com/apache/incubator-gluten/pull/5626#discussion_r1619648470
##########
gluten-core/src/main/java/org/apache/gluten/substrait/expression/WindowFunctionNode.java:
##########
@@ -80,20 +92,53 @@ private Expression.WindowFunction.Bound.Builder setBound(
builder.setUnboundedFollowing(followingBuilder.build());
break;
default:
- try {
- Long offset = Long.valueOf(boundType);
+ if (boundType instanceof PreComputeRangeFrameBound) {
+ // Used only when backend is velox and frame type is RANGE.
+ if (frameType != "RANGE") {
+ throw new GlutenException(
+ "Only Range frame supports PreComputeRangeFrameBound, but got
" + frameType);
+ }
+ ExpressionNode refNode =
+ ExpressionConverter.replaceWithExpressionTransformer(
+ ((PreComputeRangeFrameBound)
boundType).child().toAttribute(),
+
JavaConverters.asScalaIteratorConverter(originalInputAttributes.iterator())
+ .asScala()
+ .toSeq())
+ .doTransform(new HashMap<String, Long>());
+ Long offset = Long.valueOf(boundType.eval(null).toString());
if (offset < 0) {
- Expression.WindowFunction.Bound.Preceding.Builder
offsetPrecedingBuilder =
+ Expression.WindowFunction.Bound.Preceding.Builder
refPrecedingBuilder =
Expression.WindowFunction.Bound.Preceding.newBuilder();
- offsetPrecedingBuilder.setOffset(0 - offset);
- builder.setPreceding(offsetPrecedingBuilder.build());
+ refPrecedingBuilder.setRef(refNode.toProtobuf());
+ builder.setPreceding(refPrecedingBuilder.build());
} else {
- Expression.WindowFunction.Bound.Following.Builder
offsetFollowingBuilder =
+ Expression.WindowFunction.Bound.Following.Builder
refFollowingBuilder =
Expression.WindowFunction.Bound.Following.newBuilder();
- offsetFollowingBuilder.setOffset(offset);
- builder.setFollowing(offsetFollowingBuilder.build());
+ refFollowingBuilder.setRef(refNode.toProtobuf());
+ builder.setFollowing(refFollowingBuilder.build());
+ }
+ } else if (boundType.foldable()) {
+ // Used when
+ // 1. Velox backend and frame type is ROW
+ // 2. Clickhouse backend
+ try {
+ Long offset = Long.valueOf(boundType.eval(null).toString());
+ if (offset < 0) {
+ Expression.WindowFunction.Bound.Preceding.Builder
offsetPrecedingBuilder =
+ Expression.WindowFunction.Bound.Preceding.newBuilder();
+ offsetPrecedingBuilder.setOffset(0 - offset);
+ builder.setPreceding(offsetPrecedingBuilder.build());
+ } else {
+ Expression.WindowFunction.Bound.Following.Builder
offsetFollowingBuilder =
+ Expression.WindowFunction.Bound.Following.newBuilder();
+ offsetFollowingBuilder.setOffset(offset);
+ builder.setFollowing(offsetFollowingBuilder.build());
+ }
+ } catch (NumberFormatException e) {
Review Comment:
Looks we can also remove this try/catch.
##########
gluten-core/src/main/java/org/apache/gluten/substrait/expression/WindowFunctionNode.java:
##########
@@ -80,20 +92,53 @@ private Expression.WindowFunction.Bound.Builder setBound(
builder.setUnboundedFollowing(followingBuilder.build());
break;
default:
- try {
- Long offset = Long.valueOf(boundType);
+ if (boundType instanceof PreComputeRangeFrameBound) {
+ // Used only when backend is velox and frame type is RANGE.
+ if (frameType != "RANGE") {
+ throw new GlutenException(
+ "Only Range frame supports PreComputeRangeFrameBound, but got
" + frameType);
+ }
+ ExpressionNode refNode =
+ ExpressionConverter.replaceWithExpressionTransformer(
+ ((PreComputeRangeFrameBound)
boundType).child().toAttribute(),
+
JavaConverters.asScalaIteratorConverter(originalInputAttributes.iterator())
+ .asScala()
+ .toSeq())
+ .doTransform(new HashMap<String, Long>());
+ Long offset = Long.valueOf(boundType.eval(null).toString());
if (offset < 0) {
- Expression.WindowFunction.Bound.Preceding.Builder
offsetPrecedingBuilder =
+ Expression.WindowFunction.Bound.Preceding.Builder
refPrecedingBuilder =
Expression.WindowFunction.Bound.Preceding.newBuilder();
- offsetPrecedingBuilder.setOffset(0 - offset);
- builder.setPreceding(offsetPrecedingBuilder.build());
+ refPrecedingBuilder.setRef(refNode.toProtobuf());
+ builder.setPreceding(refPrecedingBuilder.build());
} else {
- Expression.WindowFunction.Bound.Following.Builder
offsetFollowingBuilder =
+ Expression.WindowFunction.Bound.Following.Builder
refFollowingBuilder =
Expression.WindowFunction.Bound.Following.newBuilder();
- offsetFollowingBuilder.setOffset(offset);
- builder.setFollowing(offsetFollowingBuilder.build());
+ refFollowingBuilder.setRef(refNode.toProtobuf());
+ builder.setFollowing(refFollowingBuilder.build());
+ }
+ } else if (boundType.foldable()) {
+ // Used when
+ // 1. Velox backend and frame type is ROW
+ // 2. Clickhouse backend
+ try {
+ Long offset = Long.valueOf(boundType.eval(null).toString());
+ if (offset < 0) {
+ Expression.WindowFunction.Bound.Preceding.Builder
offsetPrecedingBuilder =
+ Expression.WindowFunction.Bound.Preceding.newBuilder();
+ offsetPrecedingBuilder.setOffset(0 - offset);
+ builder.setPreceding(offsetPrecedingBuilder.build());
+ } else {
+ Expression.WindowFunction.Bound.Following.Builder
offsetFollowingBuilder =
+ Expression.WindowFunction.Bound.Following.newBuilder();
+ offsetFollowingBuilder.setOffset(offset);
+ builder.setFollowing(offsetFollowingBuilder.build());
+ }
+ } catch (NumberFormatException e) {
+ throw new UnsupportedOperationException(
+ "Unsupported Window Function Frame Type:" + boundType);
}
- } catch (NumberFormatException e) {
+ } else {
throw new UnsupportedOperationException(
"Unsupported Window Function Frame Type:" + boundType);
Review Comment:
Not frame type, but bound type, please help fix it in your pr.
```
"Unsupported Window Function Frame Bound Type: " + boundType
```
--
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]
