Skip to content

docs: document C2R cost for wide/nested schemas in tuning guide - #5458

Merged
sunchao merged 2 commits into
apache:mainfrom
DebadityaHait:docs/c2r-wide-nested-cost
Aug 25, 2026
Merged

sunchao merged 2 commits into
apache:mainfrom
DebadityaHait:docs/c2r-wide-nested-cost

Conversation

@DebadityaHait

Copy link
Copy Markdown

Which issue does this PR close?

Closes #5455.

Rationale for this change

The tuning guide documents the transitionRevert mechanism but never mentions schema shape as the cost driver. As reported in #5455, columnar-to-row conversion becomes very expensive for wide or deeply nested schemas, where reverting the entire stage can be cheaper than paying repeated transitions.

What changes are included in this PR?

Extends the "Reducing Row/Columnar Conversion Overhead" section of the tuning guide with one short paragraph covering schema-shape-driven C2R cost and earlier-revert guidance using full config keys.

How are these changes tested?

Docs-only change; validated with a clean local Sphinx build of the site (no warnings for the changed file). Config keys and defaults match their definitions in CometConf.scala.

…he#5455)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

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

Thanks @DebadityaHait lets have this note in separate paragraph under h3 heading, please make sure to emphasize, its not a stage fallback, rather the entire plan fallback, because c2r for complex deep schemas might be very expensive

@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

This documentation-only change connects schema-dependent columnar-to-row conversion cost to Comet's existing transition-revert controls. I found no P1/P2 issue in the complete diff from ec0f7975dcef8e658a39b273cd491352147ccd66 to 3849ef6d4c9741da99fbd985640f47d237c9cb1e; it does not change execution code or configuration defaults.

Prior state and problem

The tuning guide already explains why repeated row/columnar transitions can outweigh native execution benefits, but it does not identify wide and deeply nested schemas as a source of additional conversion work. Readers investigating those workloads could therefore miss a useful reason to profile transitions and consider an earlier fallback.

Design approach

The new paragraph stays alongside the existing transition-revert explanation and provides both fully qualified configuration keys. Its recommendation is conditional on profiling showing conversion-dominated execution, and “can be cheaper” correctly leaves the performance decision workload-dependent rather than promising a universal improvement.

Correctness / compatibility analysis

The keys and defaults match the exact-head configuration definitions: reversion is disabled by default and the maximum is 2. The rule uses a strict transitionCount > maxTransitions comparison, so setting the maximum to 1 permits zero or one C2R and reverts a stage with at least two.

The current implementation is stage-scoped in both AQE and non-AQE execution: transition counting and rewriting stop at query-stage, shuffle, and broadcast boundaries. Thus the paragraph's “stage” wording agrees with the pinned implementation; these settings do not unconditionally revert every stage in the whole query.

Key design decisions

Including the enable switch is important because changing the threshold alone has no effect while the rule is disabled. Lowering the threshold is an explicit tuning choice, while retaining the profiling prerequisite accounts for native-execution benefits, fixed conversion costs, and differences between JVM/native conversion paths.

Implementation sketch

The patch adds eight lines to docs/source/user-guide/latest/tuning.md and changes no other file. A Markdown rendering check confirms one separate paragraph with all three inline-code spans intact and surrounding headings unchanged.

Validation included the full pinned diff, configuration/rule/conversion-path source inspection, and that rendering check. CI has five successful checks and twelve skipped checks, including runtime suites and site deployment; I did not independently run a full Sphinx build or a performance benchmark.

Behavioral changes worth calling out

There is no runtime behavior change until a reader explicitly applies the documented settings. Opting in can give up native execution in qualifying stages, and required format bridges at stage boundaries may remain, so the advice should not be read as eliminating every conversion in the query.

Suggested improvements

The existing suggestion to give this schema-specific advice its own h3 is a reasonable optional organizational improvement, not a correctness blocker. Any wording revision should retain the distinction between a per-stage C2R threshold and unconditional whole-query fallback.

@sunchao

sunchao commented Aug 25, 2026

Copy link
Copy Markdown
Member

@DebadityaHait could you address the comment from @comphead in the above? after that I'll merge this. Thanks

…back

Address review feedback: give the wide/nested-schema guidance its own h3
subsection and clarify that transitionRevert falls back the entire plan to
Spark row-based execution rather than leaving a native/fallback mix.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@DebadityaHait

Copy link
Copy Markdown
Author

Thanks @comphead @sunchao — addressed in d13c7135: the guidance now sits under its own h3 heading ("Entire-Plan Fallback for Wide or Deeply Nested Schemas") and states explicitly that the fallback applies to the entire plan, with Comet removing its native operators rather than leaving a mix of native and fallback operators joined by repeated conversions.

@sunchao
sunchao merged commit f171782 into apache:main Aug 25, 2026
16 checks passed
@sunchao

sunchao commented Aug 25, 2026

Copy link
Copy Markdown
Member

Thanks, merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document C2R cost for wide/nested schemas

3 participants