querydsl / querydsl

Unified Queries for Java
https://querydsl.com
Apache License 2.0
4.71k stars 869 forks source link

BooleanBuilder OR condition generate invalid query #1435

Closed glau2 closed 9 years ago

glau2 commented 9 years ago

Hi,

I have encounter the following problem:

QueryDSL version: 3.6.5 Using the following libraries: querydsl-core querydsl-jdo querydsl-apt

{
    System.out.println("=== Test AND ===");
    BooleanBuilder builder = new BooleanBuilder();
    for (Predicate predicate : predicates) {
        builder.and(predicate);
        System.out.println(builder.getValue());
    }
}

{
    System.out.println("=== Test OR ===");
    BooleanBuilder builder = new BooleanBuilder();
    for (Predicate predicate : predicates) {
        builder.or(predicate);
        System.out.println(builder.getValue());
    }
}

The 2 predicates that i have in the ArrayList of "predicates" is the following: a) contains(lower(investorAccount.name),pf791798) b) contains(lower(investorAccount.ownedByUser.name),123) || contains(lower(investorAccount.ownedByUser.firstName),123)

This generate the following result:

=== Test AND === contains(lower(investorAccount.name),pf791798) && (contains(lower(investorAccount.ownedByUser.name),123) || contains(lower(investorAccount.ownedByUser.firstName),123)) === Test OR === contains(lower(investorAccount.name),pf791798) || contains(lower(investorAccount.ownedByUser.name),123) || contains(lower(investorAccount.ownedByUser.firstName),123)

in "Test AND", the query are generated correct as "a && (b)". However in "Test OR" the query is generated as "a || b", there are no bracket around predicate "b". The expected result should be "a || (b)"

Please could you have a look into this. And if you think my understanding is incorrect, please also let me know.

Please also let me know if you would need me to provide with more information

Many thanks!!!!

zamrokk commented 9 years ago

i join this issue, priority on parenthesis are not respected. This change the result of the query and is quite important bug. I don't agree also with the issue #690 on too many parenthesis generation. Parenthesis have their importancy cause they give priorities on boolean expression. As the code is generated, too many parenthesis doesn't annoy anyone

glau2 commented 9 years ago

Hi,

Can someone confirm if this is a bug or just my understanding of the issue is wrong. Many thanks!

Shredder121 commented 9 years ago

Since ORs have the same precedence, it doesn't matter if you say true OR (false OR true), (true OR false) OR true or just plain true OR false OR true, since for OR to work any predicate must be true, you can however reorder them via true or true or false, given that the logic is still correct (you wouldn't for example first check the boolean value, and then check if it's null).

Querydsl doesn't reorder statements, and only ORs from left to right don't need parentheses.

We could however disambiguate statements by generating parentheses when we enter a subtree with the same precedence value @timowest ?

timowest commented 9 years ago

@glau2 Where do you think should we add more parentheses?

@Shredder121 Querydsl always assumes left associative rendering. The rule is that the parenthesis for the left subtree can be skipped if the precedence is the same. For the right subtree the same precedence requires rendering of parentheses. Usually the more complex expressions are on the left side, so the amount of parentheses is minimized.

zamrokk commented 9 years ago

true OR true AND false => true (true OR true) AND false => false

do we agree with mathematical importance of parenthesis on "AND" is prior to "OR" operations for SQL ? and not on left evaluation processment

Example to run on a DB :+1: CREATE TABLE pet (name VARCHAR(20), owner VARCHAR(20), species VARCHAR(20));

insert into pet values('dog','me','dog');

select * from pet where true or true and false;

select * from pet where (true or true) and false;

select * from pet where true or (true and false);


result : line 1 and 3 return a result, line 2 is empty recordset

This is proving mathematical priorities on parenthesis done by the database

timowest commented 9 years ago

@zamrokk AND has higher precedence then OR, so

(a OR b) AND c

is not equivalent to

a OR b AND c

Querydsl respects the operator precedence and takes left associativity into account when serializing compound operations with nested operations with the same precedence.

timowest commented 9 years ago

I am closing this, since there doesn't seem to be any further action required.

yourganesan commented 7 years ago

It is a valid issue. If i do where.(e1).or(e2.and(e3), it is generating like where e1 or e2 and e3 which is wrong. What we need is where e1 or (e2 and e3). Can this issue be re-opened or new ticket is required?

Shredder121 commented 7 years ago

and has higher precedence though, the parentheses are not necessary so for cleanness sake are not included.

finatofinato commented 5 years ago

Hello, Does the fix for this issue has been done? The version of querydsl I am using is 4.2.1. I am having the same problem. Part of my query is something like:

QPedido pedido = QPedido.pedido;
BooleanExpression origem = pedido.fornecedorOrigem.cnpj.eq(cnpjMobile).and(pedido.parent.isNull());
BooleanExpression destino = pedido.fornecedorDestino.cnpj.eq(cnpjMobile).and(pedido.parent.isNotNull());
BooleanExpression ou = origem.or(destino);
return where.and(ou); 

and the GENERATED SQL is:

from
            pedido pedido0_,
            fornecedor fornecedor1_,
            fornecedor fornecedor2_ //it is generated without left join, what should be mandatory to generate the correct parentheses and results)
...
and (fornecedor1_.cnpj='xxxxxxxx' and pedido0_.parent_id is null 
or 
fornecedor2_.cnpj='xxxxxxxx' and pedido0_.parent_id is not null)

but what I need to be generated is (with parentheses separating (AND) - OR - (AND)):

from
        pedido pedido0_ 
        left join fornecedor fornecedor1_ on (pedido0_.fornecedor_origem_id=fornecedor1_.id)
        left join fornecedor fornecedor2_ on (pedido0_.fornecedor_destino_id=fornecedor2_.id)
...
and ( **(**fornecedor1_.cnpj='xxxxxxxx' and pedido0_.parent_id is null**)**  
or 
 **(**fornecedor2_.cnpj='xxxxxxxx'   and   pedido0_.parent_id is not null**)**   )

Should I create a left join manually? (I think I'm talking bulls**t :\ ) Is there a way to do that without manually create the left join?

Shredder121 commented 5 years ago
I don't see what you mean https://www.db-fiddle.com/f/n1BLLPTsw5SHzA9uCd7y8A/3 **Schema (MySQL v5.7)** ```sql create table Matrix ( value1 int, value2 int, value3 int ); insert into Matrix (value1, value2, value3) values (1, 1, 2), (2, 2, 2), (1, 2, 3), (2, 1, 2); ``` --- **Query # 1** ```sql select * from Matrix where value1 = value2 and value2 = value3 or value3 = value1 and value3 = value2; ``` | value1 | value2 | value3 | | ------ | ------ | ------ | | 2 | 2 | 2 | --- **Query \# 2** ```sql select * from Matrix where (value1 = value2 and value2 = value3) or (value3 = value1 and value3 = value2); ``` | value1 | value2 | value3 | | ------ | ------ | ------ | | 2 | 2 | 2 | --- [View on DB Fiddle](https://www.db-fiddle.com/f/n1BLLPTsw5SHzA9uCd7y8A/3)

And has higher precedence than or, so really the parentheses are superfluous, unnecessary, they do the same as not having them. What results are you seeing that should have been filtered out? Maybe you can also make a db fiddle?

finatofinato commented 5 years ago

I don't see what you mean https://www.db-fiddle.com/f/n1BLLPTsw5SHzA9uCd7y8A/3

And has higher precedence than or, so really the parentheses are superfluous, unnecessary, they do the same as not having them. What results are you seeing that should have been filtered out? Maybe you can also make a db fiddle?

sorry my mistake, I just edited and completed my post it was missing the left join and related joins code

Shredder121 commented 5 years ago

it is generated without left join, what should be mandatory to generate the correct parentheses and results

parentheses and results are 2 different things here could you first do the left join, and see if there are results that are incorrect? then we can see if more parentheses will fix that

finatofinato commented 5 years ago

it is generated without left join, what should be mandatory to generate the correct parentheses and results

parentheses and results are 2 different things here could you first do the left join, and see if there are results that are incorrect? then we can see if more parentheses will fix that

My code had an error. A mapping from Fornecedor entity was wrong. Now it is working as expected. Sorry for my unnecessary comments.

hongshengfeng commented 4 years ago

hello, although AND has higher precedence then OR, but how can i transfer it sql: e1 and e2 or ( e3 and e4 or e7)