HIVE-30037: Resolve ORDER BY ordinals when CBO declines the statement

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);
       }
     }

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);

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<String> 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<String> 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<String> 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);
+  }
+}

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);

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;

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