[SPARK-13250][SQL] Update PhysicallRDD to convert to UnsafeRow if using the vectorized scanner. #11141
Conversation
|
@davies I think this is a general purpose way we can remove 'copy' from InternalRow. We're essentially delaying the serialization as late in the plan tree as possible. |
|
In the past, we had a planner to insert a ConvertToUnsafe and ConvertToSafe for operators that expect UnsafeRow or SafeRow, that is removed to simplify complexity, maybe we need to bring that back for the parquet reader? see #10511 |
|
@davies I don't like that approach. Converting to unsaferow is not really the thing we want to be doing long term. In this example for instance, i think it would be nice if the value in the hashtable was just the unsafe row of the non-join key columns. This is hard to do if we inserted a plan node. |
|
Test build #50996 has finished for PR 11141 at commit
|
|
@nongli Are you suggesting that we should do this kind of workaround for every operator that requires UnsafeRow (in-memory cache, SortMergeJoin, CartesianProduct, Exchange, etc) ? |
|
@davies Yes I am. I don't think we're going to add a ton more operators and in all the one you mentioned, we should think hard about serializing to the in memory structure the operator wants rather than just copying. For example, CartesianProduct should probably serialize all the rows on one side contiguously; similarly sort for tungsten pages. In-memory cache should use a columnar version and not need to first go to UnsafeRow. I think we can take what I've done here and make it more componentized so it's less code duplication to use this elsewhere. I'm okay not doing this in general in the planner. The operators that need to accumulate memory should think hard about how to do it. |
|
Test build #51010 has finished for PR 11141 at commit
|
|
I updated the approach to do this a differnet way. |
|
Test build #51070 has finished for PR 11141 at commit
|
|
|
||
| // Support codegen so that we can avoid the UnsafeRow conversion in all cases. Codegen | ||
| // never requires UnsafeRow as input. | ||
| override def supportCodegen: Boolean = true |
There was a problem hiding this comment.
It's true by default in CodegenSupport
|
Could you also update the title and description of PR? |
nongli
changed the title
|
Test build #51071 has finished for PR 11141 at commit
|
|
Test build #51073 has finished for PR 11141 at commit
|
|
Test build #51081 has finished for PR 11141 at commit
|
|
Test build #51203 has finished for PR 11141 at commit
|
There was a problem hiding this comment.
Could you add SQL metric for "numOutputRows"?
|
Test build #2573 has started for PR 11141 at commit |
|
Test build #51788 has finished for PR 11141 at commit
|
|
Test build #51823 has finished for PR 11141 at commit
|
|
LGTM, @nongli Could you fix the conflicts? |
…ng the vectorized scanner. Some parts of the engine rely on UnsafeRow which the vectorized parquet scanner does not want to produce. This add a conversion in Physical RDD. In the case where codegen is used (and the scan is the start of the pipeline), there is no requirement to use UnsafeRow. This patch adds update PhysicallRDD to support codegen, which eliminates the need for the UnsafeRow conversion in all cases. The result of these changes for TPCDS-Q19 at the 10gb sf reduces the query time from 9.5 seconds to 6.5 seconds.
|
Test build #51887 has finished for PR 11141 at commit
|
|
test this please |
|
Test build #51893 has finished for PR 11141 at commit
|
|
test this please |
|
Test build #51908 has finished for PR 11141 at commit
|
|
Merging this into master, thanks! |
4 participants
Some parts of the engine rely on UnsafeRow which the vectorized parquet scanner does not want
to produce. This add a conversion in Physical RDD. In the case where codegen is used (and the
scan is the start of the pipeline), there is no requirement to use UnsafeRow. This patch adds
update PhysicallRDD to support codegen, which eliminates the need for the UnsafeRow conversion
in all cases.
The result of these changes for TPCDS-Q19 at the 10gb sf reduces the query time from 9.5 seconds
to 6.5 seconds.