Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix (#7002) Where Condition Build Issue #7004

Closed
wants to merge 4 commits into from

Conversation

VarusHsu
Copy link

  • Do only one thing
  • Non breaking API changes
  • Tested

What did this pull request do?

Add brackets wrap when sql expression contains ||

User Case Description

  • single Expr without brackets wrapped.(follow previous rules)
  • multiple Expr and contains || will brackets wrapped.(new feature)

Playground Link

go-gorm/playground#728

Issue Link

#7002

@sprataa
Copy link

sprataa commented May 6, 2024

great find, but, I believe the reason why this is not covered in the first place is because || or && are not part of the ANSI SQL standard.

as an example, || is the concatenate operator in postgresql and && is not supported. in postgresql select 'this' || ' ' || 'is' = 'this is' yields the same result as select ('this' || ' ' || 'is') = 'this is'. logically select 1 = 1 || 2 = 2 yields a syntax error and select (1 = 1)::text || (2 = 2)::text yields the string 'truetrue' as supposed.

while this change you are proposing looks harmless, I don't believe there's any good reason to have these extra comparisons overhead for something which is not a standard for the engines gorm supports.

my 2 cents are, you should be using OR, AND and CONCAT so your SQL code is ANSI compliant and works with every engine.

cheers

@VarusHsu
Copy link
Author

VarusHsu commented May 9, 2024

My thanks for your answer. @sprataa I found this feature by accident and never read standard SQL docs before.That confused me(a green hand programmer). cheers

@VarusHsu VarusHsu closed this May 9, 2024
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.

None yet

2 participants