Skip to content

Expose Desktop encoder context for result decoding#8

Merged
boutinb merged 11 commits into
jasp-stats:masterfrom
FBartos:fix/decode-embedded-result-column-names
Jun 10, 2026
Merged

Expose Desktop encoder context for result decoding#8
boutinb merged 11 commits into
jasp-stats:masterfrom
FBartos:fix/decode-embedded-result-column-names

Conversation

@FBartos

@FBartos FBartos commented May 26, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR updates jaspSyntax to expose the Desktop-native encoder context instead of implementing or preserving R-side column-token replacement:

  • replaces columnDecoderSnapshot() / columnMapping() with columnEncoderContext()
  • keeps decodeColumnText() as the R-facing decoder, but token decoding always goes through the native SyntaxInterface bridge
  • persists the opaque encoder context with loaded dataset state for later analysis-result replay
  • updates decodeAnalysisResults() to use the context for column-name tokens and transient requested-dataset factor maps for factor-level decoding
  • removes the old R mapping fallback so native decoder failures are surfaced as bugs instead of silently producing possibly wrong labels
  • hardens source-checkout subprocess loading so callr does not preload an installed jaspSyntax/SyntaxInterface before the requested source checkout is loaded
  • prefers JASP_BUILD_DIR DLLs over copied source DLLs when testing against a live Desktop build

Why

The encoding rules belong to Desktop/SyntaxInterface. jaspSyntax now only transports the source-state context and calls the native decoder. This keeps replay deterministic for old .jasp files and prevents the R layer from inventing partial mappings that can drift from Desktop behavior.

Related PRs

Validation

  • rebuilt local Desktop SyntaxInterface and ran jaspSyntax focused tests against that build
  • test-desktop-jasp-contract.R passed, with the expected gated Descriptives replay skip
  • test-dataset-helpers.R passed
  • R parse check for all R/ and tests/testthat/ files passed
  • git diff --check

FBartos and others added 6 commits May 27, 2026 09:12
loadQmlAndParseOptions now calls syntaxBridgeLoadQmlAndParseOptionsStatus,
which returns a JSON status object following the same pattern as other
bridge functions. Errors from the QML form (e.g. unresolved formula terms)
are surfaced via Rcpp::stop instead of being silently swallowed as an
empty return value.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add serializable column decoder snapshots and decodeColumnText() so embedded JASP column tokens are replaced by SyntaxInterface's ColumnEncoder rules. Prefer the native decoder for dataset helpers and result decoding, while keeping a shallow mapping fallback for explicit snapshots/tests. Harden bridge subprocess result handoff and cover empty snapshots plus the public contract in focused tests.
@FBartos

FBartos commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

@vandenman I pushed the follow-up implementation for the decoder ownership change.

The current shape is now: desktop/SyntaxInterface owns token replacement via captured ColumnEncoder snapshots (jasp-desktop#6249), jaspSyntax exposes that through columnDecoderSnapshot() / decodeColumnText(), and jaspBase decides which JASP-owned result surfaces are decoded (jaspBase#204).

A few details that address the earlier concerns:

  • native decoding is preferred; the legacy global decoder is only a compatibility fallback
  • empty decoder snapshots are valid snapshots and do not fall through to live native state
  • explicit named mappings remain supported only as shallow fallback/test input
  • decodeAnalysisResults() avoids mutating opaque classed/S4/call objects
  • subprocess handoff now writes a result RDS before native teardown so successful native results are not lost to teardown noise

Validation on Windows/R 4.6: rebuilt SyntaxInterface.dll, verified 16 declarations/exports, reran the focused jaspSyntax tests, reran the focused jaspBase tests, and reran the MixedModelsLMM
es() smoke with patched jaspBase in a temporary library.

Comment thread R/columnDecoder.R Outdated
if (.hasColumnDecoderFallback(decoderSnapshot)) {
return(.decodeColumnTextFallback(text, decoderSnapshot))
}
stop(e)

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.

Good: decodeColumnTextNative genuinely call the C++ decoding functionality!
Bad: .decodeColumnTextFallback. This reimplements the functionality in R. Do we even want this function to exist?

@FBartos & @boutinb what do you think? If we always create the snapshotJson on the fly, then I think we should be able to guarantee this? IF we can guarantee this then I think the whole fallback idea is somewhat overengineered and also as a liability. If the fallback is triggered, it means we have "broken" the guarantee which should just error. If the fallback is triggered, it does the decoding differently from how JASP does it. That can result in silently incorrect decoding of results, which I find far worse than showing e.g., encoded results as a fallback with a warning or so.

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.

Yes I agree, better an error than a wrong result.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed with this direction. The fallback is gone now: decodeColumnText() always dispatches encoded-token decoding to native SyntaxInterface, and native failures are treated as bugs instead of triggering an R replacement path. The old snapshot/mapping contract was replaced with an opaque Desktop ColumnEncoder context, so jaspSyntax carries provenance but does not implement token replacement itself.

@FBartos

FBartos commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the silent fallback concern in 2b2d73f.

What changed:

  • decodeColumnText() now always relies on the native SyntaxInterface decoder for encoded bridge tokens; captured mappings are only serialized into native decoder snapshots.
  • Removed the R-side mapping/gsub fallback after native decode failure.
  • Removed legacy .decodeColNamesStrict / .encodeColNamesStrict fallback paths from dataset/header mapping helpers.
  • decodeColumnNames() now errors if native decoding fails or leaves an encoded bridge name unchanged.
  • decodeAnalysisResults() no longer suppresses decoder setup failures; mappings are passed to native decoding rather than used as an R replacement decoder.

This keeps the architecture aligned with jasp-desktop #6249: token replacement lives in native ColumnEncoder; jaspSyntax is just the R bridge and serialized context carrier. Related jaspBase enforcement is in jaspBase #204.

Focused checks passed locally with the source DLL path on PATH:

  • test-dataset-helpers.R
  • test-desktop-jasp-contract.R

@FBartos FBartos requested review from boutinb and vandenman June 2, 2026 15:15
@FBartos FBartos changed the title Decode embedded column names in analysis results Expose Desktop encoder context for result decoding Jun 3, 2026
@FBartos

FBartos commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

@vandenman I pushed the architecture-level follow-up from the decoding review.

The old snapshot/mapping contract is gone. jaspSyntax now carries an opaque Desktop ColumnEncoder context and always dispatches token decoding to the native SyntaxInterface bridge; R-side mapping fallback was removed. I also fixed the source-checkout subprocess loader so it does not preload an installed jaspSyntax/SyntaxInterface before loading the requested source checkout, and it prefers JASP_BUILD_DIR DLLs when testing against a live Desktop build.

Related PRs: jasp-desktop jasp-stats/jasp-desktop#6249, jaspBase jasp-stats/jaspBase#204, jaspTools jasp-stats/jaspTools#79.

@boutinb boutinb merged commit 7b68373 into jasp-stats:master Jun 10, 2026
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.

3 participants