-
Notifications
You must be signed in to change notification settings - Fork 1
Made new params building logic & got rid off .Query handle #11
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
…it with QueryResultSet)
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 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" | ||
|
|
Copilot
AI
Sep 9, 2025
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.
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)) |
Copilot
AI
Sep 9, 2025
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.
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 |
Copilot
AI
Sep 9, 2025
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.
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 |
No description provided.