fix: sql fn params #366
Open
fix: sql fn params #366
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR fixes SQL function parameter substitution by replacing function parameters with type‐based default values while refactoring SQL function signature extraction and integrating these changes into the typecheck flow.
- Added utility methods to StatementId.
- Refactored SQL function parsing to utilize SQLFunctionSignature and SQLFunctionArgs.
- Updated document parsing, typecheck, and identifier application with improved default value resolution.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/pgt_workspace/src/workspace/server/statement_identifier.rs | Added helper methods (is_root, is_child, parent) for statement hierarchy. |
| crates/pgt_workspace/src/workspace/server/sql_function.rs | Introduced SQLFunctionSignature and revamped function parameter extraction. |
| crates/pgt_workspace/src/workspace/server/parsed_document.rs | Removed legacy SQLFunctionBodyStore and integrated new SQL function extraction. |
| crates/pgt_workspace/src/workspace/server/document.rs | Added statement_content method to get statement SQL content. |
| crates/pgt_workspace/src/workspace/server.rs | Updated typecheck integration with new schema cache and identifier handling. |
| crates/pgt_typecheck/src/typed_identifier.rs | Enhanced apply_identifiers with default value formatting and support for type-based defaults. |
| crates/pgt_treesitter_queries | Updated parameter extraction queries and tests for ParameterMatch. |
| Other files | Minor adjustments to tests, diagnostics, and schema cache types to support the changes. |
Comments suppressed due to low confidence (1)
crates/pgt_workspace/src/workspace/server.rs:376
- [nitpick] The async block here could be refactored for improved readability and maintainability. A clearer structure may help future developers understand the schema cache and identifier propagation logic.
// sorry for the ugly code :(
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
juleswritescode
approved these changes
May 13, 2025
Comment on lines
335
to
338
| assert_eq!( | ||
| sql_out, | ||
| "select 0 + 0 + 0 + 0 + 0 + NULL " | ||
| ); |
There was a problem hiding this comment.
Suggested change
| assert_eq!( | |
| sql_out, | |
| "select 0 + 0 + 0 + 0 + 0 + NULL " | |
| ); | |
| // the numeric parameters are filled with 0; the enum does not have a default, so it's filled with `null` | |
| assert_eq!( | |
| sql_out, | |
| "select 0 + 0 + 0 + 0 + 0 + NULL " | |
| ); |
| db: DashMap<StatementId, Option<Arc<SQLFunctionBody>>>, | ||
| #[derive(Debug, Clone)] | ||
| pub struct SQLFunctionSignature { | ||
| pub name: (Option<String>, String), |
There was a problem hiding this comment.
nit: i think it would be more readable if it had a name and a schema property
| pub struct SQLFunctionBody { | ||
| pub range: TextRange, | ||
| pub body: String, | ||
| pub struct SQLFunctionArgs { |
There was a problem hiding this comment.
this is a single Arg right?
| // If not cached, try to extract it from the AST | ||
| let fn_body = get_sql_fn(ast, content).map(Arc::new); | ||
| // Extract language from function options | ||
| let language = find_option_value(create_fn, "language")?; |
There was a problem hiding this comment.
i really like that api
Comment on lines
+85
to
+86
| // If the default value is longer than "NULL", use "NULL" instead | ||
| "NULL".to_string() |
There was a problem hiding this comment.
why are we doing this?
There was a problem hiding this comment.
because we need to throw away positions when the replacement is longer than its original, and NULL will be shorter than a few default values (e.g. timestamps or custom enums)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
the idea is to replace the fn params with a default value based on their type:
will become
Todo
apply_identifiersfixes #353
fixes #352