[GLUTEN-3620][VL] Support Range operator for Velox Backend #8161
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 on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
| import org.apache.spark.sql.GlutenSQLTestsTrait | ||
| import org.apache.spark.sql.Row | ||
|
|
||
| class GlutenSQLRangeExecSuite extends GlutenSQLTestsTrait { |
There was a problem hiding this comment.
The suite would need to be enabled in VeloxTestSettings.
Yes, I'm planning to work on the offload implementation. Currently it seems unlikely we will have to introduce code in velox. Will raise PR once possible thanks. |
| * @param child | ||
| * Child SparkPlan nodes for this operator, if any. | ||
| */ | ||
| case class RangeExecTransformer( |
There was a problem hiding this comment.
Can we name it "ColumnarRangeExec" (or VeloxColumnarRangeExec or something) as it doesn't extend the TransformSupport trait? Thanks.
| override protected def doValidateInternal(): ValidationResult = { | ||
| val isSupported = BackendsApiManager.getSettings.supportRangeExec() | ||
|
|
||
| if (!isSupported) { | ||
| return ValidationResult.failed( | ||
| s"RangeExec is not supported by the current backend." | ||
| ) | ||
| } | ||
| ValidationResult.succeeded | ||
| } |
There was a problem hiding this comment.
I think usually we have to add some code to this validator to make doValidate be used. Can you help check this?
There was a problem hiding this comment.
I am opening #8177 that may simplify the usage of validators. Perhaps could do a rebase once that PR merged.
| current += numRows * step | ||
|
|
||
| val batch = new ColumnarBatch(vectors.asInstanceOf[Array[ColumnVector]], numRows) | ||
| val offloadedBatch = ColumnarBatches.offload(allocator, batch) |
There was a problem hiding this comment.
Because of the code, the operator's behaviour acutally align more precisely with
override def batchType(): Convention.BatchType = {
ArrowNativeBatch
}
(as ColumnarBatches.offload results in native Arrow batch)
Would you see if we can add the code to this class? If yes we can also remove RangeExecBaseTransformer's default implementation.
Doing this will make query planner to insert an explicit ArrowToVelox c2c transition into query plan so we can easily collect the transition's metrics.
Futhermore, I will suggest remove line 117 and line 118 then return the batch directly, then have the batch type changed as
override def batchType(): Convention.BatchType = {
ArrowJavaBatch
}
So two explicit c2cs (ArrowJava-to-ArrowNative, ArrowNative-to-Velox) can be inserted to query plan.
More details please refer to test code.
| import org.apache.spark.sql.GlutenSQLTestsTrait | ||
| import org.apache.spark.sql.Row | ||
|
|
||
| class GlutenSQLRangeExecSuite extends GlutenSQLTestsTrait { |
There was a problem hiding this comment.
Why we need test suites for all Spark versions?
zhouyuan
changed the title
We have some early discussion on this to map it to |
|
@ArnavBalyan would you please check the failed unit tests? thanks, -yuan |
|
Hi @zhouyuan thanks for the review, interesting thread, I will go through! I'll fix the tests today/tomorrow thanks! |
What changes were proposed in this pull request?
How was this patch tested?