-
Notifications
You must be signed in to change notification settings - Fork 4
Add StructuredTable #12
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
Conversation
This allows for inserting data on an empty Table.
c159527 to
e132315
Compare
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.
Pull request overview
This PR adds a new StructuredTable type that enables type-safe parsing and manipulation of CSV data through compile-time schema validation. The existing Table implementation has been refactored into separate modules to support the new functionality while maintaining backward compatibility through re-exports.
Changes:
- Introduced
StructuredTablegeneric type that maps CSV data to user-defined struct types with automatic type conversion and validation - Refactored monolithic
src/root.ziginto separatetable.zigandschema.zigmodules for better organization - Added comprehensive test suite for the new StructuredTable functionality and updated README documentation with usage examples
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/root.zig | Converted to thin re-export module that exposes both Table and StructuredTable APIs |
| src/table.zig | Extracted core Table implementation with bug fix for getColumnCount signature |
| src/schema.zig | New module implementing StructuredTable with type-safe CSV parsing and serialization |
| src/tests/root.zig | New test harness that imports individual test files |
| src/tests/table.zig | Updated table tests with corrected import and updated API usage |
| src/tests/schema.zig | Comprehensive test suite for StructuredTable covering parsing, editing, and error handling |
| build.zig | Updated test configuration to use new test structure with module import |
| README.md | Added extensive documentation and examples for both StructuredTable and Table APIs |
Comments suppressed due to low confidence (1)
src/tests/table.zig:5
- The StructuredTable constant is imported but never used in this test file. Since this file only contains tests for the Table type (not StructuredTable), this import should be removed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn getRow(self: Self, row_index: usize) TableError!ParseResult(table_schema) { | ||
| if (row_index >= self.getRowCount()) return TableError.RowNotFound; | ||
| var out: table_schema = undefined; | ||
| inline for (schema_info.@"struct".fields) |field| { | ||
| const field_name = field.name; | ||
| const column_indexes = self.table.findColumnIndexesByValue(self.allocator, 0, field_name) catch return ParseResult(table_schema){ | ||
| .@"error" = .{ | ||
| .kind = StructureError.MissingColumn, | ||
| .field_name = field_name, | ||
| .field_type = @typeName(field.type), | ||
| .csv_value = null, | ||
| }, | ||
| }; | ||
| defer self.allocator.free(column_indexes); | ||
| if (column_indexes.len > 1) return ParseResult(table_schema){ | ||
| .@"error" = .{ | ||
| .kind = StructureError.AmbiguousColumn, | ||
| .field_name = field_name, | ||
| .field_type = @typeName(field.type), | ||
| .csv_value = null, | ||
| }, | ||
| }; | ||
| const rows = self.table.getColumnByIndex(self.allocator, column_indexes[0]) catch return ParseResult(table_schema){ | ||
| .@"error" = .{ | ||
| .kind = StructureError.MissingColumn, | ||
| .field_name = field_name, | ||
| .field_type = @typeName(field.type), | ||
| .csv_value = null, | ||
| }, | ||
| }; | ||
| defer self.allocator.free(rows); | ||
| const value = rows[row_index + 1]; | ||
| const parsed = (&self).deserializeCsvValue(field.type, value) catch |err| return ParseResult(table_schema){ | ||
| .@"error" = .{ | ||
| .kind = err, | ||
| .field_name = field_name, | ||
| .field_type = @typeName(field.type), | ||
| .csv_value = value, | ||
| }, | ||
| }; | ||
| @field(out, field_name) = parsed; | ||
| } | ||
| return ParseResult(table_schema){ | ||
| .ok = .{ | ||
| .value = out, | ||
| }, | ||
| }; | ||
| } |
Copilot
AI
Jan 18, 2026
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.
Performance concern: the function searches for each field's column index by value on every invocation, using findColumnIndexesByValue which performs a linear search. For tables with many fields or when this function is called frequently in a loop, this results in O(n*m) complexity where n is the number of fields and m is the number of columns. Consider caching the column index mapping after the first lookup to improve performance.
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.
Performance is not in scope atm.
| pub fn editRow(self: *Self, row_index: usize, row: table_schema) TableError!ParseResult(table_schema) { | ||
| if (row_index >= self.getRowCount()) return TableError.RowNotFound; | ||
| inline for (schema_info.@"struct".fields) |field| { | ||
| const field_name = field.name; | ||
| const column_indexes = self.table.findColumnIndexesByValue(self.allocator, 0, field_name) catch return ParseResult(table_schema){ | ||
| .@"error" = .{ | ||
| .kind = StructureError.MissingColumn, | ||
| .field_name = field_name, | ||
| .field_type = @typeName(field.type), | ||
| .csv_value = null, | ||
| }, | ||
| }; | ||
| defer self.allocator.free(column_indexes); | ||
| if (column_indexes.len > 1) return ParseResult(table_schema){ | ||
| .@"error" = .{ | ||
| .kind = StructureError.AmbiguousColumn, | ||
| .field_name = field_name, | ||
| .field_type = @typeName(field.type), | ||
| .csv_value = null, | ||
| }, | ||
| }; | ||
| const column_index = column_indexes[0]; | ||
| const table_index = headerAwareToTableIndex(row_index); | ||
| const value = try self.serializeCsvValue(field.type, @field(row, field_name)); | ||
| try self.table.replaceValue(table_index, column_index, value); | ||
| } | ||
| return ParseResult(table_schema){ | ||
| .ok = .{ | ||
| .value = row, | ||
| }, | ||
| }; | ||
| } |
Copilot
AI
Jan 18, 2026
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.
Performance concern: the function searches for each field's column index by value on every invocation, using findColumnIndexesByValue which performs a linear search. For tables with many fields or when this function is called frequently in a loop, this results in O(n*m) complexity where n is the number of fields and m is the number of columns. Consider caching the column index mapping after the first lookup to improve performance.
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.
Performance is not in scope at the moment.
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.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .bool => { | ||
| const lower = std.ascii.allocLowerString(self.allocator, value) catch return TableError.OutOfMemory; | ||
| defer self.allocator.free(lower); | ||
| for ([_][]const u8{ "true", "1", "yes", "y" }) |true_word| { | ||
| if (std.mem.eql(u8, true_word, lower)) { | ||
| return true; | ||
| } | ||
| } | ||
| for ([_][]const u8{ "false", "0", "no", "n" }) |false_word| { | ||
| if (std.mem.eql(u8, false_word, lower)) { | ||
| return false; | ||
| } | ||
| } | ||
| return StructureError.UnexpectedType; | ||
| }, |
Copilot
AI
Jan 18, 2026
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.
Memory allocated by std.ascii.allocLowerString is correctly freed with defer. However, this is called every time a boolean value is parsed. For better performance, consider converting the value to lowercase in-place in a small stack buffer since bool values are typically short strings.
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.
Not in scope
No description provided.