-
Notifications
You must be signed in to change notification settings - Fork 15
Advance velox version #69
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Wu Xueyang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take a look about agg failure later. Can you try to make CI passed?
cpp/src/types/PrestoToVeloxExpr.cpp
Outdated
@@ -381,23 +381,23 @@ TypedExprPtr VeloxExprConverter::toVeloxExpr( | |||
auto returnType = parseTypeSignature(pexpr.returnType); | |||
if (getFunctionName(signature) == "presto.default.avg" || | |||
getFunctionName(signature) == "presto.default.sum") { | |||
VLOG(1) << "if"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this.
cpp/src/types/PrestoToVeloxExpr.cpp
Outdated
} else if (auto sqlFunctionHandle = | ||
std::dynamic_pointer_cast<protocol::SqlFunctionHandle>( | ||
pexpr.functionHandle)) { | ||
VLOG(1) << "else"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this.
UT of agg node cannot pass currently.. It's relative to the issue mentioned above. According to the newest velox, functionHandle and arguments should be added in aggregations elements, I don't know which query @bigPYJ1151 ran to produce the JSON, maybe we should rerun it. |
I used TPCH Q1 and picked AggregationNode only. |
note: still lead to error introduced by facebookincubator/velox#7018 and I've tried my best to deal with it but failed. @jikunshang
That will fail all aggregate functions, like
Maybe it comes with difference between Presto and Trino.