Made new params building logic & got rid off .Query handle #11
Made new params building logic & got rid off .Query handle #111NepuNep1 merged 3 commits intoydb-platform:ydbfrom
Conversation
…it with QueryResultSet)
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new parameter building mechanism for YDB queries, replacing the ParamsFromMap approach with a structured ParamsBuilder pattern. The changes modernize query execution by using type-safe parameter builders and simplifying result set iteration.
- Replaces
ydb.ParamsFromMapwithydb.ParamsBuilder()for parameter construction - Updates YDB query methods to use
QueryResultSetand direct row iteration - Normalizes decimal type name casing from "Decimal" to "decimal"
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/engine/ydb/convert.go | Changes decimal type name to lowercase |
| internal/engine/ydb/catalog_tests/create_table_test.go | Updates test expectations for decimal type name |
| internal/codegen/golang/ydb_type.go | Adds JSON and date/time type mappings |
| internal/codegen/golang/templates/ydb-go-sdk/queryCode.tmpl | Replaces parameter building and simplifies result iteration |
| internal/codegen/golang/query.go | Adds ParamsBuilder generation logic with type mapping |
| examples/authors/ydb/query.sql.go | Shows generated code using new parameter pattern |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return "*string" | ||
| } | ||
| return "*string" | ||
|
|
There was a problem hiding this comment.
There's trailing whitespace on line 162 that should be removed.
| } | ||
|
|
||
| if method == "" { | ||
| panic(fmt.Sprintf("unknown YDB column type for param %s (goType=%s)", name, goType)) |
There was a problem hiding this comment.
The panic message should include the actual column type that wasn't recognized. Consider including sdk.DataType(field.Column.Type) in the error message to help with debugging.
| panic(fmt.Sprintf("unknown YDB column type for param %s (goType=%s)", name, goType)) | |
| panic(fmt.Sprintf("unknown YDB column type for param %s (goType=%s, columnType=%s)", name, goType, sdk.DataType(field.Column.Type))) |
| case "tztimestamp": | ||
| return "TzTimestamp" | ||
|
|
||
| //TODO: support other types |
There was a problem hiding this comment.
The TODO comment should specify which types need to be supported or reference an issue. An empty return value could lead to the panic in YDBParamsBuilder without clear indication of what's missing.
| //TODO: support other types | |
| // TODO: support additional YDB types such as Decimal, UUID, DyNumber, Optional, List, Tuple, Struct, Dict, Variant, Void, Null, and others as needed. | |
| // See https://github.com/ydb-platform/ydb-go-sdk/blob/main/table/types/types.go for the full list of YDB types. | |
| // Alternatively, track progress at https://github.com/sqlc-dev/sqlc/issues/XXXX |
* Made new params building logic & got rid off .Query handle (replaced it with QueryResultSet) * Cosmetics * Bug fixes --------- Co-authored-by: Viktor Pentyukhov <nepunep@172.28.98.114-red.dhcp.yndx.net>
* Made new params building logic & got rid off .Query handle (replaced it with QueryResultSet) * Cosmetics * Bug fixes --------- Co-authored-by: Viktor Pentyukhov <nepunep@172.28.98.114-red.dhcp.yndx.net>
No description provided.