[CORE] Fix fallback for spark sequence function with literal array data as input #6433
Conversation
|
Thanks for opening a pull request! Could you open an issue for this pull request on Github Issues? https://github.com/apache/incubator-gluten/issues Then could you also rename commit message and pull request title in the following format? See also: |
|
Run Gluten Clickhouse CI |
…gayangya/fix_sequence_fallback_issue
|
Run Gluten Clickhouse CI |
1 similar comment
|
Run Gluten Clickhouse CI |
gaoyangxiaozhu
changed the title
gaoyangxiaozhu
force-pushed
the
gayangya/fix_sequence_fallback_issue
branch
from
93fc653 to
373c10b
Compare
|
Run Gluten Clickhouse CI |
| @@ -664,6 +665,15 @@ class ScalarFunctionsValidateSuite extends FunctionsValidateTest { | |||
| } | |||
| } | |||
|
|
|||
| test("Test sequence function") { | |||
| withSQLConf(("spark.sql.optimizer.excludedRules", NullPropagation.ruleName)) { | |||
There was a problem hiding this comment.
Curious why this rule matters, instead of constant folding rule. Could you clarify?
There was a problem hiding this comment.
hey good question @PHILO-HE , actually the matter is constant folding rule which should be removed from the excludedRules, so here is only use to override and remove it from default test sql conf and keep others https://github.com/apache/incubator-gluten/blob/main/backends-velox/src/test/scala/org/apache/gluten/execution/FunctionsValidateTest.scala#L44
There was a problem hiding this comment.
With constant folding rule applied, will the physical plan delivered to Gluten become an Alias plan (the result is actually computed by Spark, and Gluten just offloads the Alias plan)? Could you please verify?
There was a problem hiding this comment.
hey @PHILO-HE, maybe you mis-understand here, I just removed constant folding rule from excludedRules to make sure the rule can take effect (the default spark behavior), so the result of the UT is computed actually by spark and been put into unsafeArray with Alias Literal type to offload to gluten.
if put constant folding rule in excludedRules, it will try offload sequence to velox and fallback due to unsupporetd function name.
There was a problem hiding this comment.
@gaoyangxiaozhu, I see. I think it would be better to revise test name to the one like: "sequence function optimized by Spark constant folding"
|
@PHILO-HE could you help merge ? |
@gaoyangxiaozhu, do you think this small change suggestion makes sense? |
| @@ -215,6 +217,19 @@ public static LiteralNode makeLiteral(Object obj, TypeNode typeNode) { | |||
|
|
|||
| public static LiteralNode makeLiteral(Object obj, DataType dataType, Boolean nullable) { | |||
| TypeNode typeNode = ConverterUtils.getTypeNode(dataType, nullable); | |||
| if (obj instanceof UnsafeArrayData) { | |||
There was a problem hiding this comment.
@gaoyangxiaozhu, can we just overload makeLiteral with UnsafeArrayData type as input?
There was a problem hiding this comment.
em.. actually not sure, unless we sure the literal list type always be UnSafeArrayData, but i am not sure right now.
Let's keep current change , and let me do follow up to check spark code to see if can always move to UnSafeArrayData
updated |
|
Run Gluten Clickhouse CI |
|
@gaoyangxiaozhu, please also check this comment: https://github.com/apache/incubator-gluten/pull/6433/files#r1678722614 |
hey @PHILO-HE , please check my above reply. :) |
PHILO-HE
changed the title
What changes were proposed in this pull request?
(Please fill in changes proposed in this fix)
Before this PR the below query would fail and fallback
with error
(Fixes: #ISSUE-ID)
How was this patch tested?
UT
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)