This is an automated email from the ASF dual-hosted git repository. joemcdonnell pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit d0423a83efa73a42451a5e5c7ac39b43161dd61f Author: Joe McDonnell <[email protected]> AuthorDate: Sun Oct 27 16:10:20 2024 -0700 IMPALA-13495: Make exceptions from the Calcite planner easier to classify This makes several changes to the Calcite planner to improve the generated exceptions when there are errors: 1. When the Calcite parser produces SqlParseException, this is converted to Impala's regular ParseException. 2. When the Calcite validation fails, it produces a CalciteContextException, which is a wrapper around the real cause. This converts these validation errors into AnalysisExceptions. 3. This produces UnsupportedFeatureException for non-HDFS table types like Kudu, HBase, Iceberg, and views. It also produces UnsupportedFeatureException for HDFS tables with complex types (which otherwise would hit ClassCastException). 4. This changes exception handling in CalciteJniFrontend.java so it does not convert exceptions to InternalException. The JNI code will print the stacktrace for exceptions, so this drops the existing call to print the exception stack trace. Testing: - Ran some end-to-end tests with a mode that continues past failures and examined the output. Change-Id: I6702ceac1d1d67c3d82ec357d938f12a6cf1c828 Reviewed-on: http://gerrit.cloudera.org:8080/21989 Reviewed-by: Michael Smith <[email protected]> Reviewed-by: Joe McDonnell <[email protected]> Tested-by: Joe McDonnell <[email protected]> --- .../java/org/apache/impala/analysis/Parser.java | 8 +---- .../org/apache/impala/common/ParseException.java | 35 ++++++++++++++++++++++ .../apache/impala/calcite/schema/CalciteTable.java | 7 ++++- .../impala/calcite/service/CalciteJniFrontend.java | 25 ++++------------ .../calcite/service/CalciteMetadataHandler.java | 7 +++++ .../impala/calcite/service/CalciteQueryParser.java | 25 +++++++++------- .../impala/calcite/service/CalciteValidator.java | 14 ++++++--- 7 files changed, 79 insertions(+), 42 deletions(-) diff --git a/fe/src/main/java/org/apache/impala/analysis/Parser.java b/fe/src/main/java/org/apache/impala/analysis/Parser.java index 184d975e2..4c111c6ab 100644 --- a/fe/src/main/java/org/apache/impala/analysis/Parser.java +++ b/fe/src/main/java/org/apache/impala/analysis/Parser.java @@ -20,6 +20,7 @@ package org.apache.impala.analysis; import java.io.StringReader; import org.apache.impala.common.AnalysisException; +import org.apache.impala.common.ParseException; import org.apache.impala.thrift.TQueryOptions; /** @@ -27,13 +28,6 @@ import org.apache.impala.thrift.TQueryOptions; * the expected class and exceptions to an ParseException. */ public class Parser { - @SuppressWarnings("serial") - public static class ParseException extends AnalysisException { - public ParseException(String msg, Exception e) { - super(msg, e); - } - } - /** * Parse the statement using default options. Used for testing and for * parsing internally-generated statements. See the full version for details. diff --git a/fe/src/main/java/org/apache/impala/common/ParseException.java b/fe/src/main/java/org/apache/impala/common/ParseException.java new file mode 100644 index 000000000..3e827aa42 --- /dev/null +++ b/fe/src/main/java/org/apache/impala/common/ParseException.java @@ -0,0 +1,35 @@ +// 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.impala.common; + +/** + * Thrown for errors encountered when parsing a SQL statement. + */ +public class ParseException extends AnalysisException { + public ParseException(String msg, Throwable cause) { + super(msg, cause); + } + + public ParseException(String msg) { + super(msg); + } + + public ParseException(Throwable cause) { + super(cause); + } +} diff --git a/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java b/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java index 9f86b0ac0..5f70405c8 100644 --- a/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java +++ b/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java @@ -88,12 +88,17 @@ public class CalciteTable extends RelOptAbstractTable } } - private static RelDataType buildColumnsForRelDataType(FeTable table) { + private static RelDataType buildColumnsForRelDataType(FeTable table) + throws ImpalaException { RelDataTypeFactory typeFactory = new JavaTypeFactoryImpl(new ImpalaTypeSystemImpl()); RelDataTypeFactory.Builder builder = new RelDataTypeFactory.Builder(typeFactory); // skip clustering columns, save them for the end for (Column column : table.getColumnsInHiveOrder()) { + if (column.getType().isComplexType()) { + throw new UnsupportedFeatureException( + "Calcite does not support complex types yet."); + } RelDataType type = ImpalaTypeConverter.createRelDataType(typeFactory, column.getType()); builder.add(column.getName(), type); diff --git a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java index 44708ac37..ac96e8344 100644 --- a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java +++ b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java @@ -24,7 +24,6 @@ import org.apache.calcite.rel.metadata.JaninoRelMetadataProvider; import org.apache.calcite.rel.metadata.RelMetadataQuery; import org.apache.calcite.sql.SqlExplain; import org.apache.calcite.sql.SqlNode; -import org.apache.calcite.sql.parser.SqlParseException; import org.apache.impala.analysis.Parser; import org.apache.impala.analysis.SelectStmt; import org.apache.impala.calcite.functions.FunctionResolver; @@ -33,8 +32,8 @@ import org.apache.impala.calcite.rel.node.NodeWithExprs; import org.apache.impala.calcite.rel.node.ImpalaPlanRel; import org.apache.impala.catalog.BuiltinsDb; import org.apache.impala.common.ImpalaException; -import org.apache.impala.common.InternalException; import org.apache.impala.common.JniUtil; +import org.apache.impala.common.ParseException; import org.apache.impala.service.Frontend; import org.apache.impala.service.FrontendProfile; import org.apache.impala.service.JniFrontend; @@ -42,8 +41,6 @@ import org.apache.impala.thrift.TExecRequest; import org.apache.impala.thrift.TQueryCtx; import org.apache.impala.thrift.TQueryOptions; import org.apache.thrift.TException; -import org.apache.thrift.TSerializer; -import org.apache.thrift.protocol.TBinaryProtocol; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.exception.ExceptionUtils; @@ -63,9 +60,6 @@ public class CalciteJniFrontend extends JniFrontend { protected static final Logger LOG = LoggerFactory.getLogger(CalciteJniFrontend.class.getName()); - private final static TBinaryProtocol.Factory protocolFactory_ = - new TBinaryProtocol.Factory(); - private static Pattern SEMI_JOIN = Pattern.compile("\\bsemi\\sjoin\\b", Pattern.CASE_INSENSITIVE); @@ -137,29 +131,20 @@ public class CalciteJniFrontend extends JniFrontend { TExecRequest execRequest = execRequestCreator.create(rootNode); markEvent(mdHandler, execRequest, queryCtx, "Created exec request"); - TSerializer serializer = new TSerializer(protocolFactory_); - byte[] serializedRequest = serializer.serialize(execRequest); + byte[] serializedRequest = JniUtil.serializeToThrift(execRequest); queryCtx.getTimeline().markEvent("Serialized request"); return serializedRequest; - } catch (SqlParseException e) { + } catch (ParseException e) { // do a quick parse just to make sure it's not a select stmt. If it is // a select statement, we fail the query since all select statements // should be run through the Calcite Planner. if (Parser.parse(queryCtx.getStmt()) instanceof SelectStmt) { - throw new InternalException(e.getMessage()); + throw e; } LOG.info("Calcite planner failed to parse query: " + queryCtx.getStmt()); LOG.info("Going to use original Impala planner."); return runThroughOriginalPlanner(thriftQueryContext, queryCtx); - } catch (Exception e) { - LOG.info("Calcite planner failed."); - LOG.info("Exception: " + e); - if (e != null) { - LOG.info("Stack Trace:" + ExceptionUtils.getStackTrace(e)); - throw new InternalException(e.getMessage()); - } - throw new RuntimeException(e); } } @@ -195,7 +180,7 @@ public class CalciteJniFrontend extends JniFrontend { public QueryContext(byte[] thriftQueryContext, Frontend frontend) throws ImpalaException { this.queryCtx_ = new TQueryCtx(); - JniUtil.deserializeThrift(protocolFactory_, queryCtx_, thriftQueryContext); + JniUtil.deserializeThrift(queryCtx_, thriftQueryContext); // hack to match the code in Frontend.java: // If unset, set MT_DOP to 0 to simplify the rest of the code. diff --git a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java index ed0969fc2..13ff2f678 100644 --- a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java +++ b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java @@ -46,6 +46,7 @@ import org.apache.impala.catalog.FeTable; import org.apache.impala.catalog.FeView; import org.apache.impala.catalog.HdfsTable; import org.apache.impala.common.ImpalaException; +import org.apache.impala.common.UnsupportedFeatureException; import java.util.ArrayList; import java.util.Collections; @@ -136,6 +137,12 @@ public class CalciteMetadataHandler implements CompilerStep { if (feTable == null) { continue; } + if (!(feTable instanceof HdfsTable)) { + throw new UnsupportedFeatureException( + "Table " + feTable.getFullName() + " has unsupported type " + + feTable.getClass().getSimpleName() + ". The Calcite planner only supports " + + "HDFS tables."); + } // populate the dbschema with its table, creating the dbschema if it's the // first instance seen in the query. diff --git a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteQueryParser.java b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteQueryParser.java index 718db240d..898cd9147 100644 --- a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteQueryParser.java +++ b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteQueryParser.java @@ -23,6 +23,7 @@ import org.apache.calcite.sql.parser.SqlParser; import org.apache.calcite.sql.parser.SqlParseException; import org.apache.impala.calcite.parser.ImpalaSqlParserImpl; import org.apache.impala.calcite.validate.ImpalaConformance; +import org.apache.impala.common.ParseException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -41,17 +42,21 @@ public class CalciteQueryParser implements CompilerStep { this.queryCtx_ = queryCtx; } - public SqlNode parse() throws SqlParseException { - // Create an SQL parser - SqlParser parser = SqlParser.create(queryCtx_.getStmt(), - SqlParser.config().withParserFactory(ImpalaSqlParserImpl.FACTORY) - .withConformance(ImpalaConformance.INSTANCE) - .withQuoting(Quoting.BACK_TICK_BACKSLASH) - ); + public SqlNode parse() throws ParseException { + try { + // Create an SQL parser + SqlParser parser = SqlParser.create(queryCtx_.getStmt(), + SqlParser.config().withParserFactory(ImpalaSqlParserImpl.FACTORY) + .withConformance(ImpalaConformance.INSTANCE) + .withQuoting(Quoting.BACK_TICK_BACKSLASH) + ); - // Parse the query into an AST - SqlNode sqlNode = parser.parseQuery(); - return sqlNode; + // Parse the query into an AST + SqlNode sqlNode = parser.parseQuery(); + return sqlNode; + } catch (SqlParseException e) { + throw new ParseException(e.getMessage()); + } } public void logDebug(Object resultObject) { diff --git a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java index 8bd6c7e17..028ee0460 100644 --- a/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java +++ b/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteValidator.java @@ -20,6 +20,7 @@ package org.apache.impala.calcite.service; import org.apache.calcite.jdbc.JavaTypeFactoryImpl; import org.apache.calcite.prepare.CalciteCatalogReader; import org.apache.calcite.rel.type.RelDataTypeFactory; +import org.apache.calcite.runtime.CalciteContextException; import org.apache.calcite.sql.SqlNode; import org.apache.calcite.sql.fun.SqlStdOperatorTable; import org.apache.calcite.sql.validate.SqlValidator; @@ -27,6 +28,7 @@ import org.apache.calcite.sql.validate.SqlValidatorUtil; import org.apache.impala.calcite.operators.ImpalaOperatorTable; import org.apache.impala.calcite.type.ImpalaTypeSystemImpl; import org.apache.impala.calcite.validate.ImpalaConformance; +import org.apache.impala.common.AnalysisException; import org.slf4j.Logger; @@ -63,10 +65,14 @@ public class CalciteValidator implements CompilerStep { ); } - public SqlNode validate(SqlNode parsedNode) { - // Validate the initial AST - SqlNode node = sqlValidator.validate(parsedNode); - return node; + public SqlNode validate(SqlNode parsedNode) throws AnalysisException { + try { + // Validate the initial AST + SqlNode node = sqlValidator.validate(parsedNode); + return node; + } catch (CalciteContextException e) { + throw new AnalysisException(e.getMessage(), e.getCause()); + } } public RelDataTypeFactory getTypeFactory() {
