Skip to content

fix: preserve byte renderer fallback context#844

Merged
stephenamar-db merged 1 commit into
databricks:masterfrom
He-Pin:fix/byte-renderer-context
May 12, 2026
Merged

fix: preserve byte renderer fallback context#844
stephenamar-db merged 1 commit into
databricks:masterfrom
He-Pin:fix/byte-renderer-context

Conversation

@He-Pin
Copy link
Copy Markdown
Contributor

@He-Pin He-Pin commented May 12, 2026

Motivation:
ByteRenderer's direct materialization path falls back to the generic stackless materializer for very deep arrays/objects. That fallback previously started via Materializer.apply0, which creates a fresh MaterializeContext and drops the objects already being materialized on the active ByteRenderer path.

Key Design Decision:
Reuse the active MaterializeContext when switching from ByteRenderer's direct recursive path to stackless materialization. This keeps visitedObjects, sort behavior, assertion logic, and materialization limits consistent across the fallback boundary.

Modification:

  • Change deep array/object ByteRenderer fallback to call Materializer.materializeStackless(v, this, ctx).
  • Widen materializeStackless to private[sjsonnet] so ByteRenderer can reuse it without exposing it publicly.
  • Add a regression test that proves the old path duplicated partial output before detecting a recursive object, while the fixed path preserves the outer cycle context.

Benchmark Results:
N/A — correctness-only split from render optimization review material. No performance claim is made for this PR.

Analysis:
The old fallback still eventually reported the same recursive materialization error, but it did extra work with a fresh context first. The regression test uses a recursive object with a large string field and a low fallback threshold: the fixed path flushes one copy of the large field before detecting the cycle, while the old path re-rendered the field again before failing.

Validation:

  • ./mill --no-server --ticker false --color false -j 1 'sjsonnet.jvm[3.3.7]'.test.testOnly sjsonnet.RendererTests passed.
  • ./mill --no-server --ticker false --color false -j 1 'sjsonnet.jvm[3.3.7]'.test passed.
  • ./mill --no-server --ticker false --color false -j 1 __.test passed.
  • ./mill --no-server --ticker false --color false -j 1 __.checkFormat passed.

References:

Result:
ByteRenderer fallback now preserves recursive-object detection context across the direct-to-stackless transition without changing external APIs or successful rendering semantics.

Motivation:
ByteRenderer's direct materialization fallback could restart stackless materialization with a fresh MaterializeContext, losing the objects already being rendered on the active path. That delayed recursive-object detection and could duplicate partial output before reporting the cycle.

Modification:
Thread the existing MaterializeContext into the stackless fallback for deep arrays and objects, and widen materializeStackless to package-private so ByteRenderer can reuse it. Add a regression test that distinguishes the fixed behavior from the old duplicate-render path.

Result:
Deep ByteRenderer fallback keeps cycle tracking and materialization settings consistent with the surrounding render pass while preserving the existing recursive error behavior.

References:
Source material split from render optimization review in PR databricks#776.
@He-Pin He-Pin marked this pull request as ready for review May 12, 2026 06:38
@He-Pin He-Pin marked this pull request as draft May 12, 2026 06:59
@He-Pin He-Pin marked this pull request as ready for review May 12, 2026 08:31
@stephenamar-db stephenamar-db merged commit cedc083 into databricks:master May 12, 2026
5 checks passed
@He-Pin He-Pin deleted the fix/byte-renderer-context branch May 12, 2026 20:30
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.

2 participants