feat/bugfix: Passing schema from C, plus example/tests in C #1535
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1535 +/- ##
==========================================
+ Coverage 84.73% 84.77% +0.04%
==========================================
Files 124 126 +2
Lines 34227 34332 +105
Branches 34227 34332 +105
==========================================
+ Hits 29001 29106 +105
Misses 3816 3816
Partials 1410 1410 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| char* key_str = "delta.columnMapping.physicalName"; | ||
| KernelStringSlice key = { key_str, strlen(key_str) }; | ||
| char* value = get_from_map(metadata, key, allocate_string); | ||
| char* value = get_from_string_map(metadata, key, allocate_string); |
There was a problem hiding this comment.
this was just wrong before. we actually couldn't compile with VERBOSE set to true
| } | ||
| )* | ||
| }; | ||
| // TODO: turn all the primitive visitors below into a macro once cbindgen can run on macro expanded code |
There was a problem hiding this comment.
all this code is just copy/pasted from running cargo expand so it's basically the same as before, but just not macroified
| } | ||
|
|
||
|
|
||
| // Our schema code might add (is nullable: x) to names, which we need to strip off. |
There was a problem hiding this comment.
// Our schema code might add (is nullable: x) to names, which we need to strip off.
could you give an example? I think it's name (is nullable: true) --> name?
| char* fixup_loc = fixup_name(item->name); | ||
| uintptr_t child_id = visit_schema_item(item, state, cschema); | ||
| if (fixup_loc != NULL) { | ||
| *fixup_loc = ' '; |
There was a problem hiding this comment.
| char* fixup_loc = fixup_name(item->name); | |
| uintptr_t child_id = visit_schema_item(item, state, cschema); | |
| if (fixup_loc != NULL) { | |
| *fixup_loc = ' '; | |
| // Removes the nullability information from name `name (is nullable: true)` -> `name` | |
| char* fixup_loc = fixup_name(item->name); | |
| uintptr_t child_id = visit_schema_item(item, state, cschema); | |
| // Undo the fixup. `name` -> `name (is nullable: true)` | |
| if (fixup_loc != NULL) { | |
| *fixup_loc = ' '; |
| // previous visit will have printed the issue | ||
| return 0; | ||
| } | ||
| visit_res = visit_field_array(state, name, child_visit_id, false, allocate_error); |
There was a problem hiding this comment.
we're throwing away nullability information here, and we've got a bunch of LOC dedicated to the name fixup. How hard is it to just add nullability info to item as a field, and keep name as name? I'd prefer that approach personally
EDIT: it seems like SchemaItem actually already has an is_nullable field. Let's use that
| visit_res = visit_field_array(state, name, child_visit_id, false, allocate_error); | |
| visit_res = visit_field_array(state, name, child_visit_id, item->is_nullable, allocate_error); |
| // This function looks at the type field in the schema to figure out which visitor to call. It's a | ||
| // bit gross as the schema code is string based, a real implementation would have a more robust way | ||
| // to represent a schema. | ||
| uintptr_t visit_schema_item(SchemaItem* item, KernelSchemaVisitorState *state, CSchema *cschema) { |
There was a problem hiding this comment.
nit: I'd advocate to separate this out into a separate header file so that engines can get an isolated example of schema visiting. that way we keep out the schema stuff.
2 participants
What changes are proposed in this pull request?
cbindgenruns before macro expansion, so previously none of those visitors were actually being generatedread_tableto allow specifying which columns to select, plus all the associated code to pass that back to kernelHow was this change tested?
New tests when running
make testinread_table