-
Notifications
You must be signed in to change notification settings - Fork 9
Incorporate new filter types for Array (Multi-value text choice) fields #124
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 all commits
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 |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| #include <Rcpp.h> | ||
| #include <string> | ||
| using namespace Rcpp; | ||
|
|
||
| // [[Rcpp::export]] | ||
|
|
@@ -35,11 +36,30 @@ CharacterMatrix listToMatrix(List data, List names) { | |
| } | ||
| else | ||
| { | ||
| // only try to parse the first vector element as a string if it is non-null | ||
| // only try to parse the vector elements if the first element is not null | ||
| GenericVector gv = as<GenericVector>((as<List>(data[i]))[as<int>(indexList[j])]); | ||
| if(gv.size() > 0 && !Rf_isNull(gv[0])) | ||
| { | ||
| cMatrix(i,j) = as<CharacterVector>(gv[0])[0]; | ||
| // Check if first element is a string - if so, concatenate all elements | ||
| if(TYPEOF(gv[0]) == STRSXP) | ||
| { | ||
| std::string result; | ||
| for(int k = 0; k < gv.size(); k++) | ||
| { | ||
| if(!Rf_isNull(gv[k]) && TYPEOF(gv[k]) == STRSXP) | ||
| { | ||
| String s = as<String>(gv[k]); | ||
| if(k > 0) result += ","; | ||
| result += s.get_cstring(); | ||
| } | ||
| } | ||
| cMatrix(i,j) = result; | ||
| } | ||
| else | ||
| { | ||
| // Fallback to just returning the first element | ||
| cMatrix(i,j) = as<CharacterVector>(gv[0])[0]; | ||
| } | ||
|
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. Okay, it's been a long time since I've had to look at C++ code (or think about pointers) 👎 but this looks fine.
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. Yup, this was AI generated code that I just reviewed and tested! |
||
| } | ||
| } | ||
| } | ||
|
|
||
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.
I wonder how querying multi-value foreign key columns are done in R currently? That one should also return array result.
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.
the current behavior is to just use the first array value if the response is an array. that seems a bit odd, but I didn't want to go too far with this change so I just scoped it to arrays of string values.
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.
I guess this is fine. It doesn't handle special case such as the value itself contains comma, but better than current.
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.
correct. that was my thought as well (better than current). I don't know how far we want to go for the special cases (as mentioned "string values that contain comma")