[CORE] Simplify TransformSupport #3426
Conversation
|
Thanks for opening a pull request! Could you open an issue for this pull request on Github Issues? https://github.com/oap-project/gluten/issues Then could you also rename commit message and pull request title in the following format? See also: |
|
Run Gluten Clickhouse CI |
1 similar comment
|
Run Gluten Clickhouse CI |
| @@ -58,18 +65,14 @@ trait TransformSupport extends GlutenPlan { | |||
| */ | |||
| def columnarInputRDDs: Seq[RDD[ColumnarBatch]] | |||
|
|
|||
| def getBuildPlans: Seq[(SparkPlan, SparkPlan)] | |||
|
|
|||
| def getStreamedLeafPlan: SparkPlan | |||
There was a problem hiding this comment.
These two methods are from first initial commit, but it seems we never use them now. I'm not sure the original idea. cc @rui-mo @zhztheplayer @PHILO-HE @zzcclp if you have some context
There was a problem hiding this comment.
It's some legacy code copied from Gazelle.
There was a problem hiding this comment.
Thanks. It looks good to me to remove them from the code.
| s"${this.getClass.getSimpleName} doesn't support doExecute") | ||
| } | ||
|
|
||
| final override lazy val supportsColumnar: Boolean = GlutenConfig.getConf.enableColumnarIterator |
There was a problem hiding this comment.
This config is NOT applicable to velox backend, as only columnar iterator is supported. @zzcclp, @baibaichen, is this config still workable for CH backend? Thanks!
There was a problem hiding this comment.
I'm +1 to remove this config if does not break CH backend
There was a problem hiding this comment.
@loneylee, could you please help confirm? Thanks!
|
Run Gluten Clickhouse CI |
1 similar comment
|
Run Gluten Clickhouse CI |
|
Run Gluten Clickhouse CI |
|
Run Gluten Clickhouse CI |
1 similar comment
|
Run Gluten Clickhouse CI |
|
/Benchmark Velox |
|
CC @JkSelf |
| case _ => false | ||
| } | ||
| .map(_.asInstanceOf[TransformSupport].metricsUpdater()) | ||
| .getOrElse(new NoopMetricsUpdater) |
There was a problem hiding this comment.
As NoopMetricsUpdater doesn't do any specific thing, maybe, we can change NoopMetricsUpdater from class to object type. Thanks!
| @@ -28,8 +28,7 @@ import java.util | |||
| import scala.collection.JavaConverters._ | |||
|
|
|||
| case class FilterExecTransformer(condition: Expression, child: SparkPlan) | |||
| extends FilterExecTransformerBase(condition, child) | |||
| with TransformSupport { | |||
| extends FilterExecTransformerBase(condition, child) { | |||
There was a problem hiding this comment.
FilterExecTransformerBase already extends TransfromSupport
|
Run Gluten Clickhouse CI |
|
===== Performance report for TPCH SF2000 with Velox backend, for reference only ====
|
|
===== Performance report for TPCH SF2000 with Velox backend, for reference only ====
|
What changes were proposed in this pull request?
getBuildPlansandgetStreamedLeafPlanLeafTransformSupportandUnaryTransformSupportto simplifycolumnarInputRDDsdoExecuteandsupportsColumnartoTransformSupportCOLUMNAR_ITERATOR_ENABLEDconfigHow was this patch tested?
Pass CI