-
-
Notifications
You must be signed in to change notification settings - Fork 234
Expose native column encoder context #6249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
72ebc4e
7a0fa75
ed7c12e
c21e37f
3c0575c
647fde9
1766f05
32ca2fb
37de18a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,9 +43,11 @@ | |
| #include "modules/dynamicmodule.h" | ||
| #include "archivereader.h" | ||
| #include "databaseinterface.h" | ||
| #include "columnencoder.h" | ||
|
|
||
| #include <string> | ||
| #include <vector> | ||
| #include <stdexcept> | ||
|
|
||
| #include <QtPlugin> | ||
| #ifdef USE_QT_STATIC_LIBS | ||
|
|
@@ -152,6 +154,98 @@ static const char* statusError(Json::Value status, const std::string & error) | |
| return statusResult(status); | ||
| } | ||
|
|
||
| static Json::Value columnDecoderSnapshotJson() | ||
| { | ||
| Json::Value snapshot(Json::objectValue); | ||
| Json::Value columns(Json::arrayValue); | ||
| const ColumnEncoder::colMap decodingMap = ColumnEncoder::decodingMapSnapshot(); | ||
|
|
||
| for(const auto & keyVal : decodingMap) | ||
| { | ||
| Json::Value column(Json::objectValue); | ||
| column["encoded"] = keyVal.first; | ||
| column["decoded"] = keyVal.second; | ||
| columns.append(column); | ||
| } | ||
|
|
||
| snapshot["version"] = 1; | ||
| snapshot["columns"] = columns; | ||
| return snapshot; | ||
| } | ||
|
|
||
| struct ColumnDecoderSnapshot | ||
| { | ||
| ColumnEncoder::colMap decodingMap; | ||
| bool supplied = false; | ||
| }; | ||
|
|
||
| static ColumnDecoderSnapshot columnDecoderMapFromSnapshot(const Json::Value & snapshot) | ||
| { | ||
| ColumnDecoderSnapshot snapshotState; | ||
| snapshotState.supplied = true; | ||
| const Json::Value & columns = snapshot["columns"]; | ||
| if(!columns.isArray()) | ||
| return snapshotState; | ||
|
|
||
| for(const Json::Value & column : columns) | ||
| if(column.isObject() && column["encoded"].isString() && column["decoded"].isString()) | ||
| snapshotState.decodingMap[column["encoded"].asString()] = column["decoded"].asString(); | ||
|
|
||
| return snapshotState; | ||
| } | ||
|
|
||
| static ColumnDecoderSnapshot columnDecoderMapFromSnapshotString(const char * snapshotJson) | ||
| { | ||
| if(!snapshotJson || std::string(snapshotJson).empty()) | ||
| return ColumnDecoderSnapshot(); | ||
|
|
||
| Json::Value snapshot; | ||
| Json::Reader reader; | ||
| if(!reader.parse(snapshotJson, snapshot)) | ||
| throw std::runtime_error("Could not parse column decoder snapshot JSON."); | ||
|
|
||
| return columnDecoderMapFromSnapshot(snapshot); | ||
| } | ||
|
|
||
| static Json::Value parseStringArrayJson(const char * valuesJson) | ||
| { | ||
| if(!valuesJson) | ||
| throw std::runtime_error("Cannot decode column text from a null JSON payload."); | ||
|
|
||
| Json::Value values; | ||
| Json::Reader reader; | ||
| if(!reader.parse(valuesJson, values)) | ||
| throw std::runtime_error("Could not parse column text JSON payload."); | ||
| if(!values.isArray()) | ||
| throw std::runtime_error("Column text JSON payload must be an array."); | ||
|
|
||
| return values; | ||
| } | ||
|
|
||
| static Json::Value decodeColumnTextJson(const Json::Value & values, const ColumnDecoderSnapshot & snapshot) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole function looks like it should just call
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, this should be simplified. I can change the native path to parse the incoming JSON, temporarily install the captured context into One detail to confirm: should this bridge call decode arbitrary JSON with
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are not missing much; my phrasing about "R API smaller" was imprecise.
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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok well im curious to see it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in c21e37f. The bridge-specific manual JSON decoding loop is gone.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| { | ||
| Json::Value decodedValues(Json::arrayValue); | ||
|
|
||
| for(const Json::Value & value : values) | ||
| { | ||
| if(value.isNull()) | ||
| { | ||
| decodedValues.append(Json::Value()); | ||
| } | ||
| else if(value.isString()) | ||
| { | ||
| const std::string text = value.asString(); | ||
| decodedValues.append(snapshot.supplied ? ColumnEncoder::decodeAllWithMapping(text, snapshot.decodingMap) : ColumnEncoder::decodeAll(text)); | ||
| } | ||
| else | ||
| { | ||
| throw std::runtime_error("Column text JSON payload must contain only strings or null values."); | ||
| } | ||
| } | ||
|
|
||
| return decodedValues; | ||
| } | ||
|
|
||
| static Json::Value analysisOptionsStatus(const char * filePath, int analysisNr) | ||
| { | ||
| Json::Value status = statusBase("syntaxBridgeAnalysisOptionsFromJaspFile"); | ||
|
|
@@ -459,11 +553,22 @@ const char* STDCALL syntaxBridgeLoadDataSetFromJaspFileStatus(const char * fileP | |
|
|
||
| const char* STDCALL syntaxBridgeLoadQmlAndParseOptions(const char* moduleName, const char* analysisName, const char* qmlFile, const char* options, const char* version, bool preloadData) | ||
| { | ||
| if (!init()) | ||
| { | ||
| Log::log() << "Error during initialization" << std::endl; | ||
| Json::Value status; | ||
| Json::Reader reader; | ||
| if (!reader.parse(syntaxBridgeLoadQmlAndParseOptionsStatus(moduleName, analysisName, qmlFile, options, version, preloadData), status)) | ||
| return ""; | ||
| } | ||
| if (!status["ok"].asBool()) | ||
| return ""; | ||
|
|
||
| static std::string result; | ||
| result = status["options"].toStyledString(); | ||
| return result.c_str(); | ||
| } | ||
|
|
||
| const char* STDCALL syntaxBridgeLoadQmlAndParseOptionsStatus(const char* moduleName, const char* analysisName, const char* qmlFile, const char* options, const char* version, bool preloadData) | ||
| { | ||
| if (!init()) | ||
| return statusError(statusBase("syntaxBridgeLoadQmlAndParseOptions"), "Error during initialization."); | ||
|
|
||
| std::string qmlFileStr = qmlFile, | ||
| versionStr = version, | ||
|
|
@@ -475,17 +580,15 @@ const char* STDCALL syntaxBridgeLoadQmlAndParseOptions(const char* moduleName, c | |
|
|
||
| if (!form) | ||
| { | ||
| Log::log() << "Cannot create QML Form " << qmlFileStr << std::endl; | ||
| return ""; | ||
| return statusError(statusBase("syntaxBridgeLoadQmlAndParseOptions"), "Cannot create QML Form " + qmlFileStr); | ||
| } | ||
|
|
||
| Json::Value parsedOptions; | ||
| std::string errorMsg; | ||
|
|
||
| if (!form->parseOptions(options, parsedOptions, errorMsg)) | ||
| { | ||
| Log::log() << "Error when parsing options: " << errorMsg << std::endl; | ||
| return ""; | ||
| return statusError(statusBase("syntaxBridgeLoadQmlAndParseOptions"), "Error when parsing options: " + errorMsg); | ||
| } | ||
|
|
||
| gl_extraEncodings->setCurrentNamesFromOptionsMeta(parsedOptions); | ||
|
|
@@ -494,10 +597,10 @@ const char* STDCALL syntaxBridgeLoadQmlAndParseOptions(const char* moduleName, c | |
|
|
||
| rbridge_setWantedCols(analysisColsTypes); | ||
|
|
||
| static std::string result; | ||
| result = parsedOptions.toStyledString(); | ||
|
|
||
| return result.c_str(); | ||
| Json::Value status = statusBase("syntaxBridgeLoadQmlAndParseOptions"); | ||
| status["ok"] = true; | ||
| status["options"] = parsedOptions; | ||
| return statusResult(status); | ||
| } | ||
|
|
||
| const char* STDCALL syntaxBridgeAnalysisOptionsFromJaspFile(const char * filePath, int analysisNr) | ||
|
|
@@ -655,6 +758,42 @@ const char* STDCALL syntaxBridgeGetVariableNames() | |
| return result.c_str(); | ||
| } | ||
|
|
||
| void STDCALL syntaxBridgeSetVerbose(bool verbose) | ||
| { | ||
| gl_verbose = verbose; | ||
| Log::setDefaultDestination(verbose ? logType::cout : logType::null); | ||
| Log::setWhere(verbose ? logType::cout : logType::null); | ||
| } | ||
|
|
||
| const char* STDCALL syntaxBridgeColumnDecoderSnapshot() | ||
| { | ||
| static std::string result; | ||
|
|
||
| result = columnDecoderSnapshotJson().toStyledString(); | ||
| return result.c_str(); | ||
| } | ||
|
|
||
| const char* STDCALL syntaxBridgeDecodeColumnText(const char* valuesJson, const char* decoderSnapshotJson) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why isnt the decoder snapshot json not simply set from R? And the standard ColumnEncoder functions used? WHy all these extra functions?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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 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 Related detail: should the same context continue to carry the extra QML-option encoder state (
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes indeed, R should not carry mappings or bridge specific logic. A scoped decode would require a bunch of extra interfacing functions right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
How?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mechanically:
So the only custom code is context JSON <->
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok sounds good.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, |
||
| { | ||
| static std::string result; | ||
|
|
||
| try | ||
| { | ||
| Json::Value values = parseStringArrayJson(valuesJson); | ||
| ColumnDecoderSnapshot snapshot = columnDecoderMapFromSnapshotString(decoderSnapshotJson); | ||
| result = decodeColumnTextJson(values, snapshot).toStyledString(); | ||
| return result.c_str(); | ||
| } | ||
| catch(const std::exception & exception) | ||
| { | ||
| return statusError(statusBase("syntaxBridgeDecodeColumnText"), exception.what()); | ||
| } | ||
| catch(...) | ||
| { | ||
| return statusError(statusBase("syntaxBridgeDecodeColumnText"), "Unknown error while decoding column text."); | ||
| } | ||
| } | ||
|
|
||
| } // extern "C" | ||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is a good idea but I will need to know why this was done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed so
logType::nullactually discards output in the static jaspSyntax bridge. Before this change_nullStreampointed atstd::cout, sosyntaxBridgeSetVerbose(false)could still let native bridge logging reach stdout beforeLog::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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just call the following in the syntaxbridge constructor:
Like MainWindow::initLog ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is exactly the issue this was trying to fix: when the static bridge is used from R/jaspSyntax,
MainWindow::initLog()is never called, sologType::nullstill pointed atstd::coutand 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 toggleLog::setDefaultDestination()/Log::setWhere()fromsyntaxBridgeSetVerbose(), or do you want the bridge to do the fullerMainWindow::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
PreferencesModelor Desktop-owned logging policy.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use a
nullstreamlike 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in c21e37f. I reverted the
Common/log.cppchange entirely, so the global logging defaults outside SyntaxInterface are unchanged.SyntaxInterface now owns a local null stream and calls
Log::init(&nullstream)fromconfigureBridgeLogging()before bridge status/error paths can log.syntaxBridgeSetVerbose()only toggles betweencoutandnull; no Desktop preference/file logging is needed for this standalone bridge path. I also smoke-tested the jaspSyntax status-only error path withverbose = FALSE; it returns the structured error with zero captured native output.