Skip to content

Generate SQL-native identifiers and values in metricsview.ExpressionToSQL #8472

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.

Sign up for GitHub

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

Jump to bottom
Merged
begelundmuller merged 3 commits into from
Dec 10, 2025
Merged

Generate SQL-native identifiers and values in metricsview.ExpressionToSQL #8472

begelundmuller merged 3 commits into from
Dec 10, 2025
+216 −14

Conversation

@begelundmuller
Copy link
Contributor

@begelundmuller begelundmuller commented Dec 9, 2025

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!
@begelundmuller begelundmuller self-assigned this Dec 9, 2025
k-anshul
k-anshul reviewed Dec 10, 2025
View reviewed changes
runtime/pkg/sqlstring/sqlstring.go

// ToLiteral converts a Go value to its corresponding SQL literal representation.
// It handles primitive types and lists. All other types are emitted as JSON-encoded strings.
func ToLiteral(val any) string {
Copy link
Member

@k-anshul k-anshul Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some common cases can be handled without reflection. Reference :

rill/runtime/pkg/sqlconvert/convert.go

Lines 19 to 20 in 3bae1fa

func ConvertAssign(dest, src any) error {
// Common cases, without reflect.

Copy link
Contributor Author

@begelundmuller begelundmuller Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Anshul. It's a good point and would certainly speed up the code. But this code is not currently expected to be in any hot path (unlike ConvertAssign, which certainly is in the hot path), so I will omit this optimization for now to keep the surface area and chance of edge cases limited.
@briangregoryholmes briangregoryholmes mentioned this pull request Dec 10, 2025
Merged
22 tasks
@begelundmuller begelundmuller merged commit b4ced5f into main Dec 10, 2025
13 checks passed
@begelundmuller begelundmuller deleted the begelundmuller/metrics-expression-to-metrics-sql-generate-correct-SQL-values branch December 10, 2025 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@begelundmuller @k-anshul