[GitHub] [spark] sarutak edited a comment on issue #25260: [SPARK-28520][SQL] WholeStageCodegen does not work property for LocalTableScanExec

[GitBox <gi...@apache.org>]

sarutak edited a comment on issue #25260: [SPARK-28520][SQL] WholeStageCodegen does not work property for LocalTableScanExec
URL: https://github.com/apache/spark/pull/25260#issuecomment-516636379
 
 
   Thanks @reactormonk for the comment!
   
   >  There are a few things I'm wondering about, though. Is LocalTableScanExec the only case where we're doing this kind of "if root then don't codegen" cutoff? If there are other cases, it might make more sense to extract a trait and make the insertWholeStageCodegen logic cleaner and more flexible.
   
   Yes for now. First I considered adding new trait but I found the case we need to cut off was only `LocaTableScanExec` so I decided to change `insertWholeStageCodegen` like a `SMJ` case in `insertInputAdapter`.  But it's  worth re-considering to add new trait. I'll try it.
   
   > Also, what about the case when the immediate parent of a LocalTableScanExec is a Spark stage boundary, e.g. an Exchange? That case seems to be covered by this PR already (which doesn't insert a WholeStageCodegenExec right above LocalTableScanExec. Is that correct?
   
   Yes, this PR cover the case you mentioned. Added test case using `HashAggregation` covers it.
   
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org