Improve duplicate select column detection when using order by
diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt
index 1dd7aa3..1c01a9d 100644
--- a/RELEASE-NOTES.txt
+++ b/RELEASE-NOTES.txt
@@ -102,5 +102,6 @@
CAY-2841 Multi column ColumnSelect with SHARED_CACHE fails after 1st select
CAY-2844 Joint prefetch doesn't use ObjEntity qualifier
CAY-2842 Prevent duplicate select columns when using distinct with order by
+CAY-2847 Improve duplicate select column detection when using order by
CAY-2850 Query using Clob comparison with empty String fails
CAY-2851 Replace Existing OneToOne From New Object
\ No newline at end of file
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
index 101f779..4ef9060 100644
--- 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
@@ -21,20 +21,21 @@
import static org.apache.cayenne.access.sqlbuilder.SQLBuilder.*;
-import java.util.function.Predicate;
+import java.util.Collections;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.stream.Collectors;
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.QuotingAppendable;
+import org.apache.cayenne.access.sqlbuilder.SQLGenerationContext;
+import org.apache.cayenne.access.sqlbuilder.sqltree.AliasedNode;
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 {
@@ -57,51 +58,154 @@
}
}
- private DbAttribute getOrderDbAttribute(Node translatedOrderNode)
+ private boolean orderColumnAbsent(TranslatorContext context, NodeBuilder nodeBuilder)
{
- 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));
+ OrderNodeVisitor orderVisitor = new OrderNodeVisitor();
+ nodeBuilder.build().visit( orderVisitor );
+ List<CharSequence> orderParts = orderVisitor.getParts();
return context.getResultNodeList().stream()
- .filter( result -> columnAndEntity.test(result.getDbAttribute()) )
- .noneMatch( result -> getSqlString(node(result.getNode())).equals(orderStr) );
+ .noneMatch( result -> {
+ ResultNodeVisitor resultVisitor = new ResultNodeVisitor(orderParts);
+ // Visitor aborts as soon as there's a mismatch with orderParts
+ result.getNode().visit(resultVisitor);
+ return resultVisitor.matches();
+ });
}
- 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());
+ private class OrderNodeVisitor extends AppendableVisitor // see below
+ {
+ @Override
+ public boolean onNodeStart(Node node) {
+ node.append( this );
+ node.appendChildrenStart(this);
+ return true;
}
- var strBuilder = new StringBuilderAppendable();
- var sqlVisitor = new SQLGenerationVisitor(strBuilder);
- node.visit(sqlVisitor);
- return strBuilder.append(' ').toString();
+
+ @Override
+ public void onChildNodeEnd(Node parent, Node child, int index, boolean hasMore) {
+ if (hasMore && parent != null) {
+ parent.appendChildrenSeparator(this, index);
+ }
+ }
+
+ @Override
+ public void onNodeEnd(Node node) {
+ node.appendChildrenEnd(this);
+ }
+
+ List<CharSequence> getParts() {
+ return Collections.unmodifiableList(partList);
+ }
+ }
+
+ private class ResultNodeVisitor extends AppendableVisitor // see below
+ {
+ private List<CharSequence> orderItemParts;
+ private boolean itemsMatch = true;
+ private int lastIndex = 0;
+
+ ResultNodeVisitor(List<CharSequence> orderParts) {
+ orderItemParts = orderParts;
+ }
+
+ @Override
+ public boolean onNodeStart(Node node) {
+ node.append(this);
+ if (node instanceof ColumnNode && ((ColumnNode) node).getAlias() != null) {
+ partList.removeLast(); // Remove appended alias
+ }
+ if (!(node.getParent() instanceof AliasedNode)) {
+ // Prevent appending opening bracket
+ node.appendChildrenStart(this);
+ }
+ return isEqualSoFar();
+ }
+
+ @Override
+ public void onChildNodeEnd(Node parent, Node child, int index, boolean hasMore) {
+ if (hasMore && parent != null) {
+ parent.appendChildrenSeparator(this, index);
+ isEqualSoFar();
+ }
+ }
+
+ @Override
+ public void onNodeEnd(Node node) {
+ // Prevent appending alias or closing bracket
+ if (!(node instanceof AliasedNode || node.getParent() instanceof AliasedNode)) {
+ node.appendChildrenEnd(this);
+ if (node instanceof FunctionNode && ((FunctionNode) node).getAlias() != null) {
+ if (partList.getLast().equals(((FunctionNode) node).getAlias())) {
+ partList.removeLast(); // Remove appended alias
+ }
+ }
+ isEqualSoFar();
+ }
+ }
+
+ private boolean isEqualSoFar() {
+ int currentSize = partList.size();
+ if (currentSize == lastIndex) return itemsMatch;
+ if (currentSize > orderItemParts.size()) itemsMatch = false;
+ if (itemsMatch) {
+ // In reverse to fail fast by hopefully comparing column names first
+ for (int x = currentSize-1; x >= lastIndex; x--) {
+ if (!partList.get(x).equals(orderItemParts.get(x))) {
+ itemsMatch = false;
+ break;
+ }
+ }
+ }
+ lastIndex = partList.size();
+ return itemsMatch;
+ }
+
+ boolean matches() {
+ return isEqualSoFar();
+ }
+ }
+
+ private class AppendableVisitor extends SimpleNodeTreeVisitor implements QuotingAppendable
+ {
+ protected final LinkedList<CharSequence> partList = new LinkedList<>();
+
+ @Override
+ public QuotingAppendable append(CharSequence csq) {
+ partList.add(csq);
+ return this;
+ }
+
+ @Override
+ public QuotingAppendable append(CharSequence csq, int start, int end) {
+ return this;
+ }
+
+ @Override
+ public QuotingAppendable append(char c) {
+ if (c != '.' && c != ' ') partList.add(String.valueOf(c));
+ return this;
+ }
+
+ @Override
+ public QuotingAppendable append(int c) {
+ return this;
+ }
+
+ @Override
+ public QuotingAppendable appendQuoted(CharSequence csq) {
+ partList.add(csq);
+ return this;
+ }
+
+ @Override
+ public SQLGenerationContext getContext() {
+ return null;
+ }
+
+ @Override
+ public String toString() {
+ return partList.stream().collect( Collectors.joining("|") );
+ }
}
}
diff --git a/cayenne/src/test/java/org/apache/cayenne/access/translator/select/OrderingStageTest.java b/cayenne/src/test/java/org/apache/cayenne/access/translator/select/OrderingStageTest.java
index 2d15f42..c360fd2 100644
--- a/cayenne/src/test/java/org/apache/cayenne/access/translator/select/OrderingStageTest.java
+++ b/cayenne/src/test/java/org/apache/cayenne/access/translator/select/OrderingStageTest.java
@@ -115,7 +115,7 @@
ColumnExtractorStage columnStage = new ColumnExtractorStage();
columnStage.perform(context);
- OrderingStage orderingStage = new OrderingStage();
+ OrderingDistictStage orderingStage = new OrderingDistictStage();
orderingStage.perform(context);
assertTrue(context.getQuery().isDistinct());
