Skip to content

Add PostgreSQL Operator DDL Support #2096

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
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add PostgreSQL Operator DDL Support #2096

wants to merge 7 commits into from

Conversation

@LucaCappelletti94
Copy link
Contributor

@LucaCappelletti94 LucaCappelletti94 commented Nov 9, 2025

Adds support for CREATE OPERATOR, CREATE OPERATOR FAMILY, and CREATE OPERATOR CLASS statements.

Examples

-- CREATE OPERATOR with all parameters
CREATE OPERATOR < (
    PROCEDURE = cas_lt,
    LEFTARG = CAS,
    RIGHTARG = CAS,
    COMMUTATOR = >,
    NEGATOR = >=,
    RESTRICT = scalarltsel,
    JOIN = scalarltjoinsel
);

-- CREATE OPERATOR FAMILY
CREATE OPERATOR FAMILY integer_ops USING btree;

-- CREATE OPERATOR CLASS
CREATE OPERATOR CLASS int4_ops
    DEFAULT FOR TYPE int4 USING btree FAMILY integer_ops AS
    OPERATOR 1 <,
    OPERATOR 2 <=,
    FUNCTION 1 btint4cmp(int4, int4),
    STORAGE int4;

iffyio
iffyio reviewed Nov 13, 2025
View reviewed changes
Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

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

Left some comments, a bit worried that its a lot of code and seems to be lacking test coverage, I tried to point out a few of them but would be good that we do a full pass to ensure code coverage in the PR

src/parser/mod.rs

/// Helper function to parse an operator name (which can contain special characters)
/// Operator names can be schema-qualified (e.g., schema.operator)
fn parse_operator_name(&mut self) -> Result<ObjectName, ParserError> {
Copy link
Contributor

@iffyio iffyio Nov 13, 2025

Choose a reason for hiding this comment

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

does this differ from parse_object_name?

src/parser/mod.rs
let func_name = self.parse_object_name(false)?;
join = Some(func_name);
}
_ => unreachable!("unexpected keyword in CREATE OPERATOR"),
Copy link
Contributor

@iffyio iffyio Nov 13, 2025

Choose a reason for hiding this comment

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

Can we return an error here instead of crashing in the case of a bug?

src/parser/mod.rs
let family = if self.parse_keyword(Keyword::FAMILY) {
Some(self.parse_object_name(false)?)
} else {
None
Copy link
Contributor

@iffyio iffyio Nov 13, 2025

Choose a reason for hiding this comment

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

Can we add a test case for a statement without a family?

src/parser/mod.rs
self.expect_token(&Token::RParen)?;
types
} else {
vec![]
Copy link
Contributor

@iffyio iffyio Nov 13, 2025

Choose a reason for hiding this comment

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

Can we add test coverage for this block?

src/parser/mod.rs
Comment on lines +6696 to +6698
} else if self.parse_keyword(Keyword::STORAGE) {
let storage_type = self.parse_data_type()?;
items.push(OperatorClassItem::Storage { storage_type });
Copy link
Contributor

@iffyio iffyio Nov 13, 2025

Choose a reason for hiding this comment

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

do we have tests for this part of the syntax?

src/parser/mod.rs
// Parse the operator name (can be schema-qualified)
// Operators can contain special characters like +, -, *, /, <, >, =, ~, !, @, #, %, ^, &, |, `, ?
// See https://www.postgresql.org/docs/current/sql-createoperator.html
let name = self.parse_operator_name()?;
Copy link
Contributor

@iffyio iffyio Nov 13, 2025

Choose a reason for hiding this comment

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

can we add test cases that use qualified operator names where applicable?

src/parser/mod.rs

// Check if this is a flag (HASHES or MERGES) - no '=' expected
match keyword {
Keyword::HASHES => {
Copy link
Contributor

@iffyio iffyio Nov 13, 2025

Choose a reason for hiding this comment

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

I think since we're only expecting one of each clause, we might need to add guards to the match clauses - e.g. Keyword::Hashes if !hashes => {...} Keyword::LEFTARG if left_arg.is_none() => {...}

it would be good to add negative tests that exercise such behavior.

src/parser/mod.rs

// Check if this is a flag (HASHES or MERGES) - no '=' expected
match keyword {
Keyword::HASHES => {
Copy link
Contributor

@iffyio iffyio Nov 13, 2025

Choose a reason for hiding this comment

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

Can we add test coverage for hashes, merges etc? we can do a pass through the clauses to ensure that they're properly covered

LucaCappelletti94 and others added 2 commits November 13, 2025 15:27
@LucaCappelletti94 @iffyio
Update src/parser/mod.rs
6bf05f0 
Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>
@LucaCappelletti94 @iffyio
Update src/parser/mod.rs
3d677e1 
Co-authored-by: Ifeanyi Ubah <ify1992@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iffyio iffyio iffyio left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@LucaCappelletti94 @iffyio