[GLUTEN-5625][VL] Support window range frame #5626
Conversation
|
Run Gluten Clickhouse CI |
5 similar comments
|
Run Gluten Clickhouse CI |
|
Run Gluten Clickhouse CI |
|
Run Gluten Clickhouse CI |
|
Run Gluten Clickhouse CI |
|
Run Gluten Clickhouse CI |
WangGuangxin
force-pushed
the
feat_window_range_frame
branch
from
f5d363b
to
0484902
Compare
|
Run Gluten Clickhouse CI |
|
@PHILO-HE is the PR offload SQL like below? |
|
Let me do a test once merged. Thank you! |
|
@WangGuangxin can you help to resolve conflicts? |
WangGuangxin
force-pushed
the
feat_window_range_frame
branch
from
0484902
to
7d570fb
Compare
|
Run Gluten Clickhouse CI |
@FelixYBW done |
| return std::make_tuple( | ||
| core::WindowNode::BoundType::kFollowing, | ||
| exprConverter_->toVeloxExpr(boundType.following().ref(), inputType)); | ||
| } |
There was a problem hiding this comment.
It seems the only difference of the two branches is the expression. Can we differentiate the expressions only and reuse the rest of code?
| return std::make_tuple( | ||
| core::WindowNode::BoundType::kPreceding, | ||
| exprConverter_->toVeloxExpr(boundType.preceding().ref(), inputType)); | ||
| } |
There was a problem hiding this comment.
done.
I tried to reuse the logic here, but since the type of boundType.preceding() and boundType.following() are different, so I have to pass the offset and ref instead of the expression itself
| String upperBound, | ||
| String lowerBound, | ||
| org.apache.spark.sql.catalyst.expressions.Expression upperBound, | ||
| org.apache.spark.sql.catalyst.expressions.Expression lowerBound, |
There was a problem hiding this comment.
Can we import org.apache.spark.sql.catalyst.expressions.Expression and use it here?
There was a problem hiding this comment.
The Expression is conflict with io.substrait.proto.Expression which alreay imported in this class, and Java doen't have the alias grammer like scala
| if (offset < 0) { | ||
| Expression.WindowFunction.Bound.Preceding.Builder offsetPrecedingBuilder = | ||
| Expression.WindowFunction.Bound.Preceding.newBuilder(); | ||
| offsetPrecedingBuilder.setOffset(0 - offset); |
There was a problem hiding this comment.
No need. In fact, this branch is the existing logic
|
|
||
| // the reference to pre-project range frame boundary. | ||
| Expression ref = 2; | ||
| } |
There was a problem hiding this comment.
Would you document this change to Substrait in https://github.com/apache/incubator-gluten/blob/main/docs/developers/SubstraitModifications.md?
WangGuangxin
force-pushed
the
feat_window_range_frame
branch
from
7d570fb
to
d5195af
Compare
|
Run Gluten Clickhouse CI |
|
Run Gluten Clickhouse CI |
|
Run Gluten Clickhouse CI |
What changes were proposed in this pull request?
Support window range frame by pre-project frame offset for non-SpecialFrameBoundary, based on the discussion facebookincubator/velox#7557
(Fixes: #5625)
How was this patch tested?
UT