-
Notifications
You must be signed in to change notification settings - Fork 9
Support new prepared statement metadata functions #51
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
base: main
Are you sure you want to change the base?
Conversation
taniabogatsch
left a comment
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.
Hi @hrl20! Thanks for thePR! I've had a look, and I am not opposed to exposing this functionality, even if it is not strictly part of the database/sql interface (we've gone a bit off the track of that already anyways). :D
However, there are tests missing, and also there is still quite a bit of work left if we want to expose capabilities to access the logical type through TypeInfo. Could you have a look at my comments? :)
| // ColumnCount returns the number of columns that will be returned by executing the prepared statement. | ||
| // If any of the column types is invalid (which can happen when the type is ambiguous), the result will be 1. | ||
| // Returns an error if the statement is closed or uninitialized. | ||
| func (s *Stmt) ColumnCount() (int, error) { |
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.
Can we add tests here? For both error paths, and the happy path?
statement.go
Outdated
|
|
||
| // ColumnTypes returns the types of all columns in the result set of the prepared statement. | ||
| // Returns an error if the statement is closed or uninitialized. | ||
| func (s *Stmt) ColumnTypes() ([]Type, error) { |
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.
Can we also add tests here for the error paths and the happy path (once rewritten to GetType)?
statement.go
Outdated
| // ColumnLogicalTypes returns the logical types of all columns in the result set of the prepared statement. | ||
| // Logical types provide more detailed type information than the basic Type, including custom types, | ||
| // nested structures, and type modifiers. The returned slice has one LogicalType entry for each column. | ||
| // Note: The caller is responsible for calling mapping.DestroyLogicalType on each returned LogicalType | ||
| // during cleanup. | ||
| // Returns an error if the statement is closed or uninitialized. | ||
| func (s *Stmt) ColumnLogicalTypes() ([]mapping.LogicalType, error) { | ||
| n, err := s.ColumnCount() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| types := make([]mapping.LogicalType, n) | ||
| for i := range n { | ||
| t := mapping.PreparedStatementColumnLogicalType(*s.preparedStmt, mapping.IdxT(i)) | ||
| types[i] = t | ||
| } | ||
| return types, nil | ||
| } |
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.
We need to change the signature of this function quite a bit. This should be func (s *Stmt) ColumnTypeInfo(n int) (TypeInfo, error). duckdb-go does not expose any functionality to handle logical types and we should leave as little memory management to the user as possible. TypeInfo is a wrapper around DuckDB's logical types and can be used to access (create) a matching logical type where needed in the driver (func (info *typeInfo) logicalType() mapping.LogicalType). Side-note, I am not sure why TypeInfo is an interface anymore... It should've probably just been a struct...
Either way, you'll need to add a function there func newTypeInfoFromLogicalType(lt mapping.LogicalType) (TypeInfo, error), and call that here once you have the logical type. Then, you can return that type info, which does not require any memory handling or access to the mapping package by the caller. To make this more useful, you might also want to expose some additional functionality around the TypeInfo, i.e., getters for the different nested types.
Also, we'll need tests for this.
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.
ok, I took a stab at newTypeInfoFromLogicalType, as well as exposing the internal/child types of TypeInfo that are available through the CAPI. TypeInfo.Details() will create different structs containing the details of different logical types. Open to different ways of handling this.
statement.go
Outdated
| // The returned slice has one string entry for each column, indexed starting from 0. | ||
| // For statements that do not return a result set, this returns an empty slice. | ||
| // Returns an error if the statement is closed or uninitialized. | ||
| func (s *Stmt) ColumnNames() ([]string, error) { |
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.
Let's have ColumName(n int) here and add tests.
taniabogatsch
left a comment
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.
Hi @hrl20 - finally found some time to get back on this! It's shaping up great now, thanks for all the changes w.r.t. the TypeInfo and testing! I've left a few more comments and maybe we can get some more input on the Details() about backwards compatibility (maybe not an issue).
Also, haven't seen this one before in the linter 😅
Error: type_info_test.go:349:1: Function name: TestNewTypeInfoFromLogicalType, Cyclomatic Complexity: 2, Halstead Volume: 8783.16, Maintainability Index: 19 (maintidx)
| // ColumnCount returns the number of columns that will be returned by executing the prepared statement. | ||
| // If any of the column types is invalid (which can happen when the type is ambiguous), the result will be 1. | ||
| // Returns an error if the statement is closed or uninitialized. | ||
| func (s *Stmt) ColumnCount() (int, error) { |
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.
Should we return uint here? Then, we can also take uint in the below functions and can skip the n < 0 check?
| if name == "" { | ||
| return "", nil | ||
| } | ||
| return name, nil |
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.
Aren't these paths the same?
| // Test column methods for UPDATE statement (should have no columns) | ||
| columnCount, innerErr := stmt.ColumnCount() | ||
| require.NoError(t, innerErr) | ||
| require.Equal(t, 1, columnCount) | ||
|
|
||
| // Test out of bounds access - should return empty/invalid for UPDATE with no columns | ||
| colName, innerErr := stmt.ColumnName(0) | ||
| require.NoError(t, innerErr) | ||
| require.Equal(t, "Count", colName) | ||
|
|
||
| colType, innerErr := stmt.ColumnType(0) | ||
| require.NoError(t, innerErr) | ||
| require.Equal(t, TYPE_BIGINT, colType) |
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.
Do the comments match the tests here? We compare to Count, so it is not empty/invalid? Same for TYPE_BIGINT?
| _, innerErr = stmt.ColumnType(0) | ||
| require.ErrorIs(t, innerErr, errClosedStmt) |
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.
Maybe we can add TypeInfo here, too?
| require.NoError(t, err) | ||
|
|
||
| // Test with closed statement | ||
| err = conn.Raw(func(driverConn any) error { |
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.
IIRC we already test this further up in the file?
| require.NoError(t, err) | ||
| } | ||
|
|
||
| func TestPreparedStatementColumnTypeInfo(t *testing.T) { |
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.
Ah, very cool! Was hoping to see some tests around the nested types, too. 😄
| require.NoError(t, err) | ||
|
|
||
| // Test 4: Statement with no ambiguous types should work normally | ||
| err = conn.Raw(func(driverConn any) error { |
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.
We could remove this since we already have good coverage for the happy path further up?
| type TypeInfo interface { | ||
| // InternalType returns the Type. | ||
| InternalType() Type | ||
| // Details returns type-specific details for complex types. | ||
| // Returns nil for simple/primitive types. | ||
| // Use type assertion to access specific detail types. | ||
| Details() TypeDetails | ||
| logicalType() mapping.LogicalType | ||
| } |
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 like the Details implementation. My only concern is that we might be breaking some compatibility here, if someone has their own type interface... 🤔 But tbh I am not sure if that is realistic - cc @wmTJc9IK0Q maybe you could give some input here? Since we had a similar discussion in your PR related to interfaces?
| // This allows inspecting types returned from prepared statements. | ||
| // The LogicalType must remain valid for the duration of this call. | ||
| // The returned TypeInfo does not hold a reference to the LogicalType. | ||
| func NewTypeInfoFromLogicalType(lt mapping.LogicalType) (TypeInfo, error) { |
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 don't think we need to expose this function since we don't expose the logical type?
This PR adds new methods to the
Stmttype to retrieve metadata about prepared statements before executionNew
Stmtmethods:ColumnCount()- returns number of columns in result setColumnType(n)- returns basic type of column at indexColumnName(n)- returns name of column at indexColumnTypeInfo(n)- returns detailed type information via enhanced TypeInfoAdded
Details()to the TypeInfo interface to inspect DuckDB logical types.Details()method returns type-specific metadata via new TypeDetails interface:DecimalDetails- width and scaleEnumDetails- enum valuesListDetails, ArrayDetails- child types and array sizeMapDetails- key/value typesStructDetails- struct entriesUnionDetails- union membersNewTypeInfoFromLogicalType()- converts DuckDB logical types to TypeInfo