[SPARK-24336][SQL] Support 'pass through' transformation in BasicOperators #21388
Conversation
|
Test build #90924 has finished for PR 21388 at commit
|
|
|
||
| lazy val operatorToTargetConstructor: Map[Class[_ <: LogicalPlan], Constructor[_]] = | ||
| passThroughOperators.map { | ||
| case (srcOpCls, tgtOpCls) => |
|
|
||
| lazy val operatorToConstructorParameters: Map[Class[_ <: LogicalPlan], Seq[(String, Type)]] = | ||
| passThroughOperators.map { | ||
| case (srcOpCls, _) => |
There was a problem hiding this comment.
nit:
.map { case (srcOpCls, _) =>
per https://github.com/databricks/scala-style-guide#pattern-matching
| def getClassFromTypeHandleArray(tpe: Type): Class[_] = cleanUpReflectionObjects { | ||
| tpe.dealias match { | ||
| case ty if ty <:< localTypeOf[Array[_]] => | ||
| def arrayClassFromType(tpe: `Type`): Class[_] = |
There was a problem hiding this comment.
hmm .. Could we avoid this nested function?
|
Test build #90922 has finished for PR 21388 at commit
|
|
Thanks @HyukjinKwon for reviewing. Addressed review comments. |
|
Test build #90946 has finished for PR 21388 at commit
|
|
retest this please |
|
I don’t think it is a good a idea to put reflection magic in the planner. If you want to add cases to the planner please use the existing hooks (SparkSessionExtensions, ExperimentalMethods or override SparkSessionBuilder). -1 on this PR. |
|
Test build #90952 has finished for PR 21388 at commit
|
|
@hvanhovell For me, reflection looks like the only way to achieve Btw, I'd be very happy if you don't mind to spend some time to explain which points make you being concerned about reflection in planner. Maybe adding the description explicitly would avoid the similar trial on contributors and save our time. |
|
@HeartSaVioR I really don't think we should automate these things at all. The planner is a pretty critical component, and I'd rather be explicit on how a What is the problem you are trying to solve here? |
|
@hvanhovell and I thought the comment makes sense: it would be beneficial if we just couple matching pair of (LogicalPlan, SparkPlan) for the cases which don't require some transformations while transforming. For the first time, I tried my best to stick with compile-time things, but realized it is not possible to achieve without runtime reflection (at least for me) after couple of hours. So another couple of hours were spent on resolving. I have no strong opinion to adopt reflection on planner (so happy to see the approach got rejected), but if we agree it cannot be handled without reflection, the origin comment should be removed, or describing limitations on addressing it so that others might try out with avoiding limitations. |
* The option is no longer preferred one as below comment * apache#21388 (comment) * Removing this to prevent contributors to waste their times
|
I just provided new patch to remove the comment, as it looks like no longer preferred option. Closing this one. |
What changes were proposed in this pull request?
Enable 'pass through' transformation in BasicOperators via reflection, so that every pairs of transformation which only requires converting LogicalPlan to SparkPlan via calling
planLater()can be transformed automatically. It just needs to add the pair of transformation in map.How was this patch tested?
Unit tests on existing tests.