Expose native column encoder context - #6249
Conversation
Expose SyntaxInterface decoder snapshots and text decoding for jaspSyntax, add status-returning QML option parsing, and make quiet bridge logging actually discard output. Ensure the native ColumnEncoder is initialized before option encoding/decoding and catch decoder bridge failures at the C ABI boundary.
|
@JorisGoosen do you have time to review this? You're more familiar with the decoding stuff than I am. The basic idea is that we want to export its contents as json, so that we can reinstantiate the decoder in R in jaspSyntax. This is necessary when you save results to disk, restart R, and then load the results again. However, the round trip should be perfect and easy to maintain in the future (so you should understand what's going on here). |
| namespace | ||
| { | ||
| class NullLogBuffer : public std::streambuf | ||
| { | ||
| protected: | ||
| int overflow(int c) override { return traits_type::not_eof(c); } | ||
| }; | ||
|
|
||
| NullLogBuffer nullLogBuffer; | ||
| std::ostream nullLogStream(&nullLogBuffer); | ||
| } | ||
|
|
||
| std::ostream* Log::_nullStream = &nullLogStream; |
There was a problem hiding this comment.
Maybe this is a good idea but I will need to know why this was done?
There was a problem hiding this comment.
This is needed so logType::null actually discards output in the static jaspSyntax bridge. Before this change _nullStream pointed at std::cout, so syntaxBridgeSetVerbose(false) could still let native bridge logging reach stdout before Log::init() installed an application-owned null stream. The local streambuf is a true sink; verbose mode still switches the bridge back to stdout. I added a short code comment for that rationale.
There was a problem hiding this comment.
We could just call the following in the syntaxbridge constructor:
Log::logFileNameBase = (AppDirs::logDir() + "JASP " + getSortableTimestamp()).toStdString();
Log::init(&nullstream);
Log::setLogFileName(Log::logFileNameBase + " Desktop.log");
Log::setLoggingToFile(_preferences->logToFile());
Like MainWindow::initLog ?
There was a problem hiding this comment.
That is exactly the issue this was trying to fix: when the static bridge is used from R/jaspSyntax, MainWindow::initLog() is never called, so logType::null still pointed at std::cout and JASP/native messages leaked into R sessions even with verbose disabled.
I agree this is bridge initialization concern rather than part of the decoder API. Before I patch it: do you want the SyntaxInterface bridge to only install a local null stream via Log::init(&nullStream) and then toggle Log::setDefaultDestination() / Log::setWhere() from syntaxBridgeSetVerbose(), or do you want the bridge to do the fuller MainWindow::initLog()-style log-file setup as well?
My preference would be the small bridge-local null-stream initialization, without Desktop preferences/log-file setup, because the R/headless bridge should not depend on PreferencesModel or Desktop-owned logging policy.
There was a problem hiding this comment.
You could use a nullstream like the one in MainWindow::initLog. And then Log::init(&nullstream). Much simpler than the code in this PR. And ofc in the syntaxbridge.
There was a problem hiding this comment.
Done in c21e37f. I reverted the Common/log.cpp change entirely, so the global logging defaults outside SyntaxInterface are unchanged.
SyntaxInterface now owns a local null stream and calls Log::init(&nullstream) from configureBridgeLogging() before bridge status/error paths can log. syntaxBridgeSetVerbose() only toggles between cout and null; no Desktop preference/file logging is needed for this standalone bridge path. I also smoke-tested the jaspSyntax status-only error path with verbose = FALSE; it returns the structured error with zero captured native output.
JorisGoosen
left a comment
JorisGoosen
left a comment
There was a problem hiding this comment.
So this PR has some good ideas, but its mostly adding a lot of hassle on top of columnencoder.
Instead of adding all of these functions it would be a lot easier to simply store the output of DataSet::getColumnTypesMap() somewhere in your R datastructure (so you can reload it on demand). Then when you need to decode stuff you just use ColumnEncoder as usual after setting those columnTypes in setCurrentColumnNames.
Then there is almost no extra code to maintain and everything happens in the same way.
Or is there something Im missing that makes this obligatory?
FBartos
changed the title
|
jaspBase shouldnt be updated. The extra columnencoder related classes should be either in the file WITH columnencoder or in a separate class next to columnencoder. but not with syntaxbridge. |
|
@JorisGoosen addressed in
So the package side stores/passes an opaque context, and Desktop remains the only place implementing token replacement. I also re-ran the native build plus the focused jaspSyntax, jaspBase, and jaspTools bridge tests listed in the PR body. |
| return values; | ||
| } | ||
|
|
||
| static Json::Value decodeColumnTextJson(const Json::Value & values, const ColumnDecoderSnapshot & snapshot) |
There was a problem hiding this comment.
This whole function looks like it should just call setCurrentColumnNames first with the stuff from the snapshot. And than ColumnEncoder::decodeJson. Not really sure why we need to have this whole function.
There was a problem hiding this comment.
Agreed, this should be simplified. I can change the native path to parse the incoming JSON, temporarily install the captured context into ColumnEncoder, call the standard ColumnEncoder::decodeJson(), and restore the previous state before returning.
One detail to confirm: should this bridge call decode arbitrary JSON with replaceNames = true, or keep the current R contract as a character-vector JSON payload and still use decodeJson() internally? The former is more directly the standard ColumnEncoder API; the latter keeps the R API smaller and avoids exposing more surface than jaspSyntax currently needs.
There was a problem hiding this comment.
If it is the standard API for ColumnEncoder it is already accessible from R via jaspBase right? So that should make "The R API smaller" than the latter? Or am I missing something?
There was a problem hiding this comment.
You are not missing much; my phrasing about "R API smaller" was imprecise.
jaspBase::decodeColNames() / decodeName() are module-facing convenience helpers. They eventually resolve the decoder functions available in the current JASP runtime, but they are not a standalone way for jaspSyntax to install a saved Desktop ColumnEncoder context after the live dataset has changed. The dependency direction should also stay jaspBase -> jaspSyntax/native bridge for replay, not jaspSyntax -> jaspBase.
So I think the native bridge still needs one minimal context-aware decode entry point. But I agree it should be generic and standard-ColumnEncoder-shaped: context JSON + payload JSON in, install context, call ColumnEncoder::decodeJson(), restore, return decoded JSON. Then the exported R helper can remain a thin convenience wrapper for character vectors if that is all current callers need.
There was a problem hiding this comment.
Ok well im curious to see it.
There was a problem hiding this comment.
Done in c21e37f. The bridge-specific manual JSON decoding loop is gone.
decodeColumnJson() now parses the payload, installs the snapshot context with ScopedColumnEncoderContext, calls the standard ColumnEncoder::decodeJson(payload, replaceNames), and returns the decoded payload. The payload is no longer restricted to a string array, so the normal ColumnEncoder traversal remains the single implementation for decoded JSON objects.
There was a problem hiding this comment.
The standard decoder implementation is still the one we use, but replay needs to decode against the context captured from the source file/analysis, not whatever dataset context happens to be live when R materializes the result.
That is why I kept this as a single SyntaxInterface bridge operation instead of routing state management through jaspBase: the only cross-ABI API is decode(payload, context), and native code handles install/decode/restore internally. jaspSyntax remains a thin caller and jaspBase does not need to know about encoder mappings.
| return result.c_str(); | ||
| } | ||
|
|
||
| const char* STDCALL syntaxBridgeDecodeColumnText(const char* valuesJson, const char* decoderSnapshotJson) |
There was a problem hiding this comment.
Why isnt the decoder snapshot json not simply set from R? And the standard ColumnEncoder functions used? WHy all these extra functions?
There was a problem hiding this comment.
I think I understand the intended simplification: R should not carry mappings or use bridge-specific replacement logic; it should pass the captured native context back, native code should install that context into ColumnEncoder, and then call standard ColumnEncoder decode functions.
The point I want to clarify before patching is whether you prefer a persistent R-facing set-current-ColumnEncoder-context API, or a scoped decode call that installs the snapshot, calls ColumnEncoder::decodeJson(), and restores the previous native state before returning.
I prefer the scoped form because result replay often happens after the live dataset has changed, and persistent mutation from R is easier to misuse. But it would still remove the custom decode machinery and use the normal ColumnEncoder path. Is that aligned with what you intended?
Related detail: should the same context continue to carry the extra QML-option encoder state (JaspExtraOptions_...) and restore that alongside the dataset column encoder?
There was a problem hiding this comment.
Yes indeed, R should not carry mappings or bridge specific logic.
A scoped decode would require a bunch of extra interfacing functions right?
Why not simply make sure to set the right context from R? or is that where the scope would be?
There was a problem hiding this comment.
No extra public bridge functions should be needed. My intended scoped version is a single native decode call: R passes the captured context JSON and payload JSON, SyntaxInterface installs that context into ColumnEncoder for that call, calls ColumnEncoder::decodeJson(), and restores the previous context before returning.
So the scope is inside the native decode call, not an R-side sequence of set/decode/reset calls. That keeps the R API from leaving global native decoder state changed after replay, but still uses the standard ColumnEncoder path. I will implement it that way unless you prefer the persistent set-current-context API.
There was a problem hiding this comment.
SyntaxInterface installs that context into ColumnEncoder for that call, calls ColumnEncoder::decodeJson(), and restores the previous context before returning.
How?
There was a problem hiding this comment.
Mechanically:
- Parse the context JSON into the same
ColumnEncoder::colTypeMapthat came fromDataSet::getColumnTypesMap()plus the extra-options encoder map. - Save the currently active maps:
ColumnEncoder::columnEncoder()->currentNames()gl_extraEncodings->currentNames()
- Install the captured maps:
ColumnEncoder::setCurrentColumnNames(contextColumns)gl_extraEncodings->setCurrentNames(contextExtra)
- Parse the payload JSON and call the standard native decoder:
ColumnEncoder::decodeJson(payload, replaceNames)
- Restore the previously saved maps in an RAII guard/destructor before returning.
So the only custom code is context JSON <-> colTypeMap plus the scoped restore guard. The actual replacement would be ColumnEncoder::decodeJson(), not a separate bridge-specific decoding loop.
There was a problem hiding this comment.
Implemented in c21e37f with the scope inside the single native bridge call.
R/jaspSyntax still passes only the encoded payload plus the opaque encoder context. SyntaxInterface parses that context, ScopedColumnEncoderContext saves the current ColumnEncoder state, installs the snapshot names/types on ColumnEncoder and the extra encoder, calls the standard ColumnEncoder::decodeJson(), then restores the previous native state before returning. So R does not carry mappings and there are no separate set/unset calls exposed across the ABI.
|
@JorisGoosen I pushed c21e37f with the changes discussed in the threads. Summary:
I rechecked the three review threads after the push; they are now outdated and have implementation replies. Ready for another look. |
|
|
||
| static colVec columnNames(); | ||
| static colVec columnNamesEncoded(); | ||
| static const char* extraOptionsPrefix() { return "JaspExtraOptions_"; } |
There was a problem hiding this comment.
This is now a double definition. Very ugly. Should be moved to DataBridge instead. That way it covers both engine and syntaxbridge
There was a problem hiding this comment.
Done in 3c0575c. I moved this out of ColumnEncoder and made DataBridge the single owner of the extra-options encoder.
Concretely:
- removed
ColumnEncoder::extraOptionsPrefix() - added a bridge-owned extra encoder in
DataBridge - removed
Engine::_extraEncodings - removed the separate SyntaxInterface
gl_extraEncodings - changed
rbridge_init()to derive the extra encoder from the activeDataBridge, andrbridge_setDataBridge()now refreshes that borrowed pointer whenever SyntaxInterface recreates its DataBridge
So Engine and SyntaxInterface now share the same ownership model through DataBridge, and SyntaxBridge only borrows the encoder for context capture/decode.
|
@JorisGoosen I pushed 3c0575c to address the remaining DataBridge ownership comment. The extra-options encoder is now owned by Validation after the push:
The previous review threads are now outdated with implementation replies. Ready for another look. |
| class SyntaxBridgeNullBuffer : public std::streambuf | ||
| { | ||
| protected: | ||
| int overflow(int c) override { return traits_type::not_eof(c); } | ||
| }; | ||
|
|
||
| SyntaxBridgeNullBuffer gl_nullLogBuffer; | ||
| std::ostream gl_nullLogStream(&gl_nullLogBuffer); |
There was a problem hiding this comment.
Why not just use the boost null stream we had before?
Or, if this is somehow better, why dont we replace the boost nullstream with something like this?
There was a problem hiding this comment.
Done in 647fde9. I switched SyntaxInterface to the same boost::iostreams::null_sink pattern used by Engine/MainWindow and removed the local std-only null stream implementation.
One build detail: SyntaxInterface now enables /EHsc on MSVC. Without that, Boost leaves boost::throw_exception unresolved for the null stream path in this target. After that change, SyntaxInterface rebuilds cleanly.
Validation:
- rebuilt
SyntaxInterface - verified SyntaxInterface exports against
jaspSyntax - ran focused jaspSyntax bridge contract test
- smoke-tested
verbose = FALSEstatus-error path: zero captured native output
|
|
||
| bool init(bool dbInMemory) | ||
| { | ||
| configureBridgeLogging(gl_verbose); |
There was a problem hiding this comment.
I see configureBridgeLogging here. Which is the logical place! Good. But why is it at all those other places?
There was a problem hiding this comment.
Agreed. Those extra calls were defensive leftovers from the earlier logging-leak fix for pre-init status/error paths, but they make the ownership less clear.
I pushed 1766f05 to simplify this:
configureBridgeLogging()is now only called frominit()as the central setup pointsyntaxBridgeSetVerbose()only reconfigures if logging was already initialized, so changing verbosity after init still works- pre-init/status-only failures now just return structured status errors instead of logging through
Log::log()before initialization
Validation:
- rebuilt
SyntaxInterface - verified SyntaxInterface exports against jaspSyntax
- ran the focused jaspSyntax desktop bridge contract test
- smoke-tested the pre-init
verbose = FALSEerror path; it still captures zero native output
|
It compiles and the code looks good now! Im just missing some unittest(s) in Tests/ |
jasp-stats#6235 was a duplicate of jasp-stats#6249, but adds some small extra changes. This commit add these extra changes
4 participants
Summary
This PR exposes the Desktop-owned encoder context needed by the jaspTools -> jaspSyntax bridge:
ColumnEncodercontextCommon/columnencodercontext.*, next toColumnEncoderColumnEncoderstate and then calling the normal decoder pathdecodeAllWithMapping()compatibility path so R cannot silently decode from a stale mapWhy
Encoding/decoding should remain an internal Desktop/SyntaxInterface concern. The R packages now pass around an opaque encoder context, and Desktop remains the only implementation of token replacement. That prevents incorrect replay when the live dataset has changed, and it avoids duplicating encoding rules in jaspSyntax, jaspBase, or jaspTools.
Related PRs
Validation
SyntaxInterfacewithninja -C build -j1 SyntaxInterfacethrough the local MSVC toolchaintools/check-syntaxinterface-symbols.shtest-desktop-jasp-contract.R,test-dataset-helpers.Rtest-result-object-decoding.R,test-runWrappedAnalysis.Rtest-jaspSyntax-lifecycle.Rgit diff --checkNote: this PR should be reviewed together with the linked package PRs because the contract is intentionally split by ownership: Desktop owns token replacement, jaspSyntax exposes the bridge, jaspBase decodes JASP-owned result surfaces, and jaspTools carries the captured context through replay.