Skip to content

Conversation

@1NepuNep1
Copy link
Collaborator

No description provided.

@1NepuNep1 1NepuNep1 requested a review from Copilot September 9, 2025 11:38

This comment was marked as outdated.

@1NepuNep1 1NepuNep1 requested a review from Copilot September 9, 2025 11:41
Copy link

Copilot AI left a 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.ParamsFromMap with ydb.ParamsBuilder() for parameter construction
  • Updates YDB query methods to use QueryResultSet and 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"

Copy link

Copilot AI Sep 9, 2025

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.

Suggested change
Copilot uses AI. Check for mistakes.
}

if method == "" {
panic(fmt.Sprintf("unknown YDB column type for param %s (goType=%s)", name, goType))
Copy link

Copilot AI Sep 9, 2025

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.

Suggested change
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)))
Copilot uses AI. Check for mistakes.
case "tztimestamp":
return "TzTimestamp"

//TODO: support other types
Copy link

Copilot AI Sep 9, 2025

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.

Suggested change
//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
Copilot uses AI. Check for mistakes.
@1NepuNep1 1NepuNep1 merged commit 296cca7 into ydb-platform:ydb Sep 9, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant