Repository navigation
docs: apply the style guide's "native" rules to the remaining pages - #7477
Open
Abhishek-Mohan wants to merge 11 commits into
Open
Abhishek-Mohan wants to merge 11 commits into
Abhishek-Mohan wants to merge 11 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
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, and the.doc(...)strings inCometConf.scalaandcontrib/delta/.../DeltaConf.scala. 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.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.