diff --git a/ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java b/ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java index 93285e4d3700..9394e65cf6ab 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java @@ -684,6 +684,10 @@ Operator genOPTree(ASTNode ast, PlannerContext plannerCtx) throws SemanticExcept msg = "Plan not optimized by CBO."; } this.ctx.setCboInfo(msg); + // hive.cbo.enable is still true, so analyzeInternal skipped ORDER BY ordinal + // substitution. Resolve them before the legacy planner compiles the ordinal + // as a constant (and then drops the sort entirely). See HIVE-30037. + processPositionAlias(ast, false, true); sinkOp = super.genOPTree(ast, plannerCtx); } } diff --git a/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java b/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java index cadb097c4d4d..d2d10ebdad45 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java @@ -14239,6 +14239,15 @@ private void validateCreateView() // Process the position alias in GROUPBY and ORDERBY void processPositionAlias(ASTNode ast) throws SemanticException { + // When CBO is enabled, ORDER BY ordinals are resolved in CalcitePlanner.genSortByKey. + // If CBO later declines the statement, CalcitePlanner.genOPTree calls this again with + // only ORDER BY enabled; GROUP BY expressions have already been resolved and + // must not be interpreted as ordinals a second time (HIVE-30037). + processPositionAlias(ast, true, !HiveConf.getBoolVar(conf, HiveConf.ConfVars.HIVE_CBO_ENABLED)); + } + + void processPositionAlias(ASTNode ast, boolean processGroupByPositionAlias, + boolean processOrderByPositionAlias) throws SemanticException { boolean isBothByPos = HiveConf.getBoolVar(conf, ConfVars.HIVE_GROUPBY_ORDERBY_POSITION_ALIAS); boolean isGbyByPos = isBothByPos || HiveConf.getBoolVar(conf, ConfVars.HIVE_GROUPBY_POSITION_ALIAS); @@ -14278,7 +14287,7 @@ void processPositionAlias(ASTNode ast) throws SemanticException { int selectExpCnt = selectNode.getChildCount(); // replace each of the position alias in GROUPBY with the actual column name - if (groupbyNode != null) { + if (processGroupByPositionAlias && groupbyNode != null) { for (int child_pos = 0; child_pos < groupbyNode.getChildCount(); ++child_pos) { ASTNode node = (ASTNode) groupbyNode.getChild(child_pos); if (node.getToken().getType() == HiveParser.Number) { @@ -14301,10 +14310,9 @@ void processPositionAlias(ASTNode ast) throws SemanticException { } } - // replace each of the position alias in ORDERBY with the actual column name, - // if cbo is enabled, orderby position will be processed in genPlan - if (!HiveConf.getBoolVar(conf, HiveConf.ConfVars.HIVE_CBO_ENABLED) - && orderbyNode != null) { + // replace each of the position alias in ORDERBY with the actual column name. + // When CBO actually plans the statement, ordinals are resolved in genSortByKey. + if (processOrderByPositionAlias && orderbyNode != null) { isAllCol = false; for (int child_pos = 0; child_pos < selectNode.getChildCount(); ++child_pos) { ASTNode node = (ASTNode) selectNode.getChild(child_pos).getChild(0); diff --git a/ql/src/test/org/apache/hadoop/hive/ql/parse/TestOrderByOrdinalCboDeclineRows.java b/ql/src/test/org/apache/hadoop/hive/ql/parse/TestOrderByOrdinalCboDeclineRows.java new file mode 100644 index 000000000000..8ba00de5ac90 --- /dev/null +++ b/ql/src/test/org/apache/hadoop/hive/ql/parse/TestOrderByOrdinalCboDeclineRows.java @@ -0,0 +1,116 @@ +/* + * 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 + * + * http://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.hadoop.hive.ql.parse; + +import static org.junit.Assert.assertEquals; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +import org.apache.hadoop.hive.conf.HiveConf; +import org.apache.hadoop.hive.ql.DriverFactory; +import org.apache.hadoop.hive.ql.IDriver; +import org.apache.hadoop.hive.ql.session.SessionState; +import org.apache.hive.testutils.HiveTestEnvSetup; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TestRule; + +/** + * HIVE-30037: executed row order when CBO declines the statement. + * + * {@link TestSemanticAnalyzer} already pins AST substitution on this path. + * This class runs the query (Tez mini cluster via {@link HiveTestEnvSetup}) + * and asserts the result is 3, 2, 1 rather than the pre-fix unsorted order. + */ +public class TestOrderByOrdinalCboDeclineRows { + + private static final String TABLE = "hive30037_ob_t"; + private static final String QUERY = + "select d from " + TABLE + " tablesample (5 rows) s order by 1 desc"; + + @ClassRule + public static HiveTestEnvSetup envSetup = new HiveTestEnvSetup(); + + @Rule + public TestRule methodRule = envSetup.getMethodRule(); + + @BeforeClass + public static void beforeClass() throws Exception { + IDriver driver = createDriver(); + dropTables(driver); + driver.run("create table " + TABLE + " (d int)"); + driver.run("insert into " + TABLE + " values (1), (2), (3)"); + } + + @AfterClass + public static void afterClass() throws Exception { + IDriver driver = createDriver(); + dropTables(driver); + } + + public static void dropTables(IDriver driver) throws Exception { + driver.run("drop table if exists " + TABLE); + } + + @Test + public void testTablesampleOrderByOrdinalDescReturns321() throws Exception { + IDriver driver = createDriver(); + driver.run(QUERY); + List rows = fetchRows(driver); + assertEquals("TABLESAMPLE + ORDER BY 1 DESC should return 3,2,1; fetchTask=" + + driver.getFetchTask(), Arrays.asList("3", "2", "1"), rows); + } + + @SuppressWarnings({ "rawtypes", "unchecked" }) + private static List fetchRows(IDriver driver) throws Exception { + driver.setMaxRows(100); + List rows = new ArrayList(); + if (driver.getFetchTask() != null) { + driver.getFetchTask().setMaxRows(100); + driver.getFetchTask().fetch(rows); + } else { + List batch = new ArrayList(); + while (driver.getResults(batch)) { + rows.addAll(batch); + batch.clear(); + } + } + List out = new ArrayList<>(); + for (Object row : rows) { + out.add(String.valueOf(row)); + } + return out; + } + + private static IDriver createDriver() { + HiveConf conf = envSetup.getTestCtx().hiveConf; + conf.setVar(HiveConf.ConfVars.HIVE_AUTHORIZATION_MANAGER, + "org.apache.hadoop.hive.ql.security.authorization.plugin.sqlstd.SQLStdHiveAuthorizerFactory"); + HiveConf.setBoolVar(conf, HiveConf.ConfVars.HIVE_SUPPORT_CONCURRENCY, false); + HiveConf.setBoolVar(conf, HiveConf.ConfVars.HIVE_CBO_ENABLED, true); + HiveConf.setVar(conf, HiveConf.ConfVars.HIVE_FETCH_TASK_CONVERSION, "none"); + SessionState.start(conf); + return DriverFactory.newDriver(conf); + } +} diff --git a/ql/src/test/org/apache/hadoop/hive/ql/parse/TestSemanticAnalyzer.java b/ql/src/test/org/apache/hadoop/hive/ql/parse/TestSemanticAnalyzer.java index 59a8c105a5fa..8a5bb520ebf8 100644 --- a/ql/src/test/org/apache/hadoop/hive/ql/parse/TestSemanticAnalyzer.java +++ b/ql/src/test/org/apache/hadoop/hive/ql/parse/TestSemanticAnalyzer.java @@ -677,6 +677,134 @@ public void testCboAmbiguityChecksAreReturnPathInvariant() throws Exception { } } + // ==== HIVE-30037: ORDER BY ordinals when CBO declines the statement ==== + + @Test + public void testOrderByOrdinalResolvedWhenCboDeclinesTablesample() throws Exception { + assertOrderByOrdinalResolvedOnCboDecline( + "select key from table1 tablesample (2 rows) order by 1 desc"); + } + + @Test + public void testOrderByOrdinalResolvedWhenCboDeclinesSortByLimitSubquery() throws Exception { + assertOrderByOrdinalResolvedOnCboDecline( + "select key from (select key from table1 sort by key limit 5) s order by 1 desc"); + } + + @Test + public void testOrderByOrdinalStillSortedWhenCboHandlesStatement() throws Exception { + AnalyzedQuery analyzed = analyzeQueryWithCbo( + "select key from table1 order by 1 desc"); + assertTrue(analyzed.analyzer.getCboInfo(), + analyzed.analyzer.getCboInfo() != null + && analyzed.analyzer.getCboInfo().contains("Plan optimized by CBO")); + } + + @Test + public void testCboDeclinePreservesOutOfRangeGroupByConstant() throws Exception { + assertGroupByConstantSurvivesCboDecline("100"); + } + + @Test + public void testCboDeclinePreservesInRangeGroupByConstant() throws Exception { + assertGroupByConstantSurvivesCboDecline("2"); + } + + private void assertGroupByConstantSurvivesCboDecline(String constant) throws Exception { + boolean original = conf.getBoolVar(HiveConf.ConfVars.HIVE_GROUPBY_POSITION_ALIAS); + conf.setBoolVar(HiveConf.ConfVars.HIVE_GROUPBY_POSITION_ALIAS, true); + try { + AnalyzedQuery analyzed = analyzeQueryWithCbo("select " + constant + + " as k, count(*) as n from table1 tablesample (2 rows) group by 1 order by 1"); + assertTrue(analyzed.analyzer.getCboInfo(), + analyzed.analyzer.getCboInfo() != null + && analyzed.analyzer.getCboInfo().contains("not optimized by CBO")); + ASTNode groupBy = findFirstNodeOfType(analyzed.ast, HiveParser.TOK_GROUPBY); + assertNotNull(groupBy); + assertEquals(HiveParser.Number, ((ASTNode) groupBy.getChild(0)).getType()); + assertEquals(constant, groupBy.getChild(0).getText()); + ASTNode orderByRef = findFirstOrderByRef(analyzed.ast); + assertNotNull(orderByRef); + assertEquals(HiveParser.Number, orderByRef.getType()); + assertEquals(constant, orderByRef.getText()); + } finally { + conf.setBoolVar(HiveConf.ConfVars.HIVE_GROUPBY_POSITION_ALIAS, original); + } + } + + private static ASTNode findFirstNodeOfType(ASTNode node, int type) { + if (node.getType() == type) { + return node; + } + for (int i = 0; i < node.getChildCount(); i++) { + ASTNode found = findFirstNodeOfType((ASTNode) node.getChild(i), type); + if (found != null) { + return found; + } + } + return null; + } + + private void assertOrderByOrdinalResolvedOnCboDecline(String query) throws Exception { + AnalyzedQuery analyzed = analyzeQueryWithCbo(query); + assertTrue(analyzed.analyzer.getCboInfo(), + analyzed.analyzer.getCboInfo() != null + && analyzed.analyzer.getCboInfo().contains("not optimized by CBO")); + ASTNode orderByRef = findFirstOrderByRef(analyzed.ast); + assertNotNull("expected an ORDER BY expression in " + query, orderByRef); + assertEquals("ORDER BY ordinal should be substituted with the select expression, got text=" + + orderByRef.getText() + " cboInfo=" + analyzed.analyzer.getCboInfo(), + HiveParser.TOK_TABLE_OR_COL, orderByRef.getType()); + } + + private AnalyzedQuery analyzeQueryWithCbo(String query) throws Exception { + HiveConf cboConf = new HiveConf(conf); + cboConf.setBoolVar(HiveConf.ConfVars.HIVE_CBO_ENABLED, true); + cboConf.setVar(HiveConf.ConfVars.HIVE_FETCH_TASK_CONVERSION, "none"); + SessionState.start(cboConf); + Context ctx = new Context(cboConf); + ASTNode astNode = ParseUtils.parse(query, ctx); + QueryState queryState = new QueryState.Builder().withHiveConf(cboConf).build(); + BaseSemanticAnalyzer analyzer = SemanticAnalyzerFactory.get(queryState, astNode); + analyzer.initCtx(ctx); + try { + analyzer.analyze(astNode, ctx); + } finally { + analyzer.endAnalysis(astNode); + } + return new AnalyzedQuery(analyzer, astNode); + } + + private static ASTNode findFirstOrderByRef(ASTNode node) { + if (node.getType() == HiveParser.TOK_ORDERBY && node.getChildCount() > 0 + && node.getChild(0).getChildCount() > 0) { + ASTNode colNode = (ASTNode) node.getChild(0).getChild(0); + if (colNode != null && colNode.getChildCount() > 0) { + return (ASTNode) colNode.getChild(0); + } + } + if (node.getChildren() == null) { + return null; + } + for (int i = 0; i < node.getChildCount(); i++) { + ASTNode found = findFirstOrderByRef((ASTNode) node.getChild(i)); + if (found != null) { + return found; + } + } + return null; + } + + private static final class AnalyzedQuery { + final BaseSemanticAnalyzer analyzer; + final ASTNode ast; + + AnalyzedQuery(BaseSemanticAnalyzer analyzer, ASTNode ast) { + this.analyzer = analyzer; + this.ast = ast; + } + } + private static ColumnInfo stringCol(String internalName, String tab, String alias, boolean markedAmbiguous) { ColumnInfo colInfo = new ColumnInfo(internalName, TypeInfoFactory.stringTypeInfo, tab, false); colInfo.setAlias(alias); diff --git a/ql/src/test/queries/clientpositive/order_by_pos_cbo_decline.q b/ql/src/test/queries/clientpositive/order_by_pos_cbo_decline.q new file mode 100644 index 000000000000..26d27618f674 --- /dev/null +++ b/ql/src/test/queries/clientpositive/order_by_pos_cbo_decline.q @@ -0,0 +1,12 @@ +-- HIVE-30037: CBO declines TABLESAMPLE; ORDER BY 1 DESC must return 3, 2, 1. +-- MiniLlapLocal: mvn test -pl itests/qtest -Dtest=TestMiniLlapLocalCliDriver -Dqfile=order_by_pos_cbo_decline.q +-- Overwrite golden: add -Dtest.output.overwrite=true + +set hive.fetch.task.conversion=none; +set hive.cbo.enable=true; + +create table hive30037_ob_t (d int); + +insert into hive30037_ob_t values (1), (2), (3); + +select d from hive30037_ob_t tablesample (5 rows) s order by 1 desc; diff --git a/ql/src/test/results/clientpositive/llap/order_by_pos_cbo_decline.q.out b/ql/src/test/results/clientpositive/llap/order_by_pos_cbo_decline.q.out new file mode 100644 index 000000000000..177dcfa96d66 --- /dev/null +++ b/ql/src/test/results/clientpositive/llap/order_by_pos_cbo_decline.q.out @@ -0,0 +1,28 @@ +PREHOOK: query: create table hive30037_ob_t (d int) +PREHOOK: type: CREATETABLE +PREHOOK: Output: database:default +PREHOOK: Output: default@hive30037_ob_t +POSTHOOK: query: create table hive30037_ob_t (d int) +POSTHOOK: type: CREATETABLE +POSTHOOK: Output: database:default +POSTHOOK: Output: default@hive30037_ob_t +PREHOOK: query: insert into hive30037_ob_t values (1), (2), (3) +PREHOOK: type: QUERY +PREHOOK: Input: _dummy_database@_dummy_table +PREHOOK: Output: default@hive30037_ob_t +POSTHOOK: query: insert into hive30037_ob_t values (1), (2), (3) +POSTHOOK: type: QUERY +POSTHOOK: Input: _dummy_database@_dummy_table +POSTHOOK: Output: default@hive30037_ob_t +POSTHOOK: Lineage: hive30037_ob_t.d SCRIPT [] +PREHOOK: query: select d from hive30037_ob_t tablesample (5 rows) s order by 1 desc +PREHOOK: type: QUERY +PREHOOK: Input: default@hive30037_ob_t +#### A masked pattern was here #### +POSTHOOK: query: select d from hive30037_ob_t tablesample (5 rows) s order by 1 desc +POSTHOOK: type: QUERY +POSTHOOK: Input: default@hive30037_ob_t +#### A masked pattern was here #### +3 +2 +1