snmvaughan commented on code in PR #5634:
URL: https://github.com/apache/datafusion-comet/pull/5634#discussion_r4232020760
##########
dev/diffs/4.1.3.diff:
##########
@@ -423,28 +474,63 @@ index 0d807aeae4d..6d7744e771b 100644
}
test("A cached table preserves the partitioning and ordering of its cached
SparkPlan") {
-@@ -1673,9 +1674,18 @@ class CachedTableSuite extends QueryTest with
SQLTestUtils
+@@ -1595,7 +1602,8 @@ class CachedTableSuite extends QueryTest with
SQLTestUtils
+ }
+ }
+
+- test("SPARK-36120: Support cache/uncache table with TimestampNTZ type") {
++ test("SPARK-36120: Support cache/uncache table with TimestampNTZ type",
++ IgnoreComet("Comet's cache format reports Arrow buffer sizes")) {
Review Comment:
A few of the new `IgnoreComet` tags skip more than the assertion that
differs. `SPARK-36120` also checks that a `TIMESTAMP_NTZ` relation caches and
reads back, and only `sizeInBytes === 8` depends on the format. In
`SPARK-33687` only the `checkOptimizedPlanStats(..., 4, ...)` calls on the
cached views do. In `SPARK-22673` only `sizeInBytes === 16` after `collect()`
does. `SPARK-43376` loses `runAdaptiveAndVerifyResult`'s answer check when only
the reuse count differs. Could these wrap just those assertions in `if
(!isCometEnabled)`, as other adaptations in these diffs do, so the rest keeps
running against Comet's format? (Lines 483, 1707, 3286 and 3404 here, and the
same tests in the other diffs.)
##########
spark/src/test/scala/org/apache/spark/sql/benchmark/CometInMemoryCacheBenchmark.scala:
##########
@@ -488,6 +485,116 @@ object CometInMemoryCacheBenchmark extends
CometBenchmarkBase {
}
}
+ /**
+ * What the feature changes for a query that runs with Comet, against
Spark's own cache format,
+ * with AQE on and Comet's other settings at their defaults. This is the
comparison an
+ * application gets from turning the feature on or off. Both formats are
cached from the same
+ * relation, one copy at a time as in runSparkOperatorBenchmark.
+ *
+ * Two shapes of plan read the cache. With Comet operators above the cache
scan, Comet's format
+ * runs the whole query natively, while Spark's leaves the operators
directly above its scan on
+ * Spark: Comet reads Spark's cache scan only through
spark.comet.convert.inMemoryCache.enabled,
+ * which is off by default. With a Spark operator above the scan, Comet's
format is read by the
+ * native scan and converted to rows for that operator, where Spark's is
read by Spark's own
+ * scan. The Spark operator is the aggregate, with Comet's turned off,
standing in for any
+ * operator Comet does not support.
+ */
+ private def runAdaptiveBenchmark(relation: CachedRelation): Unit = {
Review Comment:
The adaptive cases answer the read-side question well. The default flip also
changes the write side for every cached relation, though, and the guide only
compares building the cache with `zstd` against `none` (1507 ms and 1081 ms).
Could the benchmark also time building the cache in Spark's format and report
its footprint? A job that caches once and reads it once or twice pays the build
more than the reads.
##########
dev/diffs/4.1.3.diff:
##########
@@ -4410,6 +4874,11 @@ index 720b13b812e..e3ac2cebc6e 100644
+ "org.apache.spark.sql.comet.execution.shuffle.CometShuffleManager")
+ .set("spark.comet.shuffle.enabled", "true")
+
++ // CometDriverPlugin installs Comet's cache serializer when
++ // spark.comet.exec.inMemoryCache.enabled is on, as it is by default.
These sessions do not
++ // load the plugin, so install it here.
++ conf.set("spark.sql.cache.serializer",
Review Comment:
Spark keeps the first cache serializer it loads for the rest of the JVM, and
`CachedBatchSerializerSuite` and now `CachedBatchSerializerNoUnwrapSuite` clear
it in `afterAll`. Suites that build their own session run Comet since #6415 but
don't set `spark.sql.cache.serializer`. So if `BroadcastJoinSuite` or
`SparkSessionExtensionSuite` caches first after a clear, the rest of that JVM
caches in Spark's format. The adapted assertions accept both formats, so
nothing would fail, and the coverage would just go missing. Would it be more
robust to put
`-Dspark.sql.cache.serializer=org.apache.spark.sql.comet.execution.arrow.ArrowCachedBatchSerializer`
in
[`CometTestSettings`](https://github.com/apache/datafusion-comet/blob/83ca07a7dd7b3bee2532e4ac063bc83ec61ce0bf/dev/diffs/4.1.3.diff#L106)
next to the shuffle manager, in the 3.5 and later diffs?
##########
dev/diffs/4.1.3.diff:
##########
Review Comment:
This is about the `if (!isCometEnabled)` guard on `Union two datasets with
different pre-shuffle partition number` in `CoalesceShufflePartitionsSuite`.
This PR doesn't touch those lines, so I can't comment on them inline:
[3.5.9.diff#L1657](https://github.com/apache/datafusion-comet/blob/83ca07a7dd7b3bee2532e4ac063bc83ec61ce0bf/dev/diffs/3.5.9.diff#L1657),
[4.0.4.diff#L2062](https://github.com/apache/datafusion-comet/blob/83ca07a7dd7b3bee2532e4ac063bc83ec61ce0bf/dev/diffs/4.0.4.diff#L2062),
[4.1.3.diff#L2236](https://github.com/apache/datafusion-comet/blob/83ca07a7dd7b3bee2532e4ac063bc83ec61ce0bf/dev/diffs/4.1.3.diff#L2236)
and
[4.2.0.diff#L2303](https://github.com/apache/datafusion-comet/blob/83ca07a7dd7b3bee2532e4ac063bc83ec61ce0bf/dev/diffs/4.2.0.diff#L2303).
With #6459 in, I think this guard can come off on 3.5 and later. #6459
ported this exact query to `CometExecSuite` and gets two coalesced reads. That
port uses the default advisory size and checks `isCoalescedRead`, while this
test uses a 100-byte target and `hasCoalescedPartition`. I don't think the
block-size argument from #6415 applies to this assertion, though. With the
test's 5 shuffle partitions, keys 0, 1 and 2 hash to partitions 0, 4 and 3, so
partitions 1 and 2 are empty. Spark's `coalescePartitions` folds them into a
`[1, 4)` spec whatever size Comet's blocks are, which is enough for
`hasCoalescedPartition` on both join-side reads. The 3.4 diff would keep the
guard, since the rule does nothing there. This PR already re-enables
`SPARK-42101` because of #6459, so could it drop this guard too?
--
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]
