Add config option and session property to restore legacy ORDER BY behavior

presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java

@@ -64,6 +64,7 @@ public final class SystemSessionProperties
     public static final String SPILL_ENABLED = "spill_enabled";
     public static final String OPERATOR_MEMORY_LIMIT_BEFORE_SPILL = "operator_memory_limit_before_spill";
     public static final String OPTIMIZE_DISTINCT_AGGREGATIONS = "optimize_mixed_distinct_aggregations";
+    public static final String LEGACY_ORDER_BY = "legacy_order_by";
 
     private final List<PropertyMetadata<?>> sessionProperties;
 
@@ -253,7 +254,12 @@ public SystemSessionProperties(
                         OPTIMIZE_DISTINCT_AGGREGATIONS,
                         "Optimize mixed non-distinct and distinct aggregations",
                         featuresConfig.isOptimizeMixedDistinctAggregations(),
-                        false));
+                        false),
+                booleanSessionProperty(
+                        LEGACY_ORDER_BY,
+                        "Use legacy rules for column resolution in ORDER BY clause",
+                        false,
+                        featuresConfig.isLegacyOrderBy()));
     }
 
     public List<PropertyMetadata<?>> getSessionProperties()
@@ -394,4 +400,9 @@ public static boolean isOptimizeDistinctAggregationEnabled(Session session)
     {
         return session.getSystemProperty(OPTIMIZE_DISTINCT_AGGREGATIONS, Boolean.class);
     }
+
+    public static boolean isLegacyOrderByEnabled(Session session)
+    {
+        return session.getSystemProperty(LEGACY_ORDER_BY, Boolean.class);
+    }
 }

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java

@@ -52,6 +52,7 @@ public static class ProcessingOptimization
     private boolean optimizeSingleDistinct = true;
     private boolean pushTableWriteThroughUnion = true;
     private boolean legacyArrayAgg;
+    private boolean legacyOrderBy;
     private boolean optimizeMixedDistinctAggregations;
 
     private String processingOptimization = ProcessingOptimization.DISABLED;
@@ -107,6 +108,18 @@ public boolean isLegacyArrayAgg()
         return legacyArrayAgg;
     }
 
+    @Config("deprecated.legacy-order-by")
+    public FeaturesConfig setLegacyOrderBy(boolean value)
+    {
+        this.legacyOrderBy = value;
+        return this;
+    }
+
+    public boolean isLegacyOrderBy()
+    {
+        return legacyOrderBy;
+    }
+
     @Config("distributed-joins-enabled")
     public FeaturesConfig setDistributedJoinsEnabled(boolean distributedJoinsEnabled)
     {

presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

@@ -14,6 +14,7 @@
 package com.facebook.presto.sql.analyzer;
 
 import com.facebook.presto.Session;
+import com.facebook.presto.SystemSessionProperties;
 import com.facebook.presto.metadata.FunctionKind;
 import com.facebook.presto.metadata.Metadata;
 import com.facebook.presto.metadata.QualifiedObjectName;
@@ -129,6 +130,7 @@
 import java.util.Set;
 import java.util.stream.Collectors;
 
+import static com.facebook.presto.SystemSessionProperties.LEGACY_ORDER_BY;
 import static com.facebook.presto.metadata.FunctionKind.AGGREGATE;
 import static com.facebook.presto.metadata.FunctionKind.WINDOW;
 import static com.facebook.presto.metadata.MetadataUtil.createQualifiedObjectName;
@@ -1225,6 +1227,10 @@ private void analyzeHaving(QuerySpecification node, Scope scope)
 
     private List<Expression> analyzeOrderBy(QuerySpecification node, Scope sourceScope, Scope outputScope, List<Expression> outputExpressions)
     {
+        if (SystemSessionProperties.isLegacyOrderByEnabled(session)) {
+            return legacyAnalyzeOrderBy(node, sourceScope, outputScope, outputExpressions);
+        }
+
         List<SortItem> items = node.getOrderBy();
 
         ImmutableList.Builder<Expression> orderByExpressionsBuilder = ImmutableList.builder();
@@ -1275,6 +1281,90 @@ private List<Expression> analyzeOrderBy(QuerySpecification node, Scope sourceSco
         return orderByExpressions;
     }
 
+    /**
+     * Preserve the old column resolution behavior for ORDER BY while we transition workloads to new semantics
+     * TODO: remove this
+     */
+    private List<Expression> legacyAnalyzeOrderBy(QuerySpecification node, Scope sourceScope, Scope outputScope, List<Expression> outputExpressions)
+    {
+        List<SortItem> items = node.getOrderBy();
+
+        ImmutableList.Builder<Expression> orderByExpressionsBuilder = ImmutableList.builder();
+
+        if (!items.isEmpty()) {
+            // Compute aliased output terms so we can resolve order by expressions against them first
+            ImmutableMultimap.Builder<QualifiedName, Expression> byAliasBuilder = ImmutableMultimap.builder();
+            for (SelectItem item : node.getSelect().getSelectItems()) {
+                if (item instanceof SingleColumn) {
+                    Optional<String> alias = ((SingleColumn) item).getAlias();
+                    if (alias.isPresent()) {
+                        byAliasBuilder.put(QualifiedName.of(alias.get()), ((SingleColumn) item).getExpression()); // TODO: need to know if alias was quoted
+                    }
+                }
+            }
+            Multimap<QualifiedName, Expression> byAlias = byAliasBuilder.build();
+
+            for (SortItem item : items) {
+                Expression expression = item.getSortKey();
+
+                Expression orderByExpression = null;
+                if (expression instanceof QualifiedNameReference && !((QualifiedNameReference) expression).getName().getPrefix().isPresent()) {
+                    // if this is a simple name reference, try to resolve against output columns
+
+                    QualifiedName name = ((QualifiedNameReference) expression).getName();
+                    Collection<Expression> expressions = byAlias.get(name);
+                    if (expressions.size() > 1) {
+                        throw new SemanticException(AMBIGUOUS_ATTRIBUTE, expression, "'%s' in ORDER BY is ambiguous", name.getSuffix());
+                    }
+                    if (expressions.size() == 1) {
+                        orderByExpression = Iterables.getOnlyElement(expressions);
+                    }
+
+                    // otherwise, couldn't resolve name against output aliases, so fall through...
+                }
+                else if (expression instanceof LongLiteral) {
+                    // this is an ordinal in the output tuple
+
+                    long ordinal = ((LongLiteral) expression).getValue();
+                    if (ordinal < 1 || ordinal > outputExpressions.size()) {
+                        throw new SemanticException(INVALID_ORDINAL, expression, "ORDER BY position %s is not in select list", ordinal);
+                    }
+
+                    int field = Ints.checkedCast(ordinal - 1);
+                    Type type = outputScope.getRelationType().getFieldByIndex(field).getType();
+                    if (!type.isOrderable()) {
+                        throw new SemanticException(TYPE_MISMATCH, node, "The type of expression in position %s is not orderable (actual: %s), and therefore cannot be used in ORDER BY", ordinal, type);
+                    }
+
+                    orderByExpression = outputExpressions.get(field);
+                }
+
+                // otherwise, just use the expression as is
+                if (orderByExpression == null) {
+                    orderByExpression = expression;
+                }
+
+                ExpressionAnalysis expressionAnalysis = analyzeExpression(orderByExpression, sourceScope);
+                analysis.recordSubqueries(node, expressionAnalysis);
+
+                Type type = expressionAnalysis.getType(orderByExpression);
+                if (!type.isOrderable()) {
+                    throw new SemanticException(TYPE_MISMATCH, node, "Type %s is not orderable, and therefore cannot be used in ORDER BY: %s", type, expression);
+                }
+
+                orderByExpressionsBuilder.add(orderByExpression);
+            }
+        }
+
+        List<Expression> orderByExpressions = orderByExpressionsBuilder.build();
+        analysis.setOrderByExpressions(node, orderByExpressions);
+
+        if (node.getSelect().isDistinct() && !outputExpressions.containsAll(orderByExpressions)) {
+            throw new SemanticException(ORDER_BY_MUST_BE_IN_SELECT, node.getSelect(), "For SELECT DISTINCT, ORDER BY expressions must appear in select list");
+        }
+        return orderByExpressions;
+    }
+
     private static Multimap<QualifiedName, Expression> extractNamedOutputExpressions(QuerySpecification node)
     {
         // Compute aliased output terms so we can resolve order by expressions against them first
@@ -1645,6 +1735,7 @@ private RelationType analyzeView(Query query, QualifiedObjectName name, Optional
                     .setRemoteUserAddress(session.getRemoteUserAddress().orElse(null))
                     .setUserAgent(session.getUserAgent().orElse(null))
                     .setStartTime(session.getStartTime())
+                    .setSystemProperty(LEGACY_ORDER_BY, session.getSystemProperty(LEGACY_ORDER_BY, Boolean.class).toString())
                     .build();
 
             StatementAnalyzer analyzer = new StatementAnalyzer(analysis, metadata, sqlParser, viewAccessControl, viewSession);

presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestFeaturesConfig.java

@@ -54,7 +54,8 @@ public void testDefaults()
                 .setOperatorMemoryLimitBeforeSpill(DataSize.valueOf("4MB"))
                 .setSpillerSpillPath(Paths.get(System.getProperty("java.io.tmpdir"), "presto", "spills").toString())
                 .setSpillerThreads(4)
-                .setOptimizeMixedDistinctAggregations(false));
+                .setOptimizeMixedDistinctAggregations(false)
+                .setLegacyOrderBy(false));
     }
 
     @Test
@@ -63,6 +64,7 @@ public void testExplicitPropertyMappings()
         Map<String, String> propertiesLegacy = new ImmutableMap.Builder<String, String>()
                 .put("experimental.resource-groups-enabled", "true")
                 .put("deprecated.legacy-array-agg", "true")
+                .put("deprecated.legacy-order-by", "true")
                 .put("distributed-index-joins-enabled", "true")
                 .put("distributed-joins-enabled", "false")
                 .put("colocated-joins-enabled", "true")
@@ -85,6 +87,7 @@ public void testExplicitPropertyMappings()
         Map<String, String> properties = new ImmutableMap.Builder<String, String>()
                 .put("experimental.resource-groups-enabled", "true")
                 .put("deprecated.legacy-array-agg", "true")
+                .put("deprecated.legacy-order-by", "true")
                 .put("distributed-index-joins-enabled", "true")
                 .put("distributed-joins-enabled", "false")
                 .put("colocated-joins-enabled", "true")
@@ -125,7 +128,8 @@ public void testExplicitPropertyMappings()
                 .setSpillEnabled(true)
                 .setOperatorMemoryLimitBeforeSpill(DataSize.valueOf("100MB"))
                 .setSpillerSpillPath("/tmp/custom/spill/path")
-                .setSpillerThreads(42);
+                .setSpillerThreads(42)
+                .setLegacyOrderBy(true);
 
         assertFullMapping(properties, expected);
         assertDeprecatedEquivalence(FeaturesConfig.class, properties, propertiesLegacy);