Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

Return table #25

Merged
merged 6 commits into from
Jan 17, 2018
Merged

Return table #25

merged 6 commits into from
Jan 17, 2018

Conversation

tobiaslindulf
Copy link
Contributor

Support for returning a table (R data.frame or matrix) from the plugin to Qlik.

Status

[ ] Under development
[x] Waiting for code review
[x] Waiting for merge

Information

[ ] Contains breaking changes
[x] Contains new API(s)
[x] Contains documentation
[ ] Contains test
[x] Contains examples

To-do list

[ ] [Fix this thing]
[ ] [Fix another thing]
[ ] ...

message TableDescription {
repeated FieldDescription fields = 1; /// The fields of the table.
string name = 2; /// The name of the table.
int64 numberOfRows = 3; /// Number of rows in table.

Choose a reason for hiding this comment

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

To be consistent with the rest of the comments, you might want to use "The number of rows in the table." instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, this change must be updated on master and in qlik-oss/server-side-extentions as well, if we choose to correct it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we wait with this change since it is too small and does not justify the work. We change it in future versions.

else if (resultDataColumns[col].DataType == DataType.String)
{
row.Duals.Add(new Dual() { StrData = resultDataColumns[col].Strings[i] ?? "" });
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to support returning dual data? (if set in tableDescription) If not, you might want to throw an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we only get one type from Rserve so it will not be possible. I will think about what should be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tested to set the return values in all the defined functions to Dual, then run the example when only getting numerics or strings from Rserve. We actually send an error from the plugin to Qlik in this case since the data type of the first column differs from the definition (see code lines 549-554). Therefore no changes needed, all works as expected.

docs/versions.md Outdated
@@ -3,5 +3,6 @@ The table below is showing all released SSE R-plugin versions and which SSE prot

| __R-plugin Version__ | __SSE Protocol Version__ | __Qlik Sense Version__ | __QlikView Version__ |
| ----- | ----- | ----- | ----- |
| [v1.2.0](https://github.com/qlik-oss/sse-r-plugin/releases/tag/v1.2.0) | [v1.1.0](https://github.com/qlik-oss/server-side-extension/releases/tag/v1.1.0) | Qlik Sense Feb 2018 release | QlikView does not use this yet |
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a matter of preferences :) , but I would say SSE v1.1.0 is not supported in QV (or similar), rather than "..does not use this yet."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, I was thinking about that even if QlikView is not yet using SSE v1.1.0 yet it is still possible for QV to use and connect to a plugin that is supporting v1.1.0 . But I think I change the text here and put the "backwards compatibility" description in the release notes instead.

@josefinestal
Copy link
Contributor

Otherwise, it looks good! (approved)

@tobiaslindulf tobiaslindulf merged commit d552689 into master Jan 17, 2018
@tobiaslindulf tobiaslindulf deleted the return-table branch February 13, 2018 23:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants