Merge pull request #605 from Jugen/remove-duplicate-columns-on-distin…

…ct-and-orderBy CAY-2842 Prevent duplicate select columns when using distinct with order by
apache · Feb 20, 2024 · 8faf719 · 8faf719
2 parents 9dd8eae + 99138fb
commit 8faf719
Show file tree

Hide file tree

Showing 11 changed files with 263 additions and 53 deletions.
diff --git a/cayenne/src/main/java/org/apache/cayenne/access/sqlbuilder/sqltree/AliasedNode.java b/cayenne/src/main/java/org/apache/cayenne/access/sqlbuilder/sqltree/AliasedNode.java
@@ -80,7 +80,7 @@ private boolean skipContent() {
 
         // check if we have subselect as a child
         for(Node child : children) {
-            if(child.getType() == NodeType.SELECT) {
+            if(child != null && child.getType() == NodeType.SELECT) {
                 return false;
             }
         }

diff --git a/...ne/src/main/java/org/apache/cayenne/access/translator/select/DefaultSelectTranslator.java b/...ne/src/main/java/org/apache/cayenne/access/translator/select/DefaultSelectTranslator.java
@@ -43,8 +43,10 @@ public class DefaultSelectTranslator implements SelectTranslator {
             new OrderingStage(),
             new QualifierTranslationStage(),
             new HavingTranslationStage(),
+            new OrderingGroupByStage(),
             new GroupByStage(),
             new DistinctStage(),
+            new OrderingDistictStage(),
             new LimitOffsetStage(),
             new ColumnDescriptorStage(),
             new TableTreeQualifierStage(),

diff --git a/cayenne/src/main/java/org/apache/cayenne/access/translator/select/DistinctStage.java b/cayenne/src/main/java/org/apache/cayenne/access/translator/select/DistinctStage.java
@@ -20,7 +20,6 @@
 package org.apache.cayenne.access.translator.select;
 
 import java.sql.Types;
-import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
  * @since 4.2
@@ -55,7 +54,7 @@ public void perform(TranslatorContext context) {
         }
 
         // query forcing distinct or query have joins (qualifier or prefetch)
-        if(!context.getQuery().isDistinct() && !hasToManyJoin(context)) {
+        if(!context.getQuery().isDistinct() && !context.getTableTree().hasToManyJoin()) {
             return;
         }
 
@@ -69,18 +68,4 @@ public void perform(TranslatorContext context) {
         }
         context.getSelectBuilder().distinct();
     }
-
-    private boolean hasToManyJoin(TranslatorContext context) {
-        if(context.getTableTree().getNodeCount() <= 1) {
-            return false;
-        }
-
-        AtomicBoolean atomicBoolean = new AtomicBoolean(false);
-        context.getTableTree().visit(node -> {
-            if(node.getRelationship() != null && node.getRelationship().isToMany()) {
-                atomicBoolean.set(true);
-            }
-        });
-        return atomicBoolean.get();
-    }
 }
diff --git a/cayenne/src/main/java/org/apache/cayenne/access/translator/select/GroupByStage.java b/cayenne/src/main/java/org/apache/cayenne/access/translator/select/GroupByStage.java
@@ -19,17 +19,14 @@
 
 package org.apache.cayenne.access.translator.select;
 
-import org.apache.cayenne.exp.parser.ASTAggregateFunctionCall;
-import org.apache.cayenne.query.Ordering;
-
 /**
  * @since 4.2
  */
 class GroupByStage implements TranslationStage {
 
     @Override
     public void perform(TranslatorContext context) {
-        if (!hasAggregate(context)) {
+        if (!context.hasAggregate()) {
             return;
         }
 
@@ -41,25 +38,4 @@ public void perform(TranslatorContext context) {
         }
     }
 
-    private boolean hasAggregate(TranslatorContext context) {
-        if(context.getQuery().getHavingQualifier() != null) {
-            return true;
-        }
-
-        for(ResultNodeDescriptor resultNode : context.getResultNodeList()) {
-            if(resultNode.isAggregate()) {
-                return true;
-            }
-        }
-
-        if(context.getQuery().getOrderings() != null) {
-            for(Ordering ordering : context.getQuery().getOrderings()) {
-                if(ordering.getSortSpec() instanceof ASTAggregateFunctionCall) {
-                    return true;
-                }
-            }
-        }
-
-        return false;
-    }
 }
diff --git a/cayenne/src/main/java/org/apache/cayenne/access/translator/select/OrderingAbstractStage.java b/cayenne/src/main/java/org/apache/cayenne/access/translator/select/OrderingAbstractStage.java
@@ -0,0 +1,107 @@
+/*****************************************************************
+ *   Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *    https://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ ****************************************************************/
+
+package org.apache.cayenne.access.translator.select;
+
+import static org.apache.cayenne.access.sqlbuilder.SQLBuilder.*;
+
+import java.util.function.Predicate;
+
+import org.apache.cayenne.access.sqlbuilder.NodeBuilder;
+import org.apache.cayenne.access.sqlbuilder.SQLGenerationVisitor;
+import org.apache.cayenne.access.sqlbuilder.StringBuilderAppendable;
+import org.apache.cayenne.access.sqlbuilder.sqltree.ColumnNode;
+import org.apache.cayenne.access.sqlbuilder.sqltree.FunctionNode;
+import org.apache.cayenne.access.sqlbuilder.sqltree.Node;
+import org.apache.cayenne.access.sqlbuilder.sqltree.NodeType;
+import org.apache.cayenne.access.sqlbuilder.sqltree.SelectResultNode;
+import org.apache.cayenne.access.sqlbuilder.sqltree.SimpleNodeTreeVisitor;
+import org.apache.cayenne.exp.Expression;
+import org.apache.cayenne.exp.parser.ASTAggregateFunctionCall;
+import org.apache.cayenne.map.DbAttribute;
+import org.apache.cayenne.query.Ordering;
+
+abstract class OrderingAbstractStage implements TranslationStage {
+
+    protected void processOrdering(QualifierTranslator qualifierTranslator, TranslatorContext context, Ordering ordering) {
+        Expression orderExp = ordering.getSortSpec();
+        NodeBuilder nodeBuilder = node(qualifierTranslator.translate(orderExp));
+
+        if(ordering.isCaseInsensitive()) {
+            nodeBuilder = function("UPPER", nodeBuilder);
+        }
+
+        // If query is DISTINCT or GROUPING then we need to add the order column as a result column
+        if (orderColumnAbsent(context, nodeBuilder)) {
+            // deepCopy as some DB expect exactly the same expression in select and in ordering
+            ResultNodeDescriptor descriptor = context.addResultNode(nodeBuilder.build().deepCopy());
+            if(orderExp instanceof ASTAggregateFunctionCall) {
+                descriptor.setAggregate(true);
+            }
+        }
+    }
+
+    private DbAttribute getOrderDbAttribute(Node translatedOrderNode)
+    {
+        DbAttribute[] orderDbAttribute = {null};
+        translatedOrderNode.visit(new SimpleNodeTreeVisitor() {
+            @Override
+            public boolean onNodeStart(Node node) {
+                if (node.getType() == NodeType.COLUMN) {
+                    orderDbAttribute[0] = ((ColumnNode) node).getAttribute();
+                    return false;
+                }
+                return true;
+            }
+        });
+        return orderDbAttribute[0];
+    }
+
+    private boolean orderColumnAbsent(TranslatorContext context, NodeBuilder nodeBuilder)
+    {
+        var orderDbAttribute = getOrderDbAttribute(nodeBuilder.build());
+        if (orderDbAttribute == null) return false; // Alias ?
+
+        var orderEntity = orderDbAttribute.getEntity().getName();
+        var orderColumn = orderDbAttribute.getName();
+
+        Predicate<DbAttribute> columnAndEntity = dba -> dba != null
+                                    && orderColumn.equals(dba.getName())
+                                    && orderEntity.equals(dba.getEntity().getName());
+
+        var orderStr = getSqlString(order(nodeBuilder));
+
+        return context.getResultNodeList().stream()
+            .filter( result -> columnAndEntity.test(result.getDbAttribute()) )
+            .noneMatch( result -> getSqlString(node(result.getNode())).equals(orderStr) );
+    }
+
+    private String getSqlString(NodeBuilder nb) {
+        var node = nb.build();
+        if (node instanceof FunctionNode && ((FunctionNode) node).getAlias() != null)
+        {
+            // Wrap in result node otherwise content isn't generated, only alias
+            node = new SelectResultNode().addChild(node.deepCopy());
+        }
+        var strBuilder = new StringBuilderAppendable();
+        var sqlVisitor = new SQLGenerationVisitor(strBuilder);
+        node.visit(sqlVisitor);
+        return strBuilder.append(' ').toString();
+    }
+}
diff --git a/cayenne/src/main/java/org/apache/cayenne/access/translator/select/OrderingDistictStage.java b/cayenne/src/main/java/org/apache/cayenne/access/translator/select/OrderingDistictStage.java
@@ -0,0 +1,51 @@
+/*****************************************************************
+ *   Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *    https://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ ****************************************************************/
+
+package org.apache.cayenne.access.translator.select;
+
+import org.apache.cayenne.query.Ordering;
+
+import static org.apache.cayenne.access.sqlbuilder.SQLBuilder.*;
+
+/**
+ * @since 4.2.1
+ */
+class OrderingDistictStage extends OrderingAbstractStage {
+
+    @Override
+    public void perform(TranslatorContext context) {
+        if(context.getQuery().getOrderings() == null) {
+            return;
+        }
+
+        if (isDistinct(context)) {
+            // If query is DISTINCT then we need to add the order column as a result column
+            QualifierTranslator qualifierTranslator = context.getQualifierTranslator();
+            for(Ordering ordering : context.getQuery().getOrderings()) {
+                processOrdering(qualifierTranslator, context, ordering);
+            }
+        }
+    }
+
+    private boolean isDistinct(TranslatorContext context) {
+        return !context.isDistinctSuppression()
+            && (context.getQuery().isDistinct()
+            || context.getTableTree().hasToManyJoin());
+    }
+}
diff --git a/cayenne/src/main/java/org/apache/cayenne/access/translator/select/OrderingGroupByStage.java b/cayenne/src/main/java/org/apache/cayenne/access/translator/select/OrderingGroupByStage.java
@@ -0,0 +1,44 @@
+/*****************************************************************
+ *   Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *    https://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ ****************************************************************/
+
+package org.apache.cayenne.access.translator.select;
+
+import org.apache.cayenne.query.Ordering;
+
+/**
+ * @since 4.2.1
+ */
+class OrderingGroupByStage extends OrderingAbstractStage {
+
+    @Override
+    public void perform(TranslatorContext context) {
+        if(context.getQuery().getOrderings() == null) {
+            return;
+        }
+
+        if (context.hasAggregate()) {
+            // If query is GROUPING then we need to add the order column as a result column
+            QualifierTranslator qualifierTranslator = context.getQualifierTranslator();
+            for(Ordering ordering : context.getQuery().getOrderings()) {
+                processOrdering(qualifierTranslator, context, ordering);
+            }
+        }
+    }
+
+}
diff --git a/cayenne/src/main/java/org/apache/cayenne/access/translator/select/OrderingStage.java b/cayenne/src/main/java/org/apache/cayenne/access/translator/select/OrderingStage.java
@@ -23,7 +23,6 @@
 import org.apache.cayenne.access.sqlbuilder.OrderingNodeBuilder;
 import org.apache.cayenne.access.sqlbuilder.sqltree.Node;
 import org.apache.cayenne.exp.Expression;
-import org.apache.cayenne.exp.parser.ASTAggregateFunctionCall;
 import org.apache.cayenne.query.Ordering;
 
 import static org.apache.cayenne.access.sqlbuilder.SQLBuilder.*;
@@ -54,16 +53,6 @@ private void processOrdering(QualifierTranslator qualifierTranslator, Translator
             nodeBuilder = function("UPPER", nodeBuilder);
         }
 
-        // If query is DISTINCT then we need to add all ORDER BY clauses as result columns
-        if(!context.isDistinctSuppression()) {
-            // TODO: need to check duplicates?
-            // need UPPER() function here too, as some DB expect exactly the same expression in select and in ordering
-            ResultNodeDescriptor descriptor = context.addResultNode(nodeBuilder.build().deepCopy());
-            if(exp instanceof ASTAggregateFunctionCall) {
-                descriptor.setAggregate(true);
-            }
-        }
-
         OrderingNodeBuilder orderingNodeBuilder = order(nodeBuilder);
         if(ordering.isDescending()) {
             orderingNodeBuilder.desc();

diff --git a/cayenne/src/main/java/org/apache/cayenne/access/translator/select/TableTree.java b/cayenne/src/main/java/org/apache/cayenne/access/translator/select/TableTree.java
@@ -21,6 +21,7 @@
 
 import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.stream.Collectors;
 
 import org.apache.cayenne.CayenneRuntimeException;
@@ -96,6 +97,20 @@ public int getNodeCount() {
         return tableNodes.size() + 1;
     }
 
+    boolean hasToManyJoin() {
+        if(getNodeCount() <= 1) {
+            return false;
+        }
+
+        AtomicBoolean atomicBoolean = new AtomicBoolean(false);
+        visit(node -> {
+            if(node.getRelationship() != null && node.getRelationship().isToMany()) {
+                atomicBoolean.set(true);
+            }
+        });
+        return atomicBoolean.get();
+    }
+
     public void visit(TableNodeVisitor visitor) {
         visitor.visit(rootNode);