Skip to content

Commit

Permalink
Improve duplicate select column detection when using order by
Browse files Browse the repository at this point in the history
  • Loading branch information
@Jugen
Jugen committed May 17, 2024
1 parent 54840ca commit 29dadda
Show file tree
Hide file tree
Showing 3 changed files with 147 additions and 42 deletions.
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,5 +102,6 @@ CAY-2840 Vertical Inheritance: Missing subclass attributes with joint prefetch
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
186 changes: 145 additions & 41 deletions cayenne/src/main/java/org/apache/cayenne/access/translator/select/OrderingAbstractStage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -57,51 +58,154 @@ protected void processOrdering(QualifierTranslator qualifierTranslator, Translat
}
}

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;
OrderNodeVisitor orderVisitor = new OrderNodeVisitor();
nodeBuilder.build().visit( orderVisitor );
List<CharSequence> orderParts = orderVisitor.getParts();

return context.getResultNodeList().stream()
.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 class OrderNodeVisitor extends AppendableVisitor // see below
{
@Override
public boolean onNodeStart(Node node) {
node.append( this );
node.appendChildrenStart(this);
return true;
}

@Override
public void onChildNodeEnd(Node parent, Node child, int index, boolean hasMore) {
if (hasMore && parent != null) {
parent.appendChildrenSeparator(this, index);
}
});
return orderDbAttribute[0];
}

@Override
public void onNodeEnd(Node node) {
node.appendChildrenEnd(this);
}

List<CharSequence> getParts() {
return Collections.unmodifiableList(partList);
}
}

private boolean orderColumnAbsent(TranslatorContext context, NodeBuilder nodeBuilder)
private class ResultNodeVisitor extends AppendableVisitor // see below
{
var orderDbAttribute = getOrderDbAttribute(nodeBuilder.build());
if (orderDbAttribute == null) return false; // Alias ?
private List<CharSequence> orderItemParts;
private boolean itemsMatch = true;
private int lastIndex = 0;

var orderEntity = orderDbAttribute.getEntity().getName();
var orderColumn = orderDbAttribute.getName();
ResultNodeVisitor(List<CharSequence> orderParts) {
orderItemParts = orderParts;
}

Predicate<DbAttribute> columnAndEntity = dba -> dba != null
&& orderColumn.equals(dba.getName())
&& orderEntity.equals(dba.getEntity().getName());
@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();
}

var orderStr = getSqlString(order(nodeBuilder));
@Override
public void onChildNodeEnd(Node parent, Node child, int index, boolean hasMore) {
if (hasMore && parent != null) {
parent.appendChildrenSeparator(this, index);
isEqualSoFar();
}
}

return context.getResultNodeList().stream()
.filter( result -> columnAndEntity.test(result.getDbAttribute()) )
.noneMatch( result -> getSqlString(node(result.getNode())).equals(orderStr) );
@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 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();
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("|") );
}
}
}
2 changes: 1 addition & 1 deletion cayenne/src/test/java/org/apache/cayenne/access/translator/select/OrderingStageTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ public void testNoDuplicateColumnsWhenDistinct() {
ColumnExtractorStage columnStage = new ColumnExtractorStage();
columnStage.perform(context);

OrderingStage orderingStage = new OrderingStage();
OrderingDistictStage orderingStage = new OrderingDistictStage();
orderingStage.perform(context);

assertTrue(context.getQuery().isDistinct());
Expand Down

0 comments on commit 29dadda

Please sign in to comment.