Repository navigation
docs: apply the style guide's "native" rules to the remaining pages - #7477
Open
Abhishek-Mohan wants to merge 12 commits into
Open
Abhishek-Mohan wants to merge 12 commits into
Abhishek-Mohan wants to merge 12 commits into
Conversation
Replace the phrases the style guide rules out (N1, N3) with the term each one means: "runs natively", "not handled natively", "the native path", "a native path" and "ran natively" become what Comet does in Rust, and "native expression", "native spelling" and "native version" name the Rust expression, the `Tz::from_str` parser and the native library's IANA database version. The `date_format` sentence also said Comet uses `to_char` only in UTC sessions. `CometDateFormat` also uses it in other sessions when `spark.comet.expression.DateFormatClass.allowIncompatible=true`, so the sentence now says so. Part of apache#7358. Generated-by: OpenCode
github-actions
Bot
added
the
documentation
The codegen dispatcher paragraph said the serdes route "the cases their Rust implementation cannot handle" through the dispatcher. `make_timestamp` (`CometMakeTimestamp`) and `to_timestamp` with a format (`GetTimestamp`, `CometGetTimestamp`) have no Rust implementation: both extend `CometCodegenDispatch` and always dispatch. `to_timestamp` without a format is rewritten by Spark to a `Cast`, which runs in Rust. Say so. Found with the review-comet-docs skill's judgment checks. Part of apache#7358. Generated-by: OpenCode
The `date_format` sentence said Comet uses `to_char` outside UTC sessions when `allowIncompatible` is set, but not what that does. `CometDateFormat.convert` emits `to_char` without a timezone, and every `TimestampType` input is labelled "UTC", so the result is formatted in UTC. Say so. The list of Spark settings Comet does not handle said "Comet" as a whole. The Delta scan does rebase legacy-calendar dates and timestamps (`datetime_rebase.rs`, and `parquet.md`), so scope the list to the native Parquet scan. Part of apache#7358. Generated-by: OpenCode
Move every page under docs/source, README.md, and the `.doc(...)` strings in CometConf.scala and DeltaConf.scala to the style guide's rules for "native" (N1-N5). Each replacement names the meaning the code supports: "in the Comet pipeline" / "runs in Comet" (T1) where the text means "did not fall back to Spark", "Rust-implemented" / "in Rust" (T3) where it means Comet's Rust code, and "the Rust side" where "native" was a noun (N4). Allowed compounds (native shuffle, native scan, native writer, native memory, native library, native implementation, and in the contributor guide native code, native plan and the JVM/native boundary) are unchanged, as are identifiers, config keys, log lines and plan output (N6). Where a sentence was already wrong, fix it: - installation.md called the broadcast exchange native; it is JVM-side. - adding_a_new_operator.md said every `nativeExecs` operator runs entirely in Rust; the local table and in-memory cache scans do not. - adding_a_new_expression.md said an unsupported expression makes Spark "fall back to its native execution"; describe the fallback and the codegen dispatch route. - adding_a_new_expression.md tied the lossy UTF-8 decoder to native shuffle; it is used by JVM shuffle's row-to-Arrow conversion. - development.md listed CI suite groups that no longer exist. - plugin_overview.md described a v2 Parquet path and a custom vectorized reader that no longer exist. - timezones.md said known bugs are tracked in apache#6335, which is closed. - compatibility/index.md listed DECIMAL RANGE frames as diverging; Comet now falls back for them. - compatibility/expressions/_category_template/cast.md said Comet leaves an unsupported cast to Spark; it uses codegen dispatch. - the `spark.comet.debug.enabled` doc said arrays are validated on import; they are validated before export to the JVM. - pyarrow-udfs.md described a per-partition native worker; the UDF runs in an embedded interpreter on the task thread. Headings that used a ruled-out phrase are renamed. No page in the repository links to any of the old anchors. Not included: sql-file-tests.md, which apache#7333 rewrites, and the expression support-level strings in the serdes, which also print as fallback reasons in plans. Closes apache#7358. Generated-by: OpenCode
Fix sentences that named the wrong kind of operator or lost accuracy: - adding_a_new_operator.md: the `nativeExecs` map also holds the local table, in-memory cache and empty relation scans, which are JVM-side; name the map rather than calling every entry Rust-implemented. - operators.md: an unsupported operator stays in Spark with transitions around it; the stage does not fall back as a whole unless transition revert is enabled. `CollectMetricsExec` feeds any Comet operator, and typed Dataset operators stay in Spark. - CometConf: the strict fallback check sees Comet operators as children, not only Rust-implemented ones; the UDF library ABI is language-neutral, so do not call the libraries Rust UDFs; call the `explain.native` output the Rust plan, as metrics.md and understanding-comet-plans.md do. - datasources.md: the build-side bridge is for scans without a native scan, and the probe side must use native scans. - json.md and regex.md: finish the "Rust path" / "Rust engine" labels, and say that unsupported cases route through the codegen dispatcher rather than falling back to it (T5). - cast.md: keep "native implementation of" together (N2). - expressions.md: the regr_*, slice and luhn_check notes say "runs in Rust" instead of using "Native" outside the Implementation column. - pyarrow-udfs.md: name `CometNativeException`. - timezones.md: the apache#6335 epic links issues that are still open. Revert edits the guide did not require: N3 compounds in the contributor guide (native planner, native function arguments, native guard, native frames, native allocations, native reservations), the "native stream" debugging heading, the roadmap heading that issues link to by anchor, the proposal record in top_level_project_proposal.md, and "reads" -> "scans" where the matching headings keep "reads". Use plain "handoff" in iceberg-scheduler-failures.md. Part of apache#7358. Generated-by: OpenCode
Abhishek-Mohan
changed the title
- in-memory-cache.md: with Comet operators off at startup, Spark can still read Comet's cache format; the plugin keeps Spark's format so that caches are not stored in Comet's format with only Spark to read them (Plugins.maybeSetCacheSerializer). - faq.md: Rust-implemented operators and JVM shuffle pages share the off-heap pool; Comet's JVM Arrow allocator does not. - operators.md: name the transition-revert config in the intro. - metrics.md: "that connector" read as the HDFS path; name Comet's Rust S3 connector. - json.md: the JSON kernels are Comet's own Rust code, not DataFusion's. - CometConf: `spark.comet.exec.enabled` leaves Comet shuffle on. Part of apache#7358. Generated-by: OpenCode
The default `query` mode checks that every operator is a Comet operator (`checkSparkAnswerAndOperator`), so it checks that the query runs in Comet (T1). `expect_native` checks that an expression ran in Rust rather than through codegen dispatch (T3). Fix two claims while editing them: - The example showed binary `bit_length` routing through the codegen dispatcher. `CometBitLength` marks binary input `Unsupported` and has no `CodegenDispatchFallback`, so it falls back to Spark. Use `lower`, which dispatches unless `spark.comet.caseConversion.enabled=true`, as `lower.sql` asserts. - The constant-folding note said all-literal queries are evaluated by Comet. Other optimizer rules and Comet's plan-time literal handling can still remove them, so say so. This was the last page apache#7358 lists. Part of apache#7358. Generated-by: OpenCode
…ative # Conflicts: # docs/source/contributor-guide/floating_point.md # docs/source/user-guide/latest/operators.md # spark/src/main/scala/org/apache/comet/CometConf.scala
…ative # Conflicts: # docs/source/contributor-guide/roadmap.md # docs/source/user-guide/latest/iceberg.md
comphead
reviewed
Nov 11, 2026
comphead
left a comment
comphead
left a comment
Contributor
There was a problem hiding this comment.
I spot-checked the reworded claims against the code (CometConf, operators, expressions, Iceberg, memory) and found no change in meaning, config key or default. One heads up: GenerateDocs.scala still emits "selected for native execution automatically" into the generated expression compatibility pages, so those pages will keep the old term until the generator changes.
| later gains a native path (or loses its dispatcher route) fails here instead of passing unnoticed. | ||
| Prefer the SQL file equivalents, `expect_native(...)` and `expect_dispatch(...)`, when the test | ||
| fits in a fixture. | ||
| later gains a native implementation (or loses its dispatcher route) fails here instead of passing |
Contributor
There was a problem hiding this comment.
The bit_length example a few lines above has the same problem that this PR fixes in sql-file-tests.md. CometBitLength returns Unsupported for BinaryType and does not mix in CodegenDispatchFallback, so SELECT bit_length(b) falls back to Spark (bit_length.sql pins it with expect_fallback(bit_length on BinaryType is not supported)). The dispatched = Seq("bit_length") assertion would fail as written. The test name "stays native on string" also still uses a phrase that N1 rules out. Could this example use an expression that really splits by mechanism, for example lower with and without spark.comet.caseConversion.enabled, as the SQL file page now does?
Contributor
Author
There was a problem hiding this comment.
Note
🤖 Claude Opus 5.5 responding on behalf of @Abhishek-Mohan
Switched the example to lower: it uses codegen dispatch by default and runs in Rust with spark.comet.caseConversion.enabled=true, as lower.sql and lower_enabled.sql check. The sentence above it now covers a config-based split too, and the test name no longer says "native".
| (`spark.comet.shuffle.native.maxBufferBytes=0`) it spills only when the memory pool denies an allocation, so it holds | ||
| as much of the task's share of the off-heap pool as it can get. That can leave a sort or an aggregation in the same | ||
| task without memory, and the large buffers it frees at the end of the task can stay resident in the native allocator, | ||
| task without memory, and the large buffers it frees at the end of the task can stay resident in the memory allocator, |
Contributor
There was a problem hiding this comment.
Dropping "native" leaves "the memory allocator", which a reader could take to mean the JVM's. tuning/memory.md explains that this is the system allocator used by Comet's Rust code, and the spark.comet.memory.logInterval doc says "the Rust allocator". Would "stay resident in the Rust allocator" keep the meaning and match the other pages?
Contributor
Author
There was a problem hiding this comment.
Note
🤖 Claude Opus 5.5 responding on behalf of @Abhishek-Mohan
Changed to "the Rust allocator".
- adding_a_new_expression.md: the `checkSparkAnswerAndImpl` example expected binary `bit_length` to use the codegen dispatcher, but `CometBitLength` returns `Unsupported` for `BinaryType` without `CodegenDispatchFallback`, so it falls back to Spark. Use `lower`, which dispatches by default and runs in Rust with `spark.comet.caseConversion.enabled=true`, as `lower.sql` and `lower_enabled.sql` do. - tuning/shuffle.md: name the Rust allocator, as tuning/memory.md and the `spark.comet.memory.logInterval` doc do. - GenerateDocs: the expression compatibility header said "not selected for native execution automatically" (N1); say "where Comet does not select its native implementation automatically". GenerateDocsSuite expects the new text. Part of apache#7358. Generated-by: OpenCode
Contributor
Author
|
Note 🤖 Claude Opus 5.5 responding on behalf of @Abhishek-Mohan Thanks for checking. The |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #7358.
Rationale for this change
The documentation style guide from #7151 rules out "runs natively", "the native path", "native operator", "native execution" and similar phrases (
N1), allows "native" before a code noun only in the contributor guide (N3), and never as a noun (N4). #7358 tracks moving the existing pages and the published config descriptions over. This PR does that in one pass, so later edits to these pages start from text that already follows the guide.What changes are included in this PR?
Wording (
N1–N5). Each phrase the guide rules out is replaced with the term the code supports:T1) where the text means "did not fall back to Spark". Codegen dispatch counts as staying in Comet.T3) where it means Comet's Rust code. JVM-side Comet operators, such as the broadcast exchange and the in-memory cache scan, are not called Rust-implemented.N4), and "Spark's built-in shuffle" for Spark's own behavior (N5).Allowed uses are unchanged: native shuffle, native scan/reader/writer, native memory, native library, native implementation, and in the contributor guide native code, native plan and the JVM/native boundary. Identifiers, config keys, log lines and plan output are unchanged (
N6).Scope. Every page under
docs/source(user guide, contributor guide, about),README.md, the.doc(...)strings inCometConf.scalaandcontrib/delta/.../DeltaConf.scala, and the one generated header inGenerateDocs.scalathat used a ruled-out phrase ("not selected for native execution automatically"), withGenerateDocsSuiteupdated to match. Only string literals change in the Scala files. This covers every item on #7358's list, plus 16 files with the same wording that the issue's search missed.compatibility/expressions/spark-3.4/index.mdwas removed frommainwith Spark 3.4 support (#7521).Claims that were wrong, fixed while editing the sentence (
P16):installation.mdoperators.mdadding_a_new_operator.mdnativeExecsoperator runs in Rustadding_a_new_expression.mdadding_a_new_expression.mddevelopment.mdfuzz,parquet,csv,sqlplugin_overview.mdtimezones.mdtimezones.mddate_formatruns natively only in UTCallowIncompatible, formatted in UTCtimezones.mdmake_timestamp, andto_timestampwith a format, always dispatchcompatibility/index.mdDECIMALRANGE frames diverge_category_template/cast.mdspark.comet.debug.enabledadding_a_new_expression.mdcheckSparkAnswerAndImplexample expects binarybit_lengthto dispatchlowerwith and withoutspark.comet.caseConversion.enabledsql-file-tests.mdbit_lengthroutes through the codegen dispatcherlower, which dispatches by defaultsql-file-tests.mdHeadings. 15 headings that used a ruled-out phrase are renamed. Nothing in the repository links to their anchors, and a search of issues, PRs and comments found no links to them either. The roadmap heading "Native Coverage for Codegen-Dispatched Expressions" keeps its name, because seven issues and PRs link to its anchor.
contributor-guide/sql-file-tests.mdoverlaps with #7333, which rewrites some of the same lines. Its constant-folding note here uses #7333's wording, so whichever PR merges second has a small conflict to resolve.Not included:
serde/*.scala). They also print as fallback reasons in plans and are not listed in docs: move existing pages to the style guide's "native" rules #7358.top_level_project_proposal.md.How are these changes tested?
Documentation only, plus
.docstring literals.T1orT3, and each corrected claim against the code (paths in the commit messages).review-comet-docsskill on the branch. NoN1phrase remains in changed lines, and no N3-only compound (native code, native plan…) remains in changed lines outside the contributor guide. Every config key, operator name and file named in changed lines exists.-n) atmainand at the head: 32 warnings on each, none new. Each changed page has one H1, exceptplugin_overview.md, which already had two.prettier --checkpasses on every changed page../mvnw spotless:checkpasses. No test reads the changed config descriptions.Council review
Three models reviewed the sweep independently against the style guide and the code, in two rounds. Round 1 found operators described as Rust-implemented that run on the JVM, codegen dispatch called a fallback, half-renamed labels, and edits the guide did not require, which are reverted. Round 2 confirmed those fixes and found six smaller wording issues and one row of this description. The two commits after the sweep fix all of them.
sql-file-tests.mdwas added after the council rounds and checked with the same skill and build.AI assistance: written with Claude Opus 5.5 through OpenCode, and reviewed by Claude Opus 5.5, Gemini 4 Argon and GPT-6 Astra in T3 Code.