supabase-community/postgres-language-serverPublic

NotificationsYou must be signed in to change notification settings
Fork102
Star5k

fix: sql fn params#366

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

psteinroe merged 17 commits intomainfromfix/fn-params

May 22, 2025

Merged

fix: sql fn params#366

psteinroe merged 17 commits intomainfromfix/fn-params

May 22, 2025

Conversation

Copy link

Collaborator

psteinroe commentedApr 22, 2025•
edited
Loading

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

create or replacefunctionusers.select_no_ref (user_id int4) returns table (    first_nametext) language sql security invokeras $$select    first_nameFROMusers_hidden.userswhere     id= user_id;$$;

will become

create or replacefunctionusers.select_no_ref (user_id int4) returns table (    first_nametext) language sql security invokeras $$select    first_nameFROMusers_hidden.userswhere     id=0;-- <-- here$$;

Todo

pass params to typechecker
implementapply_identifiers

fixes#353
fixes#352

psteinroe added7 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 fromCopilot andjuleswritescode

May 7, 2025 08:07

CopilotAI reviewed

May 7, 2025

View reviewed changes

Copy link

Contributor

CopilotAI 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 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 OutdatedShow resolvedHide resolved

crates/pgt_treesitter_queries/src/lib.rs OutdatedShow resolvedHide resolved

crates/pgt_typecheck/src/typed_identifier.rsShow resolvedHide resolved

psteinroeand others added4 commits

May 7, 2025 10:12

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

c794aae

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update crates/pgt_treesitter_queries/src/lib.rs

4b0e3b6

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix: test

971fd7f

fix: test

8c4145a

juleswritescode approved these changes

May 13, 2025

View reviewed changes

Copy link

Collaborator

juleswritescode left a comment

Choose a reason for hiding this comment

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

just details, the concept looks great!

crates/pgt_typecheck/src/typed_identifier.rsShow resolvedHide resolved

crates/pgt_typecheck/src/typed_identifier.rs Outdated

Comment on lines 335 to 338

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

Copy link

Collaborator

juleswritescodeMay 13, 2025

Choose a reason for hiding this comment

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

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

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

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

Copy link

Collaborator

juleswritescodeMay 13, 2025

Choose a reason for hiding this comment

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

nit: i think it would be more readable if it had aname and aschema property

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

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

Copy link

Collaborator

juleswritescodeMay 13, 2025

Choose a reason for hiding this comment

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

this is a singleArg right?

Copy link

CollaboratorAuthor

psteinroeMay 14, 2025

Choose a reason for hiding this comment

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

yes!

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

		//If not cached, try to extract itfromthe AST
		letfn_body =get_sql_fn(ast, content).map(Arc::new);
		//Extract languagefromfunction options
		letlanguage =find_option_value(create_fn, "language")?;

Copy link

Collaborator

juleswritescodeMay 13, 2025

Choose a reason for hiding this comment

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

i really like that api

crates/pgt_typecheck/src/typed_identifier.rs

Comment on lines +85 to +86

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

Copy link

Collaborator

juleswritescodeMay 13, 2025

Choose a reason for hiding this comment

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

why are we doing this?

Copy link

CollaboratorAuthor

psteinroeMay 14, 2025

Choose a reason for hiding this comment

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

because we need to throw away positions when the replacement is longer than its original, andNULL will be shorter than a few default values (e.g. timestamps or custom enums)

crates/pgt_typecheck/src/typed_identifier.rsShow resolvedHide resolved

psteinroe added6 commits

May 19, 2025 08:28

fix: review

324fc87

merge main

ca280ab

fix: review

f187306

fix: review

c55a2d8

use arc for schema cache

266a217

Merge branch 'main' into fix/fn-params

a9c5040

psteinroe merged commit7ffbca6 intomain

May 22, 2025

7 checks passed

Labels

None yet

2 participants

Movatterモバイル変換

fix: sql fn params#366

fix: sql fn params#366

Uh oh!

Conversation

psteinroe commentedApr 22, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Todo

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juleswritescode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juleswritescodeMay 13, 2025

Choose a reason for hiding this comment

Uh oh!

juleswritescodeMay 13, 2025

Choose a reason for hiding this comment

Uh oh!

juleswritescodeMay 13, 2025

Choose a reason for hiding this comment

Uh oh!

psteinroeMay 14, 2025

Choose a reason for hiding this comment

Uh oh!

juleswritescodeMay 13, 2025

Choose a reason for hiding this comment

Uh oh!

juleswritescodeMay 13, 2025

Choose a reason for hiding this comment

Uh oh!

psteinroeMay 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

psteinroe commentedApr 22, 2025•
edited
Loading