fix: sql fn params #366

psteinroe · 2025-04-22T08:04:12Z

the idea is to replace the fn params with a default value based on their type:

create or replace function users.select_no_ref (user_id int4) returns table (
    first_name text
) language sql security invoker as $$
select
    first_name
FROM 
    users_hidden.users
where 
    id = user_id;
$$;

will become

create or replace function users.select_no_ref (user_id int4) returns table (
    first_name text
) language sql security invoker as $$
select
    first_name
FROM 
    users_hidden.users
where 
    id = 0; -- <-- here
$$;

Todo

pass params to typechecker
implement apply_identifiers

fixes #353
fixes #352

Copilot

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 :(

crates/pgt_workspace/src/workspace/server/sql_function.rs

crates/pgt_treesitter_queries/src/lib.rs

crates/pgt_typecheck/src/typed_identifier.rs

Co-authored-by: Copilot <[email protected]>

juleswritescode

just details, the concept looks great!

crates/pgt_typecheck/src/typed_identifier.rs

juleswritescode · 2025-05-13T06:43:32Z

crates/pgt_typecheck/src/typed_identifier.rs

+    if parts.len() == 1 && parts[0].starts_with('$') {
+        let idx = parts[0][1..].parse::<usize>().ok()?;
+        let identifier = identifiers.get(idx - 1)?;
+        return Some((identifier, idx));
+    }


crates/pgt_typecheck/src/typed_identifier.rs

juleswritescode · 2025-05-13T06:55:36Z

crates/pgt_typecheck/src/typed_identifier.rs

+        assert_eq!(
+            sql_out,
+            "select 0      + 0                           + 0  + 0                   + 0               + NULL     "
+        );


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 "

);

juleswritescode · 2025-05-13T07:06:56Z

crates/pgt_workspace/src/workspace/server/sql_function.rs

-    db: DashMap<StatementId, Option<Arc<SQLFunctionBody>>>,
+#[derive(Debug, Clone)]
+pub struct SQLFunctionSignature {
+    pub name: (Option<String>, String),


nit: i think it would be more readable if it had a name and a schema property

juleswritescode · 2025-05-13T07:07:17Z

crates/pgt_workspace/src/workspace/server/sql_function.rs

-pub struct SQLFunctionBody {
-    pub range: TextRange,
-    pub body: String,
+pub struct SQLFunctionArgs {


this is a single Arg right?

juleswritescode · 2025-05-13T07:08:10Z

crates/pgt_workspace/src/workspace/server/sql_function.rs

-        // 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")?;


i really like that api

juleswritescode · 2025-05-13T07:15:14Z

crates/pgt_typecheck/src/typed_identifier.rs

+        // If the default value is longer than "NULL", use "NULL" instead
+        "NULL".to_string()


why are we doing this?

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)

juleswritescode · 2025-05-13T07:34:08Z

crates/pgt_typecheck/src/typed_identifier.rs

+
+    let mut result = sql.to_string();
+
+    let mut valid_positions = true;


I'm not sure what position is in this context.
I understand it this way: We'll need to adjust the statements ranges if a parameter's default value is longer than the substituted text.
If that's correct, how about we rename this to something like should_adjust_ranges (default false) ?

sometimes (almost never), we get a position back for the type error. in that case, we show the diagnostics on that position instead of the entire statement. since we are replacing the vars in the text, the position we get back does not match the position in the original statement anymore if the replacements are not the same length. hence, if valid_positions is false, we throw away the position from the type error and always show the error on the entire length of the statement.

psteinroe added 7 commits April 22, 2025 10:01

fix: sql fn params

beeb661

save progress

9b6c7aa

add ts query

ba83202

progress

e032c3f

progress

896bfb1

just tests missing now

9f9cf9b

chore: merge main

733c8f2

psteinroe marked this pull request as ready for review May 7, 2025 08:07

psteinroe requested review from Copilot and juleswritescode May 7, 2025 08:07

Copilot AI reviewed May 7, 2025

View reviewed changes

crates/pgt_workspace/src/workspace/server/sql_function.rs Outdated Show resolved Hide resolved

crates/pgt_treesitter_queries/src/lib.rs Outdated Show resolved Hide resolved

crates/pgt_typecheck/src/typed_identifier.rs Show resolved Hide resolved

psteinroe and others added 4 commits May 7, 2025 10:12

Update crates/pgt_workspace/src/workspace/server/sql_function.rs

c794aae

Co-authored-by: Copilot <[email protected]>

Update crates/pgt_treesitter_queries/src/lib.rs

4b0e3b6

Co-authored-by: Copilot <[email protected]>

fix: test

971fd7f

fix: test

8c4145a

juleswritescode approved these changes May 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: sql fn params #366

fix: sql fn params #366

psteinroe commented Apr 22, 2025 •

edited

Loading

Copilot AI left a comment

juleswritescode left a comment

juleswritescode May 13, 2025

juleswritescode May 13, 2025

juleswritescode May 13, 2025

juleswritescode May 13, 2025

psteinroe May 14, 2025

juleswritescode May 13, 2025

juleswritescode May 13, 2025

psteinroe May 14, 2025

juleswritescode May 13, 2025

psteinroe May 14, 2025

		// If the default value is longer than "NULL", use "NULL" instead
		"NULL".to_string()


		let mut result = sql.to_string();

		let mut valid_positions = true;

fix: sql fn params #366

Are you sure you want to change the base?

fix: sql fn params #366

Conversation

psteinroe commented Apr 22, 2025 • edited Loading

Todo

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

juleswritescode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psteinroe commented Apr 22, 2025 •

edited

Loading