make inputs of AsymmetricJoinSizer spillable for non kudo cases #12418
Conversation
Signed-off-by: Hongbin Ma (Mahone) <[email protected]>
|
build |
|
LGTM but better have more reviews from others. |
thx @firestarman , @abellina can you also pls take a look? |
binmahone
requested review from
firestarman and
abellina
and removed request for
firestarman
sameerz
added
the
task
There was a problem hiding this comment.
As both @binmahone and @revans2 pointed out separately, the buffers added here in the host are getting added to the SpillFramework via trackNoSpill. That means they are not going to spill them in the default config we have in 25.04, not until we have a GPU oom and we need to force some host memory out to disk.
Not only that, but making every shuffle spillable is adding more change than what the AsymmetricJoin needs strictly, so we are adding more change that hasn't had enough soak time, and that means we need to not making this change here. We should look into making the change in 25.06 together with turning on host memory limits by default.
| JCudfSerialization.concatToHostBuffer(headers, buffers) | ||
| withResource(new ArrayBuffer[HostMemoryBuffer]) { buffers => | ||
| val headers = new ArrayBuffer[SerializedTableHeader] | ||
| tables.foreach(t => { |
There was a problem hiding this comment.
we already have the length of the headers and the buffers, we could create Arrays instead of ArrayBuffer here.
| @@ -291,21 +291,33 @@ private class GpuColumnarBatchSerializerInstance(metrics: Map[String, GpuMetric] | |||
| */ | |||
| class SerializedTableColumn( | |||
| val header: SerializedTableHeader, | |||
| val hostBuffer: HostMemoryBuffer) extends GpuColumnVectorBase(NullType) with SizeProvider { | |||
| val shb: SpillableHostBuffer) extends GpuColumnVectorBase(NullType) with SizeProvider { | |||
There was a problem hiding this comment.
nit, shb -> spillableHostBuffer
|
Offline synced with @abellina , he's still worried that current fix will put too much pressure on the case where we have a really huge build side, and user using default configs. (Even though in my opinion we don't even have confidence such corner cases will work with latest code). But we do agree that we can put it off to 25.06 to avoid making rash decisions. In 25.06, we will face three options: option1: keep subpartitionhashjoin and sizedhashjoin as it is, enable offheaplimit by default, and make everything spillable we'll need to decide which option to use. @revans2 @abellina |
quote issue descrption from #12417:
This PR closes #12417 by modifying SerializedTableColumn's HostMemoryBuffer field to a SpillableHostBuffer field