feat: add YDB type annotations in struct field tags for runtime type validation #1936
Conversation
summaryInferred base version: v3.118.2 |
- Implemented tag parsing for YDB type annotations (e.g., `sql:"column,type:List<Text>"`) - Added type validation during ScanStruct to ensure database types match annotations - Support for all YDB types: primitives, List, Optional, Dict, and nested combinations - Added comprehensive unit tests for tag parsing and type validation - Added example tests demonstrating the feature usage - Feature works for both query client and database/sql APIs Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
- Created comprehensive example demonstrating type annotation usage - Added README with detailed documentation on syntax and benefits - Example shows basic types, complex types (List, Optional, Dict), and type mismatch detection - Example is standalone and can be run independently Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
- Added detailed package documentation explaining type annotation feature - Documented syntax, supported types, and benefits - Included examples in package-level comments Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1936 +/- ##
===========================================
- Coverage 74.01% 47.27% -26.74%
===========================================
Files 392 392
Lines 34343 34350 +7
===========================================
- Hits 25418 16238 -9180
- Misses 7807 17509 +9702
+ Partials 1118 603 -515
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@copilot need to add tests for old behaviour - if no type info in struct field annotation - this must work also |
…pe annotations - Added TestStructWithoutTypeAnnotation: validates scanning works without type annotations - Added TestStructMixedTypeAnnotations: validates mixing fields with and without annotations - Added TestStructListWithoutTypeAnnotation: validates List types work without annotations - Added TestStructOptionalWithoutTypeAnnotation: validates Optional types work without annotations - All tests pass, confirming backward compatibility is maintained Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
Added 4 backward compatibility tests in commit a181143:
All tests pass, confirming type annotations are fully optional and backward compatible. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for optional YDB type annotations in struct field tags to enable runtime type validation when scanning database results. The feature allows developers to specify expected YDB types like type:Text, type:List<Text>, or type:Optional<Uint64> in struct tags, with automatic validation that database column types match the annotations during ScanStruct() operations.
Key Changes
- Added YDB type string parser supporting primitives, List, Optional, Dict, and nested types
- Integrated type validation into the existing ScanStruct flow with backward compatibility
- Comprehensive test coverage including 23 unit tests and examples demonstrating usage
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/query/scanner/tag.go | Implements YDB type string parsing and struct tag parsing with parseFieldTag, parseYDBType, and parsePrimitiveYDBType functions |
| internal/query/scanner/tag_test.go | Unit tests for tag parsing and YDB type parsing functions |
| internal/query/scanner/struct.go | Integrates type validation into ScanStruct by parsing field tags and validating column types |
| internal/query/scanner/struct_test.go | Tests for ScanStruct with type annotations including backward compatibility tests |
| internal/query/scanner/data.go | Adds columnTypeByName helper method for retrieving column types |
| internal/query/scanner/example_test.go | Example code demonstrating type annotation usage in struct fields |
| examples/type_annotations/main.go | Standalone example application showing type annotations in a real scenario |
| examples/type_annotations/README.md | Documentation for the type annotations feature |
| examples/type_annotations/.gitignore | Ignores compiled binary for type annotations example |
| examples/.gitignore | Formatting fix (adds trailing newline) |
| tests/slo/go.mod | Moves prometheus/client_model from indirect to direct dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/slo/go.mod
Outdated
|
|
||
| require ( | ||
| github.com/prometheus/client_golang v1.14.0 | ||
| github.com/prometheus/client_model v0.6.0 |
There was a problem hiding this comment.
[nitpick] Unrelated dependency change: Moving github.com/prometheus/client_model from indirect to direct dependencies in tests/slo/go.mod appears unrelated to the type annotations feature.
This change should either:
- Be explained in the PR description if it's intentional
- Be removed if it was an accidental side effect of running
go mod tidy
If this package is now being imported directly in the SLO test code, that would justify the change. Otherwise, this should be reverted to keep the PR focused on type annotations.
There was a problem hiding this comment.
Reverted the unrelated dependency change in commit 1af2d17. The change was an accidental side effect of running go mod tidy during development.
internal/query/scanner/struct.go
Outdated
| if expectedType.String() != actualType.String() { | ||
| return xerrors.WithStackTrace(fmt.Errorf( | ||
| "type mismatch for field '%s': expected %s, got %s", | ||
| tag.columnName, | ||
| expectedType.String(), | ||
| actualType.String(), | ||
| )) | ||
| } |
There was a problem hiding this comment.
Type comparison using string equality may be fragile. Comparing types using expectedType.String() != actualType.String() relies on the string representation being identical, which could be brittle if:
- Type formatting changes in the future
- There are whitespace differences in type representations
- Equivalent types have different string representations (e.g., aliased types)
Consider using the type system's built-in equality check instead:
if !types.Equal(expectedType, actualType) {
return xerrors.WithStackTrace(fmt.Errorf(
"type mismatch for field '%s': expected %s, got %s",
tag.columnName,
expectedType.String(),
actualType.String(),
))
}The types.Equal function uses the equalsTo method which provides proper type equality semantics.
There was a problem hiding this comment.
Fixed in commit 1af2d17. Now using types.Equal(expectedType, actualType) for proper type equality semantics instead of string comparison.
| func parseFieldTag(tagValue string) structFieldTag { | ||
| if tagValue == "" || tagValue == "-" { | ||
| return structFieldTag{columnName: tagValue} | ||
| } | ||
|
|
||
| // Split by comma to separate column name from options | ||
| parts := strings.Split(tagValue, ",") | ||
| tag := structFieldTag{ | ||
| columnName: strings.TrimSpace(parts[0]), | ||
| } | ||
|
|
||
| // Parse options (e.g., type:List<Text>) | ||
| for i := 1; i < len(parts); i++ { | ||
| option := strings.TrimSpace(parts[i]) | ||
| if strings.HasPrefix(option, "type:") { | ||
| tag.ydbType = strings.TrimSpace(strings.TrimPrefix(option, "type:")) | ||
| } | ||
| } | ||
|
|
||
| return tag | ||
| } |
There was a problem hiding this comment.
The parseFieldTag function has a bug when parsing Dict type annotations. Using strings.Split(tagValue, ",") will incorrectly split a tag like sql:"metadata,type:Dict<Text,Uint64>" into three parts: ["metadata", "type:Dict<Text", "Uint64>"] instead of two parts: ["metadata", "type:Dict<Text,Uint64>"].
This happens because strings.Split splits on all commas, including the comma inside the Dict type specification. This will cause the type annotation to be parsed incorrectly as "Dict<Text" instead of "Dict<Text,Uint64>".
Solution: Use a more sophisticated parsing approach that respects angle brackets, similar to findTopLevelComma. For example:
func parseFieldTag(tagValue string) structFieldTag {
if tagValue == "" || tagValue == "-" {
return structFieldTag{columnName: tagValue}
}
// Find first top-level comma (outside angle brackets)
commaPos := findTopLevelComma(tagValue)
var columnName, typeAnnotation string
if commaPos == -1 {
// No comma found, just column name
columnName = strings.TrimSpace(tagValue)
} else {
columnName = strings.TrimSpace(tagValue[:commaPos])
// Parse options after the comma
options := strings.TrimSpace(tagValue[commaPos+1:])
if strings.HasPrefix(options, "type:") {
typeAnnotation = strings.TrimSpace(strings.TrimPrefix(options, "type:"))
}
}
return structFieldTag{
columnName: columnName,
ydbType: typeAnnotation,
}
}There was a problem hiding this comment.
Fixed the Dict type parsing bug in commit 1af2d17. The parseFieldTag function now uses findTopLevelComma to properly handle commas inside angle brackets. Added tests to verify Dict type annotations like sql:"metadata,type:Dict<Text,Uint64>" are parsed correctly.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot need to prepend line into CHANGELOG.md about PR changes |
- Fixed parseFieldTag to use findTopLevelComma for proper Dict type parsing - Changed type comparison from string equality to types.Equal for robustness - Added tests for Dict type annotation parsing and validation - Reverted unrelated go.mod dependency change Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
- Added comprehensive entry describing the new type annotation support - Documented key features: optional annotations, runtime validation, backward compatibility - Listed supported types: primitives, List, Optional, Dict, and nested combinations Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
|
@copilot fix linter issues |
- Fixed formatting issues (trailing whitespace, indentation) - Removed unused columnTypeByName function (U1000 staticcheck warning) - All tests pass and code is now lint-clean Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
Summary
This PR successfully implements YDB type annotations in struct field tags as requested in the issue.
Implementation Checklist
Features Delivered
1. Tag Parsing
sql:"column_name,type:YDBType"findTopLevelComma2. Type Validation
types.Equalfor robust type comparison instead of string equality3. Testing
4. Documentation
Code Review Fixes
1. Fixed Dict Type Parsing Bug
Issue:
parseFieldTagusedstrings.Split(tagValue, ",")which incorrectly split Dict types likesql:"metadata,type:Dict<Text,Uint64>"into three parts.Solution: Now uses
findTopLevelCommato respect angle brackets when parsing tags.Tests: Added explicit tests for Dict type parsing with commas.
2. Improved Type Comparison
Issue: Used string comparison (
expectedType.String() != actualType.String()) which could be fragile.Solution: Now uses
types.Equal(expectedType, actualType)for proper type equality semantics.3. Reverted Unrelated Dependency Change
Issue:
github.com/prometheus/client_modelwas moved from indirect to direct intests/slo/go.mod.Solution: Reverted to keep PR focused on type annotations feature.
4. Added CHANGELOG Entry
Issue: Missing CHANGELOG.md entry for this feature.
Solution: Added comprehensive entry describing the type annotation support.
5. Fixed Linter Issues
Issue: Code had formatting and staticcheck warnings.
Solution:
columnTypeByNamefunctionBackward Compatibility
Type annotations are completely optional. The following test cases confirm backward compatibility:
TestStructWithoutTypeAnnotation- Fields without type annotations work as beforeTestStructMixedTypeAnnotations- Fields can mix annotated and non-annotatedTestStructListWithoutTypeAnnotation- List types work without annotationsTestStructOptionalWithoutTypeAnnotation- Optional types work without annotationsExample Usage
Benefits
Test Results
All tests passing:
Total test count: 25 new tests + all existing tests passing
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.