Repository navigation
fix: make Comet's in-memory cache opt-in again - #6963
Conversation
Set spark.comet.exec.inMemoryCache.enabled back to false on every Spark version. Since apache#5634 turned it on from Spark 3.5, sorted cached relations reach native joins that do not keep the order Spark relies on (apache#6787, apache#6898, apache#6922), so those wrong results are reachable with default settings. Keep Comet's cache suites and the 3.5, 4.0, 4.1 and 4.2 Spark SQL diffs running against Comet's cache format by setting the flag beside the cache serializer, and restore the opt-in wording in the docs.
sunchao
left a comment
There was a problem hiding this comment.
Reviewed the full 12-file diff against a1230c194f0526317df4bf2be78486c5c806eb88 at head 3919e069077a0198a3fae486bfc0db0d96f3af95. The PR is not a draft. The snapshot and live GitHub discussion contain no reviews, conversation comments, or review threads.
Routed skills: .ai/skills/review-comet-pr/SKILL.md. No subsystem sibling applies to this configuration, test-harness, and documentation change.
Summary
- Prior state and problem: Comet’s Arrow cache was enabled by default on Spark 3.5 and later, exposing the documented ordering problems tracked in #6787, #6898, and #6922. This PR restores explicit opt-in.
- Design approach: Change the existing cache flag’s default to
falseacross Spark versions while explicitly enabling it in cache tests and Spark SQL test harnesses. - Correctness: Traced the flag through
CometDriverPlugin.maybeSetCacheSerializer, native cache-scan selection, andCometCacheColumnarRule. Checked upstream Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0 sources: serializer selection is retained byInMemoryRelation, and cache scans advertise the cached plan’s ordering. The new default consistently selects Spark’s cache path. No introduced P1/P2 correctness issue was identified. - Compatibility analysis: Compared the affected configuration with
branch-1.1at18732aa2150b52e1b456fdb6317fee5947edfb68, where this flag also defaults tofalse. Explicit opt-in remains supported. Spark 3.4’s default is unchanged, and its existing union/AQE limitation remains documented. - Key design decisions: Reuse the existing supported flag, preserve startup-time serializer selection, and keep Spark SQL suites exercising Comet’s cache despite the production default changing.
- Implementation sketch: Replace the version-dependent default with
false, enable the cache inCometInMemoryCacheSuite.sparkConf, update the default assertion, and add explicit opt-in settings toCometTestSettings,SharedSparkSession, andTestHivefor Spark 3.5–4.2. Update the associated guides and operator tables. - Performance: Default cache-dependent pipelines can lose native acceleration relative to the base commit. That is the deliberate correctness tradeoff. The patch adds no per-row processing or allocation. No unintended performance regression was established, and no benchmarks were run.
- Design: The rollback is narrow and straightforward. Removing the version-dependent default simplifies the policy without changing the cache implementation.
- Abstraction & complexity: No new abstraction or configuration is introduced. The test settings use existing initialization points, including JVM-wide defaults needed because Spark retains its first cache serializer.
- Behavioral changes worth calling out: Applications using defaults on Spark 3.5+ return to Spark’s cache format and scan path. Enabling Comet’s format still requires configuration before startup. The ordering defects remain relevant to explicit opt-in users and are documented as limitations.
- Suggested improvements: No additional change meeting the P1/P2 reporting bar was identified. The outstanding integration checks remain necessary validation evidence.
Exact-head CI, observed at 2026-10-14 01:17 UTC: 142 checks, with 19 successful, 107 skipped, and 16 running. No failures were reported. Linux build/test checks and the labeled Spark 3.5, 4.0, 4.1, and 4.2 SQL jobs were still running, with those SQL jobs at their build stage. This is not a completed CI verdict.
Validation: git diff --check passed. The changed sections of all four Spark patches applied cleanly to their upstream tags for both base and head. Comparing the applied sources confirmed that executable changes consist only of the three cache opt-in settings per version. Scala 2.13.17 syntax parsing passed for the two changed Scala files and twelve patched upstream files. Parsing does not establish typechecking or runtime compatibility. No Maven, native, or Spark integration suites were run locally, and the author’s reported test results were not independently reproduced. The project working tree remains unchanged.
No introduced P1/P2 issues found within this review. No substantiated unresolved existing review blockers were identified.
Which issue does this PR close?
None. This turns the default from #5634 back off for the 1.2.0 release, because of #6787, #6898 and #6922. Part of #6834.
Rationale for this change
Since #5634 made
spark.comet.exec.inMemoryCache.enableddefault totrueon Spark 3.5 and later, three open wrong-results bugs are reachable with default settings. On branch-1.1, where the cache is opt-in, none of them is:They share a cause. Comet operators report the order Spark reports for them without always keeping it natively, and a cached relation carries that order into later queries, where nothing checks it. Each review round on #6835 has found another case, most recently a cache written through a native sample. With default settings, the cache is the main way a sorted input reaches a native plan, so turning it off removes that exposure while the fixes land.
The default has been on in
mainfor four days, and 1.2.0 would be the first release to ship it. 1.1.0 shipped the cache as opt-in, so with this change no release has it on by default, and the 1.2.0 upgrade guide no longer needs a section for it. Turning it back on can wait for #6787, #6898 and #6922 to be fixed, ideally early in a release cycle so it gets a full cycle onmainbefore it ships.What changes are included in this PR?
spark.comet.exec.inMemoryCache.enableddefaults tofalseon every Spark version. The Spark 3.4 special case goes away. The 3.4 union limitation it described stays in the config doc and the user guide for anyone who turns the cache on.CometInMemoryCacheSuitesets the flag in itssparkConf. The other cache suites already set it themselves. The test that pinned the per-version default now asserts that it isfalse.CometTestSettings,SharedSparkSessionandTestHive, so Spark's suites keep running against Comet's cache format. Without the flag they would write Comet's format and read it back through Spark's fallback scan. The 3.4.3 diff is unchanged.How are these changes tested?
CometInMemoryCacheSuite,CometInMemoryCachePruningSuite,CometInMemoryCacheKryoSuite,CometInMemoryCacheKryoUnregisteredSuiteandCometInMemoryCacheKryoClassesToRegisterSuitepass locally on Spark 4.1 (80 tests) and Spark 3.4 (74 tests, and 6 canceled that need Spark 3.5 or 4.0).run-all-spark-profilesand therun-spark-3.5-tests,run-spark-4.0-tests,run-spark-4.1-testsandrun-spark-4.2-testslabels run Comet's suites and Spark's SQL suites before this is queued.