Skip to content

fix: make Comet's in-memory cache opt-in again - #6963

Merged
andygrove merged 1 commit into
apache:mainfrom
andygrove:andygrove/in-memory-cache-opt-in
Oct 14, 2026
Merged

andygrove merged 1 commit into
apache:mainfrom
andygrove:andygrove/in-memory-cache-opt-in

Conversation

@andygrove

Copy link
Copy Markdown
Member

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.enabled default to true on 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:

Issue With default settings Fix
#6787 An outer hash join over a sorted cache returns unsorted partitions #6835, in review
#6898 A broadcast nested loop join over a sorted cache does the same. With AQE off, a sort-merge join above it returns 10600 rows where Spark returns 30000 None yet
#6922 Cache shapes that #6835 still leaves out of order None yet

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 main for 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 on main before it ships.

What changes are included in this PR?

  • spark.comet.exec.inMemoryCache.enabled defaults to false on 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.
  • Tests keep running against Comet's cache format:
    • CometInMemoryCacheSuite sets the flag in its sparkConf. The other cache suites already set it themselves. The test that pinned the per-version default now asserts that it is false.
    • The 3.5.9, 4.0.4, 4.1.3 and 4.2.0 Spark SQL diffs set the flag beside Comet's cache serializer in CometTestSettings, SharedSparkSession and TestHive, 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.
  • Docs: the user guide, the operator tables and the plan guide describe the cache as experimental and disabled by default again. The In-Memory Cache guide's Limitations section describes the ordering issues and links them. The 1.2.0 upgrade guide section is removed, and the Spark SQL tests guide explains why the test JVMs turn the cache on.

How are these changes tested?

  • CometInMemoryCacheSuite, CometInMemoryCachePruningSuite, CometInMemoryCacheKryoSuite, CometInMemoryCacheKryoUnregisteredSuite and CometInMemoryCacheKryoClassesToRegisterSuite pass locally on Spark 4.1 (80 tests) and Spark 3.4 (74 tests, and 6 canceled that need Spark 3.5 or 4.0).
  • Each diff was regenerated from a clean checkout of its tag with the current diff applied. Before the edit, the regenerated diff matched the current one byte for byte. After it, the diff changes only in the lines above, and it applies cleanly to a fresh checkout of the tag.
  • run-all-spark-profiles and the run-spark-3.5-tests, run-spark-4.0-tests, run-spark-4.1-tests and run-spark-4.2-tests labels run Comet's suites and Spark's SQL suites before this is queued.

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.
@andygrove andygrove added run-all-spark-profiles Run the Comet test suites against every Spark profile on this pull request, ahead of the merge queue run-spark-3.5-tests Run the Spark 3.5 SQL tests on this PR run-spark-4.0-tests run-spark-4.1-tests Run the Spark 4.1 SQL tests on this pull request instead of waiting for the merge queue run-spark-4.2-tests Run the Spark 4.2 SQL tests on this PR labels Oct 14, 2026
@github-actions github-actions Bot added the bug Something isn't working label Oct 14, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 false across 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, and CometCacheColumnarRule. Checked upstream Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0 sources: serializer selection is retained by InMemoryRelation, 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.1 at 18732aa2150b52e1b456fdb6317fee5947edfb68, where this flag also defaults to false. 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 in CometInMemoryCacheSuite.sparkConf, update the default assertion, and add explicit opt-in settings to CometTestSettings, SharedSparkSession, and TestHive for 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.

@andygrove andygrove added this to the 1.2.0 milestone Oct 14, 2026
@andygrove
andygrove requested a review from mbutrovich October 14, 2026 14:30
@andygrove
andygrove added this pull request to the merge queue Oct 14, 2026
Merged via the queue into apache:main with commit 9e9eb44 Oct 14, 2026
278 of 282 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working run-all-spark-profiles Run the Comet test suites against every Spark profile on this pull request, ahead of the merge queue run-spark-3.5-tests Run the Spark 3.5 SQL tests on this PR run-spark-4.0-tests run-spark-4.1-tests Run the Spark 4.1 SQL tests on this pull request instead of waiting for the merge queue run-spark-4.2-tests Run the Spark 4.2 SQL tests on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants