Copilot commented on code in PR #12765:
URL: https://github.com/apache/gluten/pull/12765#discussion_r3776883259
##########
backends-velox/src/main/scala/org/apache/gluten/execution/VeloxBroadcastBuildSideCache.scala:
##########
@@ -189,6 +190,22 @@ object VeloxBroadcastBuildSideCache
}
}
+ /**
+ * Recovers the raw build side relation that belongs to a driver-side built
hash table.
+ *
+ * `driverSerializedCache` pins the relation on the driver for the lifetime
of the broadcast, so
+ * this returns a non-null value there even if the broadcast block itself
had to be re-read from
+ * disk. On executors the cache is always empty and the result is null,
which is fine because the
+ * raw batches are never consumed there.
+ */
Review Comment:
This Scaladoc says `driverSerializedCache` "pins the relation on the driver
for the lifetime of the broadcast", but the cache is configured with
`expireAfterAccess`, so entries can still be evicted/expired while the
broadcast is alive. If that happens,
`SerializedBroadcastHashTable.readExternal` cannot recover `buildSideRelation`
on the driver after a disk reload, and driver-side consumers will see null and
fail later.
##########
backends-velox/src/main/scala/org/apache/gluten/execution/SerializedBroadcastHashTable.scala:
##########
@@ -48,8 +62,12 @@ class SerializedBroadcastHashTable(
out.writeBoolean(droppedDuplicates)
out.writeLong(bloomFilterBlocksByteSize)
out.writeLong(hashProbeDynamicFiltersProduced)
+ out.writeUTF(if (driverRelationId == null) "" else driverRelationId)
serializedData.writeExternal(out)
- out.writeObject(buildSideRelation)
+ // `buildSideRelation` is intentionally not written. It holds the raw
build side batches, which
+ // are only consumed on the driver (DPP key extraction through
`transform`, and fallback to
+ // vanilla Spark through `deserialized`). Executors read the serialized
hash table and never
+ // touch the raw batches, so shipping both would roughly double the
broadcast payload.
}
Review Comment:
The broadcast payload format and semantics changed (no longer serializing
`buildSideRelation`, plus `driverRelationId` recovery and per-JVM handle
memoization with `cloneHashTable`/`releaseIfOwner`). There are existing
driver-side broadcast hash table tests (e.g. `VeloxHashJoinSuite`), but none
that exercise a serialize/deserialize round-trip and validate: (1) executors
see `buildSideRelation == null`, (2) the driver can recover `buildSideRelation`
after re-read, and (3) multiple `deserialize(cacheKey)` calls clone rather than
re-deserialize. Adding a focused unit test would help prevent regressions.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
