This is an automated email from the ASF dual-hosted git repository. jbapple pushed a commit to branch branch-4.0.0 in repository https://gitbox.apache.org/repos/asf/impala.git
The following commit(s) were added to refs/heads/branch-4.0.0 by this push: new 9ac4a53 IMPALA-7844: Default to not allow ordinals in HAVING clause 9ac4a53 is described below commit 9ac4a53c61560a5aefb4da1d9a1997fd3cd1357f Author: stiga-huang <huangquanl...@gmail.com> AuthorDate: Mon May 10 16:45:31 2021 +0800 IMPALA-7844: Default to not allow ordinals in HAVING clause Base on the discussion of the previous patch, we decide to not allow ordinals in the HAVING clause since 4.0. It's a non-standard feature that unintentionally supported by Impala 3.x and earlier versions. This patch disables it by default, and add a feature flag to turn it on for users that do depend on it. Tests: - Modify existing FE tests to test on the flag. - Add custom cluster test to verify the flag works. Change-Id: I0a57b8b65b046fae483e485e8391f8222fa586a5 Reviewed-on: http://gerrit.cloudera.org:8080/17415 Reviewed-by: Zoltan Borok-Nagy <borokna...@cloudera.com> Tested-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com> --- be/src/util/backend-gflag-util.cc | 6 ++++ common/thrift/BackendGflags.thrift | 2 ++ .../org/apache/impala/analysis/SelectStmt.java | 37 ++-------------------- .../org/apache/impala/service/BackendConfig.java | 8 +++++ .../apache/impala/analysis/AnalyzeStmtsTest.java | 12 ++++--- tests/custom_cluster/test_disable_features.py | 16 ++++++++++ 6 files changed, 42 insertions(+), 39 deletions(-) diff --git a/be/src/util/backend-gflag-util.cc b/be/src/util/backend-gflag-util.cc index 54e4c39..a218c06 100644 --- a/be/src/util/backend-gflag-util.cc +++ b/be/src/util/backend-gflag-util.cc @@ -160,6 +160,11 @@ DEFINE_bool(enable_row_filtering, true, "If false, disable the row filtering feature. Defaults to be true. Enabling this flag" " requires enable_column_masking to be true."); +DEFINE_bool(allow_ordinals_in_having, false, + "If true, allow using ordinals in HAVING clause. This non-standard feature is " + "supported in Impala 3.x and earlier. We intend to disable it since 4.0. So it " + "defaults to be false. See IMPALA-7844."); + namespace impala { Status GetConfigFromCommand(const string& flag_cmd, string& result) { @@ -279,6 +284,7 @@ Status PopulateThriftBackendGflags(TBackendGflags& cfg) { cfg.__set_saml2_ee_test_mode(FLAGS_saml2_ee_test_mode); cfg.__set_scratch_dirs(FLAGS_scratch_dirs); cfg.__set_max_wait_time_for_sync_ddl_s(FLAGS_max_wait_time_for_sync_ddl_s); + cfg.__set_allow_ordinals_in_having(FLAGS_allow_ordinals_in_having); return Status::OK(); } diff --git a/common/thrift/BackendGflags.thrift b/common/thrift/BackendGflags.thrift index 7d75c69..2d25efe 100644 --- a/common/thrift/BackendGflags.thrift +++ b/common/thrift/BackendGflags.thrift @@ -195,4 +195,6 @@ struct TBackendGflags { 85: required bool enable_row_filtering 86: required i32 max_wait_time_for_sync_ddl_s + + 87: required bool allow_ordinals_in_having } diff --git a/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java b/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java index ef175eb..e153195 100644 --- a/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java @@ -19,7 +19,6 @@ package org.apache.impala.analysis; import java.util.ArrayList; import java.util.Collections; -import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Set; @@ -39,6 +38,7 @@ import org.apache.impala.common.Pair; import org.apache.impala.common.TableAliasGenerator; import org.apache.impala.common.TreeNode; import org.apache.impala.rewrite.ExprRewriter; +import org.apache.impala.service.BackendConfig; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -54,39 +54,6 @@ import com.google.common.collect.Lists; public class SelectStmt extends QueryStmt { private final static Logger LOG = LoggerFactory.getLogger(SelectStmt.class); - /** - * Impala 3.x and earlier provide non-standard support for a single ordinal - * in the HAVING clause: - * - * SELECT region, count(*), count(*) > 10 FROM sales - * HAVING 3 - * - * Most other SQL dialects do not allow ordinals in expressions (such as - * HAVING), only in lists (GROUP BY, ORDER BY). The standard way to express - * the above is: - * - * SELECT region, count(*) FROM sales - * HAVING count(*) > 10 - * - * Or, better (not currently supported by IMPALA): - * - * SELECT region, count(*) c FROM SALES - * HAVING c > 10 - * - * We intend to disable this non-standard feature in the next major - * release. For now, we leave the feature/bug enabled, controlled by - * the following constant. - * - * In that same release, we will enable the third example above. - * At present, all that is supported is the odd: - * - * SELECT region, count(*), count(*) > 10 p FROM sales - * HAVING p - * - * See IMPALA-7844. - */ - public static final boolean ALLOW_ORDINALS_IN_HAVING = true; - ///////////////////////////////////////// // BEGIN: Members that need to be reset() @@ -739,7 +706,7 @@ public class SelectStmt extends QueryStmt { } // Resolve (top-level) aliases and analyzes havingPred_ = resolveReferenceExpr(havingClause_, "HAVING", analyzer_, - ALLOW_ORDINALS_IN_HAVING); + BackendConfig.INSTANCE.getAllowOrdinalsInHaving()); // can't contain analytic exprs Expr analyticExpr = havingPred_.findFirstOf(AnalyticExpr.class); if (analyticExpr != null) { diff --git a/fe/src/main/java/org/apache/impala/service/BackendConfig.java b/fe/src/main/java/org/apache/impala/service/BackendConfig.java index 9513b9b..4f68669 100644 --- a/fe/src/main/java/org/apache/impala/service/BackendConfig.java +++ b/fe/src/main/java/org/apache/impala/service/BackendConfig.java @@ -253,6 +253,14 @@ public class BackendConfig { public String getScratchDirs() { return backendCfg_.scratch_dirs; } + public boolean getAllowOrdinalsInHaving() { + return backendCfg_.allow_ordinals_in_having; + } + + public void setAllowOrdinalsInHaving(boolean allow_ordinals_in_having) { + backendCfg_.allow_ordinals_in_having = allow_ordinals_in_having; + } + // Inits the auth_to_local configuration in the static KerberosName class. private static void initAuthToLocal() { // If auth_to_local is enabled, we read the configuration hadoop.security.auth_to_local diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java index 59c0028..12aa6bd 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java @@ -37,6 +37,7 @@ import org.apache.impala.catalog.Type; import org.apache.impala.common.AnalysisException; import org.apache.impala.common.FileSystemUtil; import org.apache.impala.common.ImpalaException; +import org.apache.impala.service.BackendConfig; import org.apache.impala.thrift.TFunctionCategory; import org.junit.Assert; import org.junit.Test; @@ -1138,9 +1139,10 @@ public class AnalyzeStmtsTest extends AnalyzerTest { // HAVING is an expression, not a list like GROUP BY or ORDER BY. // Verify that an integer is treated as a constant, not an ordinal. @Test - public void TestHavingIntegers() throws AnalysisException { - if (SelectStmt.ALLOW_ORDINALS_IN_HAVING) { + public void TestHavingIntegers() { + try { // Legacy 3.x functionality + BackendConfig.INSTANCE.setAllowOrdinalsInHaving(true); AnalyzesOk("select not bool_col as nb from functional.alltypes having 1"); AnalysisError("select count(*) from functional.alltypes having 1", "HAVING clause 'count(*)' requires return type 'BOOLEAN'. " + @@ -1151,15 +1153,15 @@ public class AnalyzeStmtsTest extends AnalyzerTest { "sum(id) OVER (ORDER BY id ASC)"); AnalysisError("select sum(id) over(order by id) from functional.alltypes having -1", "HAVING: ordinal must be >= 1: -1"); - } else { // Forward-looking, post 3.x functionality // IMPALA-7844: Ordinals not supported in HAVING since // HAVING is an expression, not a list like GROUP BY or ORDER BY. + BackendConfig.INSTANCE.setAllowOrdinalsInHaving(false); AnalysisError("select not bool_col as nb from functional.alltypes having 1", "HAVING clause '1' requires return type 'BOOLEAN'. " + "Actual type is 'TINYINT'."); AnalysisError("select not bool_col as nb from functional.alltypes having -1", - "HAVING clause '1' requires return type 'BOOLEAN'. " + + "HAVING clause '-1' requires return type 'BOOLEAN'. " + "Actual type is 'TINYINT'."); // Constant exprs should not be interpreted as ordinals AnalysisError("select int_col, bool_col, count(*) from functional.alltypes " + @@ -1170,6 +1172,8 @@ public class AnalyzeStmtsTest extends AnalyzerTest { "group by 1, 2 having if(TRUE, 2, int_col)", "HAVING clause 'if(TRUE, 2, int_col)' requires return type 'BOOLEAN'. " + "Actual type is 'INT'."); + } finally { + BackendConfig.INSTANCE.setAllowOrdinalsInHaving(false); } } diff --git a/tests/custom_cluster/test_disable_features.py b/tests/custom_cluster/test_disable_features.py index a8d2c0b..8d2725c 100644 --- a/tests/custom_cluster/test_disable_features.py +++ b/tests/custom_cluster/test_disable_features.py @@ -47,3 +47,19 @@ class TestDisableFeatures(CustomClusterTestSuite): use_db=unique_database, multiple_impalad=True) self.run_test_case('QueryTest/alter-table-set-column-stats', vector, use_db=unique_database, multiple_impalad=True) + + @pytest.mark.execute_serially + @CustomClusterTestSuite.with_args("--allow_ordinals_in_having=true") + def test_allow_ordinals_in_having(self, vector): + """Mirror the FE tests in AnalyzeStmtsTest#TestHavingIntegers to make sure the flag + can bring back the legacy feature""" + self.client.execute("select not bool_col as nb from functional.alltypes having 1") + self.execute_query_expect_failure( + self.client, "select count(*) from functional.alltypes having 1") + self.client.execute("select count(*) > 10 from functional.alltypes having 1") + self.execute_query_expect_failure( + self.client, + "select sum(id) over(order by id) from functional.alltypes having 1") + self.execute_query_expect_failure( + self.client, + "select sum(id) over(order by id) from functional.alltypes having -1")