diff --git a/presto-docs/src/main/sphinx/functions/window.rst b/presto-docs/src/main/sphinx/functions/window.rst index 76ef29d48b62d..d3680fed1821c 100644 --- a/presto-docs/src/main/sphinx/functions/window.rst +++ b/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 ------------------- diff --git a/presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java b/presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java index ea73a92df1270..aff958ad99d08 100644 --- a/presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java +++ b/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> 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> getSessionProperties() { return sessionProperties; diff --git a/presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java b/presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java index 545b400a14276..946a54bdfb28a 100644 --- a/presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java +++ b/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; + } } diff --git a/presto-main/src/main/java/com/facebook/presto/sql/analyzer/SemanticErrorCode.java b/presto-main/src/main/java/com/facebook/presto/sql/analyzer/SemanticErrorCode.java index 012f766445ec2..64b90694df629 100644 --- a/presto-main/src/main/java/com/facebook/presto/sql/analyzer/SemanticErrorCode.java +++ b/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, diff --git a/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java b/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java index f697e3c759eb5..6b8de35c6071f 100644 --- a/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java +++ b/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 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 toExtract = ImmutableList.builder(); toExtract.addAll(windowFunction.getArguments()); diff --git a/presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestAnalyzer.java b/presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestAnalyzer.java index 0d89d31cb9b4a..991b5e35fde9d 100644 --- a/presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestAnalyzer.java +++ b/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 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() { diff --git a/presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestFeaturesConfig.java b/presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestFeaturesConfig.java index 323a78941ed67..daaf23b2cb8ad 100644 --- a/presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestFeaturesConfig.java +++ b/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); }