Skip to content

Support two-argument TRIM(string, characters) in PostgreSQL#2240

Merged
yoavcloud merged 4 commits intoapache:mainfrom
LucaCappelletti94:pg_query_parity_2
Feb 27, 2026
Merged

Support two-argument TRIM(string, characters) in PostgreSQL#2240
yoavcloud merged 4 commits intoapache:mainfrom
LucaCappelletti94:pg_query_parity_2

Conversation

@LucaCappelletti94
Copy link
Contributor

PostgreSQL supports TRIM(string, characters) as a function form, but sqlparser-rs rejected it under PostgreSqlDialect with: ParserError("Expected: ), found: ,").

This PR adds PostgreSQL support for the comma-style TRIM form and adds regression coverage.

@LucaCappelletti94 LucaCappelletti94 marked this pull request as ready for review February 25, 2026 11:12
})
} else if self.consume_token(&Token::Comma)
&& dialect_of!(self is DuckDbDialect | SnowflakeDialect | BigQueryDialect | GenericDialect)
&& dialect_of!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you turn this into a function of the Dialect trait? For example: self.dialect.supports_comma_separated_trim.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will try to run a review of which dialects supports it, I hope it won't become a nightmare of little differences.

Copy link
Contributor

@yoavcloud yoavcloud Feb 27, 2026

Choose a reason for hiding this comment

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

I didn't mean to send you on a wild goose chase, just use whatever is already supported in code: DuckDbDialect, SnowflakeDialect, BigQueryDialect, GenericDialect and now PostgreSQL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am currently just double checking the dialects I actively work with, just in case there are small differences. I found this missing feature via differential fuzzing and not with direct use, so I am not THAT familiar with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And done, a couple of dialects were still missing, it was a good thing to spend a bit more time to check these.

}

#[test]
fn parse_postgres_two_argument_trim() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this unit test can be moved to sqlparser_common.rs so it will cover all supporting dialects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok done

Add a new Dialect::supports_comma_separated_trim() capability with a
false default and opt in the dialects that currently support TRIM(expr,
characters).

Use this capability in parse_trim() instead of a hardcoded dialect list
so dialect support is declared at the dialect layer.
Move two-argument TRIM assertions from postgres-only tests into the
common parser suite and select dialects via
supports_comma_separated_trim().

Keep explicit failure coverage for dialects that do not support comma
syntax and remove the redundant postgres-specific test.
@yoavcloud yoavcloud added this pull request to the merge queue Feb 27, 2026
@yoavcloud
Copy link
Contributor

Thank you @LucaCappelletti94!

Merged via the queue into apache:main with commit 5b7bc1a Feb 27, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants