Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1516153957
##
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 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())).startsWith(orderStr) );
Review Comment:
Thank you! I've seen it but unfortunately didn't have time to review it.
Hope will get some free time to check it out in the next few days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service,
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1516017214
##
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 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())).startsWith(orderStr) );
Review Comment:
@stariy95 Have submitted PR #610 (ready for review) against
[CAY-2847](https://issues.apache.org/jira/browse/CAY-2847) to address this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1507364687
##
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 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())).startsWith(orderStr) );
Review Comment:
@Jugen If it's more than a few lines of code, then it's better to open a new
Jira issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1507359482
##
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 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())).startsWith(orderStr) );
Review Comment:
@stariy95 I'm ready to submit a new PR for the Node.visit pattern but need
some guidance on how to go about it ?
Should I create a new Jira RFE issue and submit against that, or just do an
ordinary PR ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail:
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 merged PR #605: URL: https://github.com/apache/cayenne/pull/605 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1494864476
##
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 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())).startsWith(orderStr) );
Review Comment:
I think I'll give option 2 a shot first then. It'll take me a couple of days
though (maybe next week) as I've got some other commitments .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service,
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on PR #605: URL: https://github.com/apache/cayenne/pull/605#issuecomment-1952256950 Thank you @Jugen for this huge effort! Will review everything for the final time and merge it soon. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1494406514
##
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 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())).startsWith(orderStr) );
Review Comment:
I'm ok with either 2 or 3, don't think that the first option is much better
that the existing solution. 3 is the most universal thing, but 2 should be a
bit easier, I think. So it's up to you. And again we could do it as a separate
task.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1493818116
##
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 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())).startsWith(orderStr) );
Review Comment:
> Ok, so it's a bit fragile but should hold for now.
I agree on it being fragile and have given it some thought. I think we have
3 options:
1. Change it to **_equals_** instead. This will be more robust at the
expense of duplicating only aliased columns.
2. I can try and do something with Node.visit to somehow compare the order
Node with the result column nodes.
3. Or add an abstract
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on PR #605: URL: https://github.com/apache/cayenne/pull/605#issuecomment-1949087363 @Jugen Yes, just to keep everything neat -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on PR #605: URL: https://github.com/apache/cayenne/pull/605#issuecomment-1948630563 So do I squash my side and then FORCE push ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on PR #605: URL: https://github.com/apache/cayenne/pull/605#issuecomment-1948620781 @Jugen I think we are finally ready to merge it to 5.0 branch. We could fine tune it after if needed. Could you please squash commits here? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1492635983
##
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 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())).startsWith(orderStr) );
Review Comment:
Ok, so it's a bit fragile but should hold for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1492620306
##
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 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())).startsWith(orderStr) );
Review Comment:
Actually I'm wrong wrt my last example because Cayenne wraps the expression
in brackets like so:
`SELECT DISTINCT ( column + 1 ) FROM table ORDER BY column` so we then get:
```
"( column + 1 ) ".startsWith("column ") -> false
```
and the resulting SQL will be: `SELECT DISTINCT ( column + 1 ), column FROM
table ORDER BY column`
--
This is an automated message from the Apache Git Service.
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1492609044
##
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 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())).startsWith(orderStr) );
Review Comment:
But reversing your example will fail for: `SELECT DISTINCT column + 1 FROM
table ORDER BY column` we would get:
```
"column + 1 ".startsWith("column ") -> true
```
and so produces the following SQL: `SELECT DISTINCT column + 1 FROM table
ORDER BY column`
which fails !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1492608373
##
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 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())).startsWith(orderStr) );
Review Comment:
Yes this case should be ok. But I suppose the opposite case will be
incorrect. I.e. `SELECT DISTINCT column + 1 FROM table ORDER BY column` will
result in `"column + 1 ".startsWith("column ") -> true`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail:
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1492595875
##
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 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())).startsWith(orderStr) );
Review Comment:
So for `SELECT DISTINCT column FROM table ORDER BY column + 1` we would get:
```
"column ".startsWith("column + 1 ") -> false
```
and so produces the following SQL: `SELECT DISTINCT column, column + 1 FROM
table ORDER BY column + 1`
which is correct isn't ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1492495670
##
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 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())).startsWith(orderStr) );
Review Comment:
Yeah, and `DbAttribute` filter should help too. Probably we could still get
wrong results with complex orderings. Like `SELECT column FROM table ORDER BY
column + 1`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1492478211
##
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 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())).startsWith(orderStr) );
Review Comment:
Note that in `getSqlString` a trailing space is appended in order to prevent
false positives like this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1492412472
##
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 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())).startsWith(orderStr) );
Review Comment:
I wonder if this check could return false positive result in case of some
similar column names, like `item` and `item_code`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please contact
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on PR #605: URL: https://github.com/apache/cayenne/pull/605#issuecomment-1947853798 @stariy95 I don't have any further changes in mind, so we can proceed with a final review when convenient -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on PR #605: URL: https://github.com/apache/cayenne/pull/605#issuecomment-1945847402 @stariy95 Have added the DbAttribute filter. (If I measured right then we're saving about 1ms per 100 columns) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on PR #605: URL: https://github.com/apache/cayenne/pull/605#issuecomment-1944189731 Yep, filtering by `DbAttribute` should make sense. It should filter out almost everything in most cases. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on PR #605: URL: https://github.com/apache/cayenne/pull/605#issuecomment-1944099714 I like how it's turned out as well, thanks so much for your guidance. I can filter using DbAttribute, before doing the SQL compare, to save a millisecond or two ([Graph](https://www.desmos.com/calculator/0hgx4gpmii)). What do you think ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on PR #605: URL: https://github.com/apache/cayenne/pull/605#issuecomment-1943371860 @Jugen Great! I like how it's turned out overall. The last think that seems off is performance of the duplicate check. But I don't have a better idea how to optimize that. If you got neither, I think we could do that as a separate task. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on PR #605: URL: https://github.com/apache/cayenne/pull/605#issuecomment-1943106918 @stariy95 have added OrderingGroupByStage as suggested. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1487416426
##
cayenne/src/main/java/org/apache/cayenne/access/translator/select/OrderingStage.java:
##
@@ -54,13 +60,15 @@ 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);
+// If query is DISTINCT or GROUPING then we need to add all missing
ORDER BY clauses as result columns
Review Comment:
It unfortunately can't go into OrderingDistinctStage because this needs to
happen **before** the GroupByStage.
I can move it into a new OrderingGroupByStage though .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1487338797
##
cayenne/src/main/java/org/apache/cayenne/access/translator/select/OrderingStage.java:
##
@@ -54,13 +60,15 @@ 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);
+// If query is DISTINCT or GROUPING then we need to add all missing
ORDER BY clauses as result columns
Review Comment:
Maybe we could move this logic to the new stage too? Then we'll have here
just a simple ordering processing, and a new stage will process all orderings
that need to be added to the result set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on PR #605: URL: https://github.com/apache/cayenne/pull/605#issuecomment-1940518358 @stariy95 Have now split OrderingStage into two as suggested. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1486308984
##
cayenne/src/main/java/org/apache/cayenne/access/translator/select/GroupByStage.java:
##
@@ -41,7 +41,7 @@ public void perform(TranslatorContext context) {
}
}
-private boolean hasAggregate(TranslatorContext context) {
+static boolean hasAggregate(TranslatorContext context) {
Review Comment:
Moved shared logic into StageUtil instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1486306763
##
cayenne/src/main/java/org/apache/cayenne/access/translator/select/DefaultSelectTranslator.java:
##
@@ -40,11 +40,11 @@ public class DefaultSelectTranslator implements
SelectTranslator {
private static final TranslationStage[] TRANSLATION_STAGES = {
new ColumnExtractorStage(),
new PrefetchNodeStage(),
-new OrderingStage(),
new QualifierTranslationStage(),
new HavingTranslationStage(),
-new GroupByStage(),
new DistinctStage(),
+new OrderingStage(),// Relies on DistinctStage
Review Comment:
I've restored the original Stage ordering in DefaultSelectTranslator and
instead moved all the shared logic code into a StageUtil class. This process
revealed that Orderings with compound paths now had to be explicitly dealt with
in OrderingStage :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1486041480
##
cayenne/src/main/java/org/apache/cayenne/access/translator/select/DefaultSelectTranslator.java:
##
@@ -40,11 +40,11 @@ public class DefaultSelectTranslator implements
SelectTranslator {
private static final TranslationStage[] TRANSLATION_STAGES = {
new ColumnExtractorStage(),
new PrefetchNodeStage(),
-new OrderingStage(),
new QualifierTranslationStage(),
new HavingTranslationStage(),
-new GroupByStage(),
new DistinctStage(),
+new OrderingStage(),// Relies on DistinctStage
Review Comment:
Translation stages are there just to split all the complext logic to
apprehensible chunks. So it should be totally fine to add more stages. This
logic is separated enough, we'll only need a way to read ordering nodes in a
later stage:
```java
if (isDistinctOrGroupByQuery) {
Optional column = getResultColumn(context,
order(nodeBuilder));
if (!column.isPresent()) {
// deepCopy 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);
}
}
}
```
##
cayenne/src/main/java/org/apache/cayenne/access/translator/select/GroupByStage.java:
##
@@ -41,7 +41,7 @@ public void perform(TranslatorContext context) {
}
}
-private boolean hasAggregate(TranslatorContext context) {
+static boolean hasAggregate(TranslatorContext context) {
Review Comment:
Yeah, we may add a flag to the context and keep the logic in the
`GroupByStage`, then it should be decent enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1486015595
##
cayenne/src/main/java/org/apache/cayenne/access/translator/select/DefaultSelectTranslator.java:
##
@@ -40,11 +40,11 @@ public class DefaultSelectTranslator implements
SelectTranslator {
private static final TranslationStage[] TRANSLATION_STAGES = {
new ColumnExtractorStage(),
new PrefetchNodeStage(),
-new OrderingStage(),
new QualifierTranslationStage(),
new HavingTranslationStage(),
-new GroupByStage(),
new DistinctStage(),
+new OrderingStage(),// Relies on DistinctStage
Review Comment:
Or combine this with `hasAggregate` from GroupByStage into a `StageUtil`
class ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1486012235
##
cayenne/src/main/java/org/apache/cayenne/access/translator/select/DefaultSelectTranslator.java:
##
@@ -40,11 +40,11 @@ public class DefaultSelectTranslator implements
SelectTranslator {
private static final TranslationStage[] TRANSLATION_STAGES = {
new ColumnExtractorStage(),
new PrefetchNodeStage(),
-new OrderingStage(),
new QualifierTranslationStage(),
new HavingTranslationStage(),
-new GroupByStage(),
new DistinctStage(),
+new OrderingStage(),// Relies on DistinctStage
Review Comment:
Actually it may be better moving it into a separate `DistinctUtil` class ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1485996375
##
cayenne/src/main/java/org/apache/cayenne/access/translator/select/DefaultSelectTranslator.java:
##
@@ -40,11 +40,11 @@ public class DefaultSelectTranslator implements
SelectTranslator {
private static final TranslationStage[] TRANSLATION_STAGES = {
new ColumnExtractorStage(),
new PrefetchNodeStage(),
-new OrderingStage(),
new QualifierTranslationStage(),
new HavingTranslationStage(),
-new GroupByStage(),
new DistinctStage(),
+new OrderingStage(),// Relies on DistinctStage
Review Comment:
Another possible solution is to move the bulk of the logic in
`DistinctStage.perform` to `TranslatorContext`, then I can revert this change.
What do you think ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1485966341
##
cayenne/src/main/java/org/apache/cayenne/access/translator/select/GroupByStage.java:
##
@@ -41,7 +41,7 @@ public void perform(TranslatorContext context) {
}
}
-private boolean hasAggregate(TranslatorContext context) {
+static boolean hasAggregate(TranslatorContext context) {
Review Comment:
A possibility is moving it to TranslatorContext, what do you think ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1485959529
##
cayenne/src/main/java/org/apache/cayenne/access/translator/select/OrderingStage.java:
##
@@ -71,4 +85,26 @@ private void processOrdering(QualifierTranslator
qualifierTranslator, Translator
context.getSelectBuilder().orderBy(orderingNodeBuilder);
}
+private Optional getResultColumn(TranslatorContext context,
OrderingNodeBuilder orderBuilder) {
+String orderStr = getSqlString(orderBuilder);
+
+return context.getResultNodeList().stream()
+.map(result -> result.getNode())
+// the column may have an alias so just check that the
orderStr part matches
+.filter(resultNode ->
getSqlString(node(resultNode)).startsWith(orderStr))
+.findFirst();
Review Comment:
I thought it's a bit heavy as well. Will need guidance later .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1485952787
##
cayenne/src/main/java/org/apache/cayenne/access/translator/select/DistinctStage.java:
##
@@ -70,7 +70,7 @@ public void perform(TranslatorContext context) {
context.getSelectBuilder().distinct();
}
-private boolean hasToManyJoin(TranslatorContext context) {
+static boolean hasToManyJoin(TranslatorContext context) {
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on PR #605: URL: https://github.com/apache/cayenne/pull/605#issuecomment-1938319464 @Jugen Thanks for keeping it up. I did my review, but it seems a bit early :) There are mostly minor things with the exception of the translation stages rearrangement. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on code in PR #605:
URL: https://github.com/apache/cayenne/pull/605#discussion_r1485816260
##
cayenne/src/main/java/org/apache/cayenne/access/translator/select/GroupByStage.java:
##
@@ -41,7 +41,7 @@ public void perform(TranslatorContext context) {
}
}
-private boolean hasAggregate(TranslatorContext context) {
+static boolean hasAggregate(TranslatorContext context) {
Review Comment:
This again breaks stage logic separation, but I don't see a good option here
as with the `hasToManyJoins()` method
##
cayenne/src/main/java/org/apache/cayenne/access/translator/select/OrderingStage.java:
##
@@ -71,4 +85,26 @@ private void processOrdering(QualifierTranslator
qualifierTranslator, Translator
context.getSelectBuilder().orderBy(orderingNodeBuilder);
}
+private Optional getResultColumn(TranslatorContext context,
OrderingNodeBuilder orderBuilder) {
+String orderStr = getSqlString(orderBuilder);
+
+return context.getResultNodeList().stream()
+.map(result -> result.getNode())
+// the column may have an alias so just check that the
orderStr part matches
+.filter(resultNode ->
getSqlString(node(resultNode)).startsWith(orderStr))
+.findFirst();
Review Comment:
This seems a bit heavy, but we could worry about performance later.
##
cayenne/src/main/java/org/apache/cayenne/access/translator/select/DistinctStage.java:
##
@@ -70,7 +70,7 @@ public void perform(TranslatorContext context) {
context.getSelectBuilder().distinct();
}
-private boolean hasToManyJoin(TranslatorContext context) {
+static boolean hasToManyJoin(TranslatorContext context) {
Review Comment:
We may move this logic to the `TableTree` class to keep stages as separeted
as possible.
##
cayenne/src/main/java/org/apache/cayenne/access/translator/select/DefaultSelectTranslator.java:
##
@@ -40,11 +40,11 @@ public class DefaultSelectTranslator implements
SelectTranslator {
private static final TranslationStage[] TRANSLATION_STAGES = {
new ColumnExtractorStage(),
new PrefetchNodeStage(),
-new OrderingStage(),
new QualifierTranslationStage(),
new HavingTranslationStage(),
-new GroupByStage(),
new DistinctStage(),
+new OrderingStage(),// Relies on DistinctStage
Review Comment:
This change is not safe, logic in the `DistinctStage` relies on joins
defined, while ordering expression translation could add mode joins. Probably
we should try to split `OrderingStage` into two parts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
Jugen commented on PR #605: URL: https://github.com/apache/cayenne/pull/605#issuecomment-1935535384 Am looking into it, it's just trickier than I thought :-) Have solved most so far, just one more to test to resolve. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] CAY-2842 Prevent duplicate select columns when using distinct with order by [cayenne]
stariy95 commented on PR #605: URL: https://github.com/apache/cayenne/pull/605#issuecomment-1935525475 Hi @Jugen Thanks for another PR! It seems there are some problems with the tests, please see https://github.com/apache/cayenne/actions/runs/7830706412/job/21367291178?pr=605 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cayenne.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
