Skip to content

docs: update the user guide for the 1.1.0 release - #6168

Queued
andygrove wants to merge 4 commits into
apache:mainfrom
andygrove:docs-1.1.0-user-guide
Queued

andygrove wants to merge 4 commits into
apache:mainfrom
andygrove:docs-1.1.0-user-guide

Conversation

@andygrove

@andygrove andygrove commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Which issue does this PR close?

Part of #5327.

Rationale for this change

The release process asks for a review of the user guide before the release branch is cut. I went through every page under docs/source/user-guide/latest/ against the changes merged since 1.0.0 branched (af534e0), and checked each statement I touched against the code.

What changes are included in this PR?

Most of the diff corrects statements that stopped being true. The CI table in the installation guide still said every Spark version is tested on every change, but since #5963 only Spark 4.1 is gated before merge and the other profiles run nightly. The Spark 3.4/3.5 "known limitations" described Parquet reads that Comet has rejected, like Spark does, since before 1.0. A number of expression notes, and the cast legend, said "falls back" where Comet now routes the case through the codegen dispatcher by default. The S3 region and Iceberg NOT IN pushdown descriptions were wrong, as were the tokio thread defaults and several shuffle and scan metric names. The Kubernetes example pointed at a Java 11 / Spark 3.5 image and set no off-heap memory, which leaves Comet disabled.

The compatibility guide listed divergences that 1.1.0 fixed: the boolean to decimal cast, explicit positive timestamp years, the ABFS object store cache key, and most of the ANSI error-message items. Those entries are removed.

It also documents 1.1.0 behaviour that had no coverage. That includes the experimental in-memory cache, the native Iceberg writer (including the credential provider's WRITE access mode), nested hash shuffle keys, BINARY sort-merge join keys, and the aggregate spill and memory metrics. It also covers native TIME functions, scans of tables whose Variant columns aren't read, and missing expression and plan-node rows. The installation guide now documents CometKryoRegistrator, which native broadcast needs when Kryo runs with spark.kryo.registrationRequired=true, whether or not the in-memory cache is enabled. The upgrade guide gets an "Upgrading to Comet 1.1.0" section. Diffing the generated config reference against 1.0.0 shows no removed keys, no changed defaults, and no spark.comet.legacy.* keys, so the section says no behavior change needs a legacy key, and notes that JDK 17 is now required. The release process also loses its two notes about tag pushes publishing a Docker image, since that workflow was removed in #4241.

A few things are deliberately left alone:

  • The Iceberg transform-residual limitation still says those scans fall back. Some of them actually return wrong rows, and the right wording depends on whether fix(iceberg): don't push transform residuals as their source column, fail on residual errors #6154 lands before the cut.
  • The links in expressions.md to compatibility/expressions/<category>.md don't resolve on the published site, because those pages only exist under spark-<ver>/ at build time. 1.0.0 has the same problem. Should they point at the Spark 4.1 pages or at the index?
  • The from_unixtime example in understanding-comet-plans.md no longer falls back, and a replacement needs real plan output.
  • There is no Spark 4.2 expression-compatibility page yet.
  • Some generated text is stale at its source: the scalaUDF.codegen.enabled and memoryOverhead config descriptions, and the TruncTimestamp fallback reason. Those are code changes, so a follow-up.

How are these changes tested?

This is documentation only. Each corrected or added statement was checked against the code at dd68a53: serde support levels, config definitions and defaults, operator node names, and metric names. The branch has since been merged with main at 88a1f48. Where main had changed the same pages (Variant, Iceberg writes, shuffle metrics), the merge keeps main's newer text. Prettier passes. I haven't run the full docs build, but a script checked the relative links and anchors in the changed pages. The only targets that don't resolve are the pre-existing category links mentioned above.

Sweep of the user guide against everything merged since 1.0.0 branched
(af534e0), per the release preparation step in the release process.

Correct statements that no longer match the code: the CI coverage table
(only Spark 4.1 is gated before merge since the non-default profiles
moved to the nightly run), the Spark 3.x Parquet limitations Comet has
long since closed, cast/JSON/regex/array notes that say "falls back"
where Comet now routes through the codegen dispatcher, S3 region
auto-detection, Iceberg NOT IN pushdown, tokio thread defaults, shuffle
and scan metric names, and a Kubernetes example that pointed at a
Java 11 / Spark 3.5 image and left Comet disabled for lack of off-heap
memory.

Remove known-divergence entries that 1.1.0 fixed (boolean to decimal
cast, explicit positive timestamp years, the ABFS object store cache
key, most of the ANSI error-message items), and document new 1.1.0
behaviour: the experimental in-memory cache, the native Iceberg writer
and the WRITE credential access mode, nested hash shuffle keys, BINARY
sort-merge join keys, aggregate spill and memory metrics, native TIME
functions, Variant-bearing tables whose Variant columns are not read,
and the missing expression and plan-node rows.

Add an "Upgrading to Comet 1.1.0" section to the upgrade guide, and drop
the release-process notes about tag pushes publishing a Docker image,
since that workflow was removed in apache#4241.
@github-actions github-actions Bot added documentation Improvements or additions to documentation area:Iceberg labels Sep 24, 2026
@andygrove andygrove added this to the 1.1.0 milestone Sep 24, 2026

@mbutrovich mbutrovich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First pass, thanks @andygrove!

Comment on lines +51 to +54
With `spark.kryo.registrationRequired=true`, also set
`spark.kryo.registrator=org.apache.comet.CometKryoRegistrator` before creating the
`SparkContext`. Otherwise caching fails as soon as a block is serialized, including the disk
half of the default `MEMORY_AND_DISK` storage level.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The Kryo requirement also applies to native broadcast, which is on by default, so it isn't specific to the in-memory cache. CometKryoRegistrator registers Utils.arrowBytesKryoClasses (the Array[ChunkedByteBuffer] that a native broadcast ships) along with the cache classes (CometKryoRegistrator.scala). The startup warning in Plugins.scala#L141-L148 names both. A user with spark.serializer=KryoSerializer and spark.kryo.registrationRequired=true who never enables the cache will hit "Class is not registered" on the first broadcast join, and this is the only page that mentions the registrator. Could you document it somewhere every user reads, such as "Additional Configuration" in installation.md, and have this section link to it? The condition is also narrower than written here: it applies only when spark.serializer is org.apache.spark.serializer.KryoSerializer.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved to a new "Kryo serialization" subsection under "Additional Configuration" in installation.md, limited to spark.serializer=org.apache.spark.serializer.KryoSerializer with spark.kryo.registrationRequired=true. It says native broadcast fails without the registrator whether or not the cache is enabled. Main has since added in-memory-cache.md (#5543), so the section in compatibility/operators.md now just points to that page, and both pages link to the installation section. d531350

```

These `s3.*` storage properties are not specific to the Hive catalog shown here. When `s3.access-key-id` / `s3.secret-access-key` are omitted, credentials come from the standard AWS chain (environment variables, instance profiles, and so on). `client.region` is auto-detected for AWS but should be set for non-AWS endpoints. If your REST catalog vends temporary credentials, the native reader does not consume them automatically, and wiring that requires the credential provider bridge. See Iceberg's [S3 FileIO](https://iceberg.apache.org/docs/latest/aws/#s3-fileio) docs for the full property list, and [S3 Credential Providers](s3-credential-providers.md) for vended or per-request credentials.
These `s3.*` storage properties are not specific to the Hive catalog shown here. When `s3.access-key-id` / `s3.secret-access-key` are omitted, credentials come from the standard AWS chain (environment variables, instance profiles, and so on). `client.region` is not auto-detected: when neither it nor `AWS_REGION` is set, Comet uses `us-east-1`, so set it for AWS buckets in any other region. If your REST catalog vends temporary credentials, the native reader does not consume them automatically, and wiring that requires the credential provider bridge. See Iceberg's [S3 FileIO](https://iceberg.apache.org/docs/latest/aws/#s3-fileio) docs for the full property list, and [S3 Credential Providers](s3-credential-providers.md) for vended or per-request credentials.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This describes the region default as depending only on client.region and AWS_REGION. The check in iceberg_common.rs#L139-L145 also accepts s3.region and AWS_DEFAULT_REGION. A reader who has set s3.region could take this sentence to mean Comet ignores it. s3-credential-providers.md in this PR already lists all four sources. How about matching that wording here?

Suggested change
These `s3.*` storage properties are not specific to the Hive catalog shown here. When `s3.access-key-id` / `s3.secret-access-key` are omitted, credentials come from the standard AWS chain (environment variables, instance profiles, and so on). `client.region` is not auto-detected: when neither it nor `AWS_REGION` is set, Comet uses `us-east-1`, so set it for AWS buckets in any other region. If your REST catalog vends temporary credentials, the native reader does not consume them automatically, and wiring that requires the credential provider bridge. See Iceberg's [S3 FileIO](https://iceberg.apache.org/docs/latest/aws/#s3-fileio) docs for the full property list, and [S3 Credential Providers](s3-credential-providers.md) for vended or per-request credentials.
These `s3.*` storage properties are not specific to the Hive catalog shown here. When `s3.access-key-id` / `s3.secret-access-key` are omitted, credentials come from the standard AWS chain (environment variables, instance profiles, and so on). The region is not auto-detected: when neither the catalog (`client.region` or `s3.region`) nor the executor environment (`AWS_REGION` or `AWS_DEFAULT_REGION`) supplies one, Comet uses `us-east-1`, so set it for AWS buckets in any other region. If your REST catalog vends temporary credentials, the native reader does not consume them automatically, and wiring that requires the credential provider bridge. See Iceberg's [S3 FileIO](https://iceberg.apache.org/docs/latest/aws/#s3-fileio) docs for the full property list, and [S3 Credential Providers](s3-credential-providers.md) for vended or per-request credentials.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Applied your wording in d531350.

Comment on lines +115 to +117
- Byte / Short `Add`, `Subtract`, and `Multiply` overflow raises `ARITHMETIC_OVERFLOW` (for
example `byte overflow`) where Spark raises `BINARY_ARITHMETIC_OVERFLOW`, and integral
`ARITHMETIC_OVERFLOW` messages omit Spark's `try_` suggestion.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is now the only bullet in the section without an issue link, since #5071 was closed by #5162 with these two items still open. #6169 already asks for a follow-up issue so this entry can link to it. Could you file that issue now and add the link in this PR? The text ships with 1.1.0, and a reader who hits the divergence has nowhere to follow it otherwise.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Filed #6217 for the two remaining items and linked it from the bullet in d531350.

@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.

Summary

  • Prior state and problem: The user guide contained stale installation, compatibility, tuning, and metrics descriptions ahead of 1.1.0.
  • Design approach: Updates 25 documentation files without changing executable code.
  • Correctness / compatibility analysis: Checked the full base-relative diff against Comet implementations and relevant Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0 sources. No introduced P1/P2 issues found within this review.
  • Key design decisions: Distinguishes native execution, JVM codegen dispatch, and Spark fallback while documenting experimental features and their opt-in settings.
  • Implementation sketch: Revises compatibility tables, deployment examples, plan-node names, metric descriptions, and upgrade guidance. This adds no runtime overhead or implementation abstractions.
  • Behavioral changes worth calling out: Documents JDK 17 requirements, cache and Iceberg-write configuration, nested shuffle keys, and revised CI coverage. Runtime behavior is unchanged.
  • Suggested improvements: Address the already-reported Kryo setup concern. With KryoSerializer and spark.kryo.registrationRequired=true, native broadcasts also need registration, even with caching disabled. CometKryoRegistrator, the startup warning, Spark’s registrations, and the existing broadcast test substantiate this unresolved P2 concern. It is not duplicated as a new finding.

Reviewed full head c473dbb2adc37fcd77592ff1227a233c08577b78 against base dd68a531c413c287c1f5fbe12ade4edb488900e5. The PR remains open and non-draft. Read all supplied reviews, comments, and threads.

Routed skill: review-comet-pr. Its documentation-only routing requires no sibling skills.

Exact-head CI: All executed checks passed, including Required Checks and Preflight. Fifteen jobs were skipped, including builds, Spark SQL, Iceberg, UDF, benchmarks, and site deployment.

Validation: git diff --check passed. All 19 new relative-link targets exist. An isolated Sphinx render including copied category templates produced 170 pages with zero errors and 20 warnings concerning existing or ungenerated links. Full Maven-generated documentation, JVM/native tests, Kubernetes deployment, and benchmarks were not run. Mermaid SVG rendering was bypassed. The project worktree is unchanged.

# Conflicts:
#	docs/source/user-guide/latest/datatypes.md
#	docs/source/user-guide/latest/iceberg-writes.md
#	docs/source/user-guide/latest/iceberg.md
#	docs/source/user-guide/latest/metrics.md
Document the Kryo registrator under "Additional Configuration" in the
installation guide. Native broadcast needs it with
spark.kryo.registrationRequired=true whether or not the in-memory cache
is enabled, so link to it from the cache docs rather than describing it
as a cache-only setting. Point the operator compatibility cache section
and the operators table at the in-memory cache page that landed on main,
instead of repeating it, which also drops a stale storage-format claim.

List every source Comet checks for the Iceberg S3 region, and link the
ANSI integral overflow divergence to apache#6217.

@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.

Summary

  • Prior state and problem: The user guide contained outdated installation, compatibility, tuning, and metrics information ahead of 1.1.0.
  • Design approach: Refreshes 26 documentation files. Reviewed the entire base-relative diff.
  • Correctness / compatibility analysis: Checked updated claims against Comet implementations and relevant Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0 sources. No introduced P1/P2 issues found within this review.
  • Key design decisions: Distinguishes native execution, JVM codegen dispatch, and Spark fallback. Centralizes Kryo setup in the installation guide and preserves experimental-feature qualifications.
  • Implementation sketch: Revises compatibility tables, deployment examples, configuration guidance, plan-node names, and metrics descriptions. No executable code, runtime overhead, or implementation abstractions are added.
  • Behavioral changes worth calling out: Documents existing JDK requirements, cache and Iceberg-write support, nested shuffle keys, and CI coverage. Runtime behavior is unchanged.
  • Suggested improvements: None at P1/P2. The existing Kryo registration, Iceberg region, and ANSI tracking feedback is addressed. No substantiated existing blockers remain.

Reviewed head d531350c67df88f96e24f0724b0535429b083de3 against base 88a1f48cc8a5c8f017016737c86f0912bbaefbe9. Confirmed the PR remains open and non-draft. Read the supplied reviews, comments, and threads.

Routed skill: review-comet-pr. Its documentation-only routing requires no sibling skills.

Exact-head CI: Seven checks succeeded, including Required Checks and Preflight. Fifteen jobs were skipped, including builds, Spark SQL, Iceberg, UDF tests, benchmarks, and site deployment. No failed or unfinished checks.

Validation: git diff --check passed. All 25 new relative links resolve. An isolated Sphinx render with copied category templates produced 172 HTML pages and 20 warnings for existing or ungenerated links. Full Maven-generated documentation, historical release content, Mermaid SVG rendering, JVM/native tests, Kubernetes deployment, and benchmarks were not validated. The project worktree remains unchanged.

@andygrove
andygrove added this pull request to the merge queue Sep 25, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Sep 25, 2026
Resolve the tuning.md conflict with apache#6194's wording for the paragraph on
pools shared by a task's native plans. Both sides removed the incorrect
claim that a native shuffle runs two native plans; apache#6194 also names the
operators that do run separate plans.

# Conflicts:
#	docs/source/user-guide/latest/tuning.md
@andygrove
andygrove added this pull request to the merge queue Sep 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Iceberg documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants