Supports scanning of Array, IPv4, IPv6, and Map types into Go values that implement the sql.Scanner interface.
#1570
Conversation
Co-authored-by: Marco Gazerro <[email protected]>
Co-authored-by: Marco Gazerro <[email protected]>
Co-authored-by: Marco Gazerro <[email protected]>
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for scanning Array, IPv4, IPv6, and Map types into Go values implementing the sql.Scanner interface. The key changes include:
- New test cases for SQL scanner support for Array, IPv4, IPv6, and Map types.
- Updated ScanRow implementations in lib/column files to handle sql.Scanner.
- Minor improvements to error handling and consistency in test assertions.
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 |
|---|---|
| tests/map_test.go | Added TestSQLScannerMap to validate scanning for map types. |
| tests/ipv6_test.go | Introduced TestSQLScannerIPv6 and updated import order. |
| tests/ipv4_test.go | Introduced TestSQLScannerIPv4 and verified IPv4 scanning functionality. |
| tests/array_test.go | Added TestSQLScannerArray and test enhancements for array scanning. |
| lib/column/map.go | Updated ScanRow to support sql.Scanner for map columns. |
| lib/column/ipv6.go | Updated ScanRow to support sql.Scanner for IPv6 columns. |
| lib/column/ipv4.go | Updated ScanRow to support sql.Scanner for IPv4 columns. |
| lib/column/array.go | Updated ScanRow to support sql.Scanner for arrays. |
| require.NoError(t, batch.Append(col1Data)) | ||
| } | ||
| require.Equal(t, 10, batch.Rows()) | ||
| require.Nil(t, batch.Send()) |
There was a problem hiding this comment.
[nitpick] For consistency with other tests, consider replacing require.Nil(t, batch.Send()) with require.NoError(t, batch.Send()).
| require.Nil(t, batch.Send()) | |
| require.NoError(t, batch.Send()) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
The test added in this PR follows the structure of a test that was already in the repository (TestInterfaceArray), so I would maintain consistency between the two tests. I would suggest making this change separately, if necessary, outside of this PR, in order to update it in the other test and potentially in other areas as well.
tests/array_test.go
Outdated
| ok := assert.ObjectsAreEqual(col1Data, col1.value) | ||
| if !ok { | ||
| t.Fatalf("expected %#v, got %#v", col1Data, col1.value) | ||
| } |
There was a problem hiding this comment.
[nitpick] To maintain consistency with similar tests, use assert.Equal(t, col1Data, col1.value) instead of a manual equality check and t.Fatalf block.
| ok := assert.ObjectsAreEqual(col1Data, col1.value) | |
| if !ok { | |
| t.Fatalf("expected %#v, got %#v", col1Data, col1.value) | |
| } | |
| assert.Equal(t, col1Data, col1.value, "expected and actual values do not match") |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Done in commit Use assert.Equal instead of assert.ObjectsAreEqual.
Summary
Supports scanning of Array, IPv4, IPv6, and Map types into Go values that implement the
sql.Scannerinterface.Checklist
Does the TYPES.md file need to be updated? I couldn't figure it out.