CAY-2972 Fewer parentheses in generated SQL

Author: andrus

RELEASE-NOTES.txt

@@ -22,6 +22,7 @@ CAY-2963 Replace TypesHandler / types.xml with hardcoded map
 CAY-2969 Extender API for "soft" delete 
 CAY-2970 Tighten deferred value resolution contract on commit
 CAY-2971 Remove extra spaces within SQL parenthesis
+CAY-2972 Fewer parentheses in generated SQL
 
 Bug Fixes:
 

cayenne/src/main/java/org/apache/cayenne/access/sqlbuilder/sqltree/BetweenNode.java

@@ -47,6 +47,11 @@ public void appendChildrenSeparator(SQLAppendable buffer, int childIdx) {
         }
     }
 
+    @Override
+    protected boolean isComparison() {
+        return true;
+    }
+
     @Override
     public Node copy() {
         return new BetweenNode(not);

cayenne/src/main/java/org/apache/cayenne/access/sqlbuilder/sqltree/ElseNode.java

@@ -26,6 +26,10 @@
  */
 public class ElseNode extends Node {
 
+    public ElseNode() {
+        super(NodeType.ELSE);
+    }
+
     @Override
     public SQLAppendable append(SQLAppendable buffer) {
         return buffer.appendTokenSeparator().append("ELSE");

cayenne/src/main/java/org/apache/cayenne/access/sqlbuilder/sqltree/ExpressionNode.java

@@ -41,24 +41,57 @@ public SQLAppendable append(SQLAppendable buffer) {
 
     @Override
     public void appendChildrenStart(SQLAppendable buffer) {
-        if(parent != null
-                && parent.type != NodeType.WHERE
-                && parent.type != NodeType.JOIN
-                && parent.type != NodeType.UPDATE_SET) {
+        if (needsParentheses()) {
             buffer.appendTokenSeparator().append('(').suppressNextTokenSeparator();
         }
     }
 
     @Override
     public void appendChildrenEnd(SQLAppendable buffer) {
-        if(parent != null
-                && parent.type != NodeType.WHERE
-                && parent.type != NodeType.JOIN
-                && parent.type != NodeType.UPDATE_SET) {
+        if (needsParentheses()) {
             buffer.append(")");
         }
     }
 
+    protected boolean needsParentheses() {
+        if (parent == null
+                || parent.type == NodeType.WHERE
+                || parent.type == NodeType.JOIN
+                || parent.type == NodeType.UPDATE_SET
+                || parent.type == NodeType.WHEN
+                || parent.type == NodeType.THEN
+                || parent.type == NodeType.ELSE) {
+            return false;
+        }
+
+        if (parent instanceof ExpressionNode parentExpr) {
+            String parentOp = parentExpr.logicalOperator();
+            if (parentOp != null) {
+                if (isComparison()) {
+                    return false;
+                }
+
+                return !parentOp.equals(logicalOperator());
+            }
+        }
+        return true;
+    }
+
+    /**
+     * @return {@code "AND"} or {@code "OR"} if this node is a logical connective, otherwise {@code null}.
+     */
+    protected String logicalOperator() {
+        return null;
+    }
+
+    /**
+     * @return true for comparison operators that bind tighter than {@code AND}/{@code OR} and therefore don't need
+     * parentheses when nested directly under them.
+     */
+    protected boolean isComparison() {
+        return type == NodeType.EQUALITY || type == NodeType.LIKE;
+    }
+
     @Override
     public String toString() {
         return "{ExpressionNode}";

cayenne/src/main/java/org/apache/cayenne/access/sqlbuilder/sqltree/NodeType.java

@@ -40,5 +40,8 @@ public enum NodeType {
     INSERT_COLUMNS,
     INSERT_VALUES,
     ORDER_BY,
-    SELECT
+    SELECT,
+    WHEN,
+    THEN,
+    ELSE
 }

cayenne/src/main/java/org/apache/cayenne/access/sqlbuilder/sqltree/OpExpressionNode.java

@@ -39,6 +39,19 @@ public void appendChildrenSeparator(SQLAppendable buffer, int childInd) {
         buffer.appendTokenSeparator().append(op);
     }
 
+    @Override
+    protected String logicalOperator() {
+        return "AND".equals(op) || "OR".equals(op) ? op : null;
+    }
+
+    @Override
+    protected boolean isComparison() {
+        return switch (op) {
+            case "<", "<=", ">", ">=" -> true;
+            default -> false;
+        };
+    }
+
     @Override
     public Node copy() {
         return new OpExpressionNode(op);

cayenne/src/main/java/org/apache/cayenne/access/sqlbuilder/sqltree/ThenNode.java

@@ -26,6 +26,10 @@
  */
 public class ThenNode extends Node {
 
+    public ThenNode() {
+        super(NodeType.THEN);
+    }
+
     @Override
     public SQLAppendable append(SQLAppendable buffer) {
         return buffer.appendTokenSeparator().append("THEN");

cayenne/src/main/java/org/apache/cayenne/access/sqlbuilder/sqltree/WhenNode.java

@@ -26,6 +26,10 @@
  */
 public class WhenNode extends Node {
 
+    public WhenNode() {
+        super(NodeType.WHEN);
+    }
+
     @Override
     public SQLAppendable append(SQLAppendable buffer) {
         return buffer.appendTokenSeparator().append("WHEN");

cayenne/src/test/java/org/apache/cayenne/access/sqlbuilder/DeleteBuilderTest.java

@@ -46,7 +46,7 @@ public void deleteWithQualifier() {
                         .and(column("col3").eq(value(null)))
         ).build();
         assertInstanceOf(DeleteNode.class, node);
-        assertSQL("DELETE FROM test WHERE ((col1 = 1) AND (col2 = 'test')) AND (col3 IS NULL)", node);
+        assertSQL("DELETE FROM test WHERE col1 = 1 AND col2 = 'test' AND col3 IS NULL", node);
     }
 
     @Test

cayenne/src/test/java/org/apache/cayenne/access/sqlbuilder/SelectBuilderTest.java

@@ -96,7 +96,7 @@ public void selectFromWhereComplex() {
                 .where(column("a").eq(value(123)).and(column("c").lt(column("d"))));
         Node node = builder.build();
         assertInstanceOf(SelectNode.class, node);
-        assertSQL("SELECT a FROM b WHERE (a = 123) AND (c < d)", node);
+        assertSQL("SELECT a FROM b WHERE a = 123 AND c < d", node);
     }
 
     @Test
@@ -112,13 +112,14 @@ public void validSelectCaseWhen() {
 
         Node node = builder.build();
         assertInstanceOf(SelectNode.class, node);
-        assertSQL("SELECT OrderID, Quantity, " +
-                "CASE " +
-                    "WHEN ((Quantity > 30) AND (Quantity < 100)) THEN 'The quantity from 30 to 100' " +
-                    "WHEN (Quantity = 30) THEN 'The quantity is 30' " +
-                    "ELSE 'The quantity is under 30' " +
-                    "END QuantityText " +
-                "FROM OrderDetails", node);
+        assertSQL("""
+                SELECT OrderID, Quantity, \
+                CASE \
+                WHEN Quantity > 30 AND Quantity < 100 THEN 'The quantity from 30 to 100' \
+                WHEN Quantity = 30 THEN 'The quantity is 30' \
+                ELSE 'The quantity is under 30' \
+                END QuantityText \
+                FROM OrderDetails""", node);
     }
 
     @Test
@@ -179,16 +180,17 @@ public void complexQuery() {
                 .orderBy(count(table("p").column("PAINTING_TITLE")).as("p_count").desc(), column("a_id").asc())
                 .build();
         assertInstanceOf(SelectNode.class, node);
-        assertSQL("SELECT DISTINCT" +
-                    " a.ARTIST_ID a_id, COUNT(p.PAINTING_TITLE) p_count" +
-                " FROM ARTIST a" +
-                " LEFT JOIN PAINTING p ON (a.ARTIST_ID = p.ARTIST_ID) AND (p.ESTIMATED_PRICE > 10)" +
-                " WHERE (((a.ARTIST_NAME = 'Picasso')" +
-                    " AND EXISTS (SELECT * FROM GALLERY g WHERE g.GALLERY_ID = p.GALLERY_ID))" +
-                    " AND (1 = 1)) OR false" +
-                " GROUP BY a.ARTIST_ID" +
-                " HAVING NOT (COUNT(p.PAINTING_TITLE) > 3)" +
-                " ORDER BY p_count DESC, a_id", node);
+        assertSQL("""
+                SELECT DISTINCT \
+                a.ARTIST_ID a_id, COUNT(p.PAINTING_TITLE) p_count \
+                FROM ARTIST a \
+                LEFT JOIN PAINTING p ON a.ARTIST_ID = p.ARTIST_ID AND p.ESTIMATED_PRICE > 10 \
+                WHERE (a.ARTIST_NAME = 'Picasso' \
+                AND EXISTS (SELECT * FROM GALLERY g WHERE g.GALLERY_ID = p.GALLERY_ID) \
+                AND 1 = 1) OR false \
+                GROUP BY a.ARTIST_ID \
+                HAVING NOT (COUNT(p.PAINTING_TITLE) > 3) \
+                ORDER BY p_count DESC, a_id""", node);
     }
 
 }