Disallow ORDER BY literal in window functions

presto-docs/src/main/sphinx/functions/window.rst

@@ -26,6 +26,8 @@ For example, the following query ranks orders for each clerk by price::
     FROM orders
     ORDER BY clerk, rnk
 
+**Note that ORDER BY clause within window functions does not support ordinals. You need to use the actual expressions**
+
 Aggregate Functions
 -------------------
 

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

@@ -171,6 +171,7 @@ public final class SystemSessionProperties
     public static final String REMOTE_FUNCTIONS_ENABLED = "remote_functions_enabled";
     public static final String CHECK_ACCESS_CONTROL_ON_UTILIZED_COLUMNS_ONLY = "check_access_control_on_utilized_columns_only";
     public static final String SKIP_REDUNDANT_SORT = "skip_redundant_sort";
+    public static final String ALLOW_WINDOW_ORDER_BY_LITERALS = "allow_window_order_by_literals";
 
     private final List<PropertyMetadata<?>> sessionProperties;
 
@@ -890,6 +891,11 @@ public SystemSessionProperties(
                         CHECK_ACCESS_CONTROL_ON_UTILIZED_COLUMNS_ONLY,
                         "Apply access control rules on only those columns that are required to produce the query output",
                         featuresConfig.isCheckAccessControlOnUtilizedColumnsOnly(),
+                        false),
+                booleanProperty(
+                        ALLOW_WINDOW_ORDER_BY_LITERALS,
+                        "Allow ORDER BY literals in window functions",
+                        featuresConfig.isAllowWindowOrderByLiterals(),
                         false));
     }
 
@@ -898,6 +904,11 @@ public static boolean isSkipRedundantSort(Session session)
         return session.getSystemProperty(SKIP_REDUNDANT_SORT, Boolean.class);
     }
 
+    public static boolean isAllowWindowOrderByLiterals(Session session)
+    {
+        return session.getSystemProperty(ALLOW_WINDOW_ORDER_BY_LITERALS, Boolean.class);
+    }
+
     public List<PropertyMetadata<?>> getSessionProperties()
     {
         return sessionProperties;

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

@@ -172,6 +172,7 @@ public class FeaturesConfig
     private boolean inlineSqlFunctions = true;
     private boolean checkAccessControlOnUtilizedColumnsOnly;
     private boolean skipRedundantSort = true;
+    private boolean isAllowWindowOrderByLiterals = true;
 
     private String warnOnNoTableLayoutFilter = "";
 
@@ -1477,4 +1478,16 @@ public FeaturesConfig setSkipRedundantSort(boolean value)
         this.skipRedundantSort = value;
         return this;
     }
+
+    public boolean isAllowWindowOrderByLiterals()
+    {
+        return isAllowWindowOrderByLiterals;
+    }
+
+    @Config("is-allow-window-order-by-literals")
+    public FeaturesConfig setAllowWindowOrderByLiterals(boolean value)
+    {
+        this.isAllowWindowOrderByLiterals = value;
+        return this;
+    }
 }

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

@@ -18,6 +18,7 @@ public enum SemanticErrorCode
     MUST_BE_AGGREGATE_OR_GROUP_BY,
     NESTED_AGGREGATION,
     NESTED_WINDOW,
+    WINDOW_FUNCTION_ORDERBY_LITERAL,
     MUST_BE_WINDOW_FUNCTION,
     MUST_BE_AGGREGATION_FUNCTION,
     WINDOW_REQUIRES_OVER,

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

@@ -91,6 +91,7 @@
 import com.facebook.presto.sql.tree.JoinOn;
 import com.facebook.presto.sql.tree.JoinUsing;
 import com.facebook.presto.sql.tree.Lateral;
+import com.facebook.presto.sql.tree.Literal;
 import com.facebook.presto.sql.tree.LongLiteral;
 import com.facebook.presto.sql.tree.NaturalJoin;
 import com.facebook.presto.sql.tree.Node;
@@ -155,6 +156,7 @@
 import java.util.stream.Collectors;
 
 import static com.facebook.presto.SystemSessionProperties.getMaxGroupingSets;
+import static com.facebook.presto.SystemSessionProperties.isAllowWindowOrderByLiterals;
 import static com.facebook.presto.common.type.BigintType.BIGINT;
 import static com.facebook.presto.common.type.BooleanType.BOOLEAN;
 import static com.facebook.presto.common.type.TypeSignature.parseTypeSignature;
@@ -211,6 +213,7 @@
 import static com.facebook.presto.sql.analyzer.SemanticErrorCode.VIEW_IS_STALE;
 import static com.facebook.presto.sql.analyzer.SemanticErrorCode.VIEW_PARSE_ERROR;
 import static com.facebook.presto.sql.analyzer.SemanticErrorCode.WILDCARD_WITHOUT_FROM;
+import static com.facebook.presto.sql.analyzer.SemanticErrorCode.WINDOW_FUNCTION_ORDERBY_LITERAL;
 import static com.facebook.presto.sql.analyzer.TypeSignatureProvider.fromTypes;
 import static com.facebook.presto.sql.planner.ExpressionDeterminismEvaluator.isDeterministic;
 import static com.facebook.presto.sql.planner.ExpressionInterpreter.expressionOptimizer;
@@ -1534,6 +1537,27 @@ private List<FunctionCall> analyzeWindowFunctions(QuerySpecification node, List<
                 }
 
                 Window window = windowFunction.getWindow().get();
+                if (window.getOrderBy().filter(
+                        orderBy -> orderBy.getSortItems()
+                                .stream()
+                                .anyMatch(item -> item.getSortKey() instanceof Literal))
+                        .isPresent()) {
+                    if (isAllowWindowOrderByLiterals(session)) {
+                        warningCollector.add(
+                                new PrestoWarning(
+                                        PERFORMANCE_WARNING,
+                                        String.format(
+                                                "ORDER BY literals/constants with window function: '%s' is unnecessary and expensive. If you intend to ORDER BY using ordinals, please use the actual expression instead of the ordinal",
+                                                windowFunction)));
+                    }
+                    else {
+                        throw new SemanticException(
+                                WINDOW_FUNCTION_ORDERBY_LITERAL,
+                                node,
+                                "ORDER BY literals/constants with window function: '%s' is unnecessary and expensive. If you intend to ORDER BY using ordinals, please use the actual expression instead of the ordinal",
+                                windowFunction);
+                    }
+                }
 
                 ImmutableList.Builder<Node> toExtract = ImmutableList.builder();
                 toExtract.addAll(windowFunction.getArguments());

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

@@ -22,6 +22,7 @@
 import com.facebook.presto.memory.NodeMemoryConfig;
 import com.facebook.presto.metadata.SessionPropertyManager;
 import com.facebook.presto.spi.PrestoWarning;
+import com.facebook.presto.spi.StandardWarningCode;
 import com.facebook.presto.spi.WarningCollector;
 import org.testng.annotations.Test;
 
@@ -71,6 +72,7 @@
 import static com.facebook.presto.sql.analyzer.SemanticErrorCode.VIEW_IS_RECURSIVE;
 import static com.facebook.presto.sql.analyzer.SemanticErrorCode.VIEW_IS_STALE;
 import static com.facebook.presto.sql.analyzer.SemanticErrorCode.WILDCARD_WITHOUT_FROM;
+import static com.facebook.presto.sql.analyzer.SemanticErrorCode.WINDOW_FUNCTION_ORDERBY_LITERAL;
 import static com.facebook.presto.sql.analyzer.SemanticErrorCode.WINDOW_REQUIRES_OVER;
 import static com.facebook.presto.testing.TestingSession.testSessionBuilder;
 import static java.lang.String.format;
@@ -81,6 +83,15 @@
 public class TestAnalyzer
         extends AbstractAnalyzerTest
 {
+    private static void assertHasWarning(WarningCollector warningCollector, StandardWarningCode code, String match)
+    {
+        List<PrestoWarning> warnings = warningCollector.getWarnings();
+        assertEquals(warnings.size(), 1);
+        PrestoWarning warning = warnings.get(0);
+        assertEquals(warning.getWarningCode(), code.toWarningCode());
+        assertTrue(warning.getMessage().startsWith(match));
+    }
+
     @Test
     public void testNonComparableGroupBy()
     {
@@ -99,6 +110,34 @@ public void testNonComparableWindowOrder()
         assertFails(TYPE_MISMATCH, "SELECT row_number() OVER (ORDER BY t.x) FROM (VALUES(color('red'))) AS t(x)");
     }
 
+    @Test
+    public void testWindowOrderByAnalysis()
+    {
+        assertHasWarning(analyzeWithWarnings("SELECT SUM(x) OVER (PARTITION BY y ORDER BY 1) AS s\n" +
+                "FROM (values (1,10), (2, 10)) AS T(x, y)"), PERFORMANCE_WARNING, "ORDER BY literals/constants with window function:");
+
+        assertHasWarning(analyzeWithWarnings("SELECT SUM(x) OVER (ORDER BY 1) AS s\n" +
+                "FROM (values (1,10), (2, 10)) AS T(x, y)"), PERFORMANCE_WARNING, "ORDER BY literals/constants with window function:");
+
+        // Now test for error when the session param is set to disallow this.
+        Session session = testSessionBuilder(new SessionPropertyManager(new SystemSessionProperties(
+                new QueryManagerConfig(),
+                new TaskManagerConfig(),
+                new MemoryManagerConfig(),
+                new FeaturesConfig().setAllowWindowOrderByLiterals(false),
+                new NodeMemoryConfig(),
+                new WarningCollectorConfig()))).build();
+        assertFails(session, WINDOW_FUNCTION_ORDERBY_LITERAL,
+                "SELECT SUM(x) OVER (PARTITION BY y ORDER BY 1) AS s\n" +
+                        "FROM (values (1,10), (2, 10)) AS T(x, y)");
+        assertFails(session, WINDOW_FUNCTION_ORDERBY_LITERAL,
+                "SELECT SUM(x) OVER (ORDER BY 1) AS s\n" +
+                        "FROM (values (1,10), (2, 10)) AS T(x, y)");
+
+        analyze(session, "SELECT SUM(x) OVER (PARTITION BY y ORDER BY y) AS s\n" +
+                        "FROM (values (1,10), (2, 10)) AS T(x, y)");
+    }
+
     @Test
     public void testNonComparableDistinctAggregation()
     {

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

@@ -151,7 +151,8 @@ public void testDefaults()
                 .setSkipRedundantSort(true)
                 .setWarnOnNoTableLayoutFilter("")
                 .setInlineSqlFunctions(true)
-                .setCheckAccessControlOnUtilizedColumnsOnly(false));
+                .setCheckAccessControlOnUtilizedColumnsOnly(false)
+                .setAllowWindowOrderByLiterals(true));
     }
 
     @Test
@@ -256,6 +257,7 @@ public void testExplicitPropertyMappings()
                 .put("inline-sql-functions", "false")
                 .put("check-access-control-on-utilized-columns-only", "true")
                 .put("optimizer.skip-redundant-sort", "false")
+                .put("is-allow-window-order-by-literals", "false")
                 .build();
 
         FeaturesConfig expected = new FeaturesConfig()
@@ -357,7 +359,8 @@ public void testExplicitPropertyMappings()
                 .setWarnOnNoTableLayoutFilter("ry@nlikestheyankees,ds")
                 .setInlineSqlFunctions(false)
                 .setCheckAccessControlOnUtilizedColumnsOnly(true)
-                .setSkipRedundantSort(false);
+                .setSkipRedundantSort(false)
+                .setAllowWindowOrderByLiterals(false);
         assertFullMapping(properties, expected);
     }