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<Node> 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