You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2017/12/09 06:13:54 UTC
[1/4] impala git commit: IMPALA-6280: Materialize
TupleIsNullPredicate for insert sorts
Repository: impala
Updated Branches:
refs/heads/master 11497c2aa -> 12fa20d1a
IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
When a sort is inserted into a plan for an INSERT due to either the
target table being a Kudu table or the use of the 'clustered' hint,
and a TupleIsNullPredicate is present in the output of the sort, the
TupleIsNullPredicate may reference an incorrect tuple (i.e. not the
materialized sort tuple), leading to errors.
The solution is to materialize the TupleIsNullPredicate into the sort
tuple and then perform the appropriate expr substitutions, as is
already done for the case of analytic sorts.
Testing:
- Added an e2e test with a query that would previously fail.
Change-Id: I6c0ca717aa4321a5cc84edd1d5857912f8c85583
Reviewed-on: http://gerrit.cloudera.org:8080/8791
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/2e83ba57
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/2e83ba57
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/2e83ba57
Branch: refs/heads/master
Commit: 2e83ba5796d42bf3a568bfae5823896dfa644cc2
Parents: 11497c2
Author: Thomas Tauber-Marshall <tm...@cloudera.com>
Authored: Thu Dec 7 14:39:31 2017 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Fri Dec 8 23:58:18 2017 +0000
----------------------------------------------------------------------
.../org/apache/impala/analysis/SortInfo.java | 35 ++++++++++++++++++--
.../apache/impala/planner/AnalyticPlanner.java | 19 ++++-------
.../queries/QueryTest/insert.test | 11 ++++++
.../queries/QueryTest/kudu_upsert.test | 10 ++++++
4 files changed, 60 insertions(+), 15 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/2e83ba57/fe/src/main/java/org/apache/impala/analysis/SortInfo.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/SortInfo.java b/fe/src/main/java/org/apache/impala/analysis/SortInfo.java
index 88cb375..745de6d 100644
--- a/fe/src/main/java/org/apache/impala/analysis/SortInfo.java
+++ b/fe/src/main/java/org/apache/impala/analysis/SortInfo.java
@@ -192,8 +192,9 @@ public class SortInfo {
// materialized ordering exprs. Using a LinkedHashSet prevents the slots getting
// reordered unnecessarily.
Set<SlotRef> sourceSlots = Sets.newLinkedHashSet();
- TreeNode.collect(Expr.substituteList(resultExprs, substOrderBy, analyzer, false),
- Predicates.instanceOf(SlotRef.class), sourceSlots);
+ List<Expr> substResultExprs =
+ Expr.substituteList(resultExprs, substOrderBy, analyzer, false);
+ TreeNode.collect(substResultExprs, Predicates.instanceOf(SlotRef.class), sourceSlots);
TreeNode.collect(Expr.substituteList(orderingExprs_, substOrderBy, analyzer, false),
Predicates.instanceOf(SlotRef.class), sourceSlots);
for (SlotRef origSlotRef: sourceSlots) {
@@ -208,6 +209,9 @@ public class SortInfo {
}
}
+ materializeTupleIsNullPredicates(
+ sortTupleDesc, substResultExprs, sortTupleExprs, substOrderBy, analyzer);
+
// The ordering exprs are evaluated against the sort tuple, so they must reflect the
// materialization decision above.
substituteOrderingExprs(substOrderBy, analyzer);
@@ -247,4 +251,31 @@ public class SortInfo {
}
return substOrderBy;
}
+
+ /**
+ * Collects the unique TupleIsNullPredicates from 'exprs' and for each one:
+ * - Materializes it into a new slot in 'sortTupleDesc'
+ * - Adds it to 'sortSlotExprs'
+ * - Adds an entry in 'substOrderBy' mapping it to a SlotRef into the new slot
+ */
+ public static void materializeTupleIsNullPredicates(TupleDescriptor sortTupleDesc,
+ List<Expr> exprs, List<Expr> sortSlotExprs, ExprSubstitutionMap substOrderBy,
+ Analyzer analyzer) {
+ List<Expr> tupleIsNullPreds = Lists.newArrayList();
+ TreeNode.collect(
+ exprs, Predicates.instanceOf(TupleIsNullPredicate.class), tupleIsNullPreds);
+ Expr.removeDuplicates(tupleIsNullPreds);
+
+ // Materialize relevant unique TupleIsNullPredicates.
+ for (Expr tupleIsNullPred: tupleIsNullPreds) {
+ SlotDescriptor sortSlotDesc = analyzer.addSlotDescriptor(sortTupleDesc);
+ sortSlotDesc.setType(tupleIsNullPred.getType());
+ sortSlotDesc.setIsMaterialized(true);
+ sortSlotDesc.setSourceExpr(tupleIsNullPred);
+ sortSlotDesc.setLabel(tupleIsNullPred.toSql());
+ SlotRef cloneRef = new SlotRef(sortSlotDesc);
+ substOrderBy.put(tupleIsNullPred, cloneRef);
+ sortSlotExprs.add(tupleIsNullPred.clone());
+ }
+ }
}
http://git-wip-us.apache.org/repos/asf/impala/blob/2e83ba57/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java b/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
index 1140a7d..156bd5b 100644
--- a/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
+++ b/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
@@ -284,24 +284,17 @@ public class AnalyticPlanner {
// by a SlotRef into the sort's tuple in ancestor nodes (IMPALA-1519).
ExprSubstitutionMap inputSmap = input.getOutputSmap();
if (inputSmap != null) {
- List<Expr> tupleIsNullPredsToMaterialize = Lists.newArrayList();
+ List<Expr> relevantRhsExprs = Lists.newArrayList();
for (int i = 0; i < inputSmap.size(); ++i) {
Expr rhsExpr = inputSmap.getRhs().get(i);
// Ignore substitutions that are irrelevant at this plan node and its ancestors.
- if (!rhsExpr.isBoundByTupleIds(input.getTupleIds())) continue;
- rhsExpr.collect(TupleIsNullPredicate.class, tupleIsNullPredsToMaterialize);
+ if (rhsExpr.isBoundByTupleIds(input.getTupleIds())) {
+ relevantRhsExprs.add(rhsExpr);
+ }
}
- Expr.removeDuplicates(tupleIsNullPredsToMaterialize);
- // Materialize relevant unique TupleIsNullPredicates.
- for (Expr tupleIsNullPred: tupleIsNullPredsToMaterialize) {
- SlotDescriptor sortSlotDesc = analyzer_.addSlotDescriptor(sortTupleDesc);
- sortSlotDesc.setType(tupleIsNullPred.getType());
- sortSlotDesc.setIsMaterialized(true);
- sortSlotDesc.setSourceExpr(tupleIsNullPred);
- sortSlotDesc.setLabel(tupleIsNullPred.toSql());
- sortSlotExprs.add(tupleIsNullPred.clone());
- }
+ SortInfo.materializeTupleIsNullPredicates(sortTupleDesc, relevantRhsExprs,
+ sortSlotExprs, sortSmap, analyzer_);
}
SortInfo sortInfo = new SortInfo(sortExprs, isAsc, nullsFirst);
http://git-wip-us.apache.org/repos/asf/impala/blob/2e83ba57/testdata/workloads/functional-query/queries/QueryTest/insert.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/insert.test b/testdata/workloads/functional-query/queries/QueryTest/insert.test
index 5601b35..07a665a 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/insert.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/insert.test
@@ -945,3 +945,14 @@ RESET alltypesnopart_insert
---- RESULTS
: 100
====
+---- QUERY
+# IMPALA-6280: clustered with outer join, inline view, and TupleisNullPredicate
+insert into table alltypesinsert (int_col)
+partition (year, month) /*+ clustered,shuffle */
+select v.id, t1.id, t1.month from
+(select coalesce(id, 10) id from functional.alltypessmall) v
+right outer join functional.alltypestiny t1 on t1.id = v.id
+where v.id = 0
+---- RESULTS
+year=0/month=1/: 1
+====
\ No newline at end of file
http://git-wip-us.apache.org/repos/asf/impala/blob/2e83ba57/testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test b/testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test
index 8180075..57aaa46 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test
@@ -388,6 +388,16 @@ NumRowErrors: 0
---- RESULTS
====
---- QUERY
+# IMPALA-6280: check that TupleIsNullPredicate is materialized correctly
+upsert into table tdata (id, vali)
+select t1.id, v.id from functional.alltypestiny t1
+left outer join (select ifnull(id, 10) id from functional.alltypessmall) v
+on t1.id = v.id limit 1
+---- RUNTIME_PROFILE
+NumModifiedRows: 1
+NumRowErrors: 0
+====
+---- QUERY
create table multiple_key_cols
(string_col string, bigint_col bigint, tinyint_col tinyint, smallint_col smallint,
bool_col boolean null, int_col int null, double_col double null,
[2/4] impala git commit: IMPALA-6067: Enable S3 access via IAM roles
for EC2 VMs
Posted by ta...@apache.org.
IMPALA-6067: Enable S3 access via IAM roles for EC2 VMs
For some time Impala in a production environment has been able
to access data stored in Amazon S3 buckets using credentials specified
in a number of ways:
- storing Amazon access keys in environment variables or
in core-site.xml.
- using proprietary management tools to store Amazon access keys
securely
- using Amazon IAM roles bound to VMs running in EC2.
The development minicluster environment used the first approach,
which risked leaking these keys.
This change enables Impala builds to use IAM
roles to access S3 buckets when running on an Amazon EC2 virtual
machine. The changes mainly ensure that environment variables carrying
the traditional AWS credentials do not conflict with credentials supplied
by the IAM role attached to the VM instance.
IAM role based credentials are accessible through the EC2
instance-property mechanism; for further details see Amazon's docs at
http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/iam-roles-for-amazon-ec2.html#instance-metadata-security-credentials
The change also removes the remaining references to the s3n: provider.
In the FE tests all URIs referring to s3n: are replaced with their
s3a: equivalents, except for a single negative test in
AnalyzeStmtsTest.java, which is removed.
In addition to the code changes, the s3n: and s3a: credential properties
are also removed from core-site.xml.tmpl. The s3a: provider can pick up
AWS S3 credentials from environment variables or IAM properties bound
to the VM instance, which is a more flexible approach.
As environment variables have precedence over IAM roles, care must be
taken when managing the canonical environment variables carrying
AWS credentials. There are two requirements to be reconciled:
1. The FE tests have code that examines s3a: URIs; this code needs
existing, but not necessarily valid AWS credentials.
2. When the Impala test suite is executed on an EC2 VM, AWS credentials
can be supplied via IAM roles. These credentials can be used only
if the AWS_* environment variables are unset (do not exist).
The tradeoff is managed following these rules:
1. When AWS_* environment variables are set before invoking the
Impala configuration scripts, their value is preserved and
the config scripts ensure that the variables are exported.
2. If the AWS_* variables are missing or empty, they will be unset
to ensure that credentials supplied by Amazon's IAM roles can be
accessed,
3. except if the scripts are running outside of EC2 (so there can be
no IAM roles) and TARGET_FILESYSTEM is not set "s3". This combination
is most often the case on a developer's local workstation.
In this case the AWS_* credential variables are forcibly set to
dummy values to allow the FE tests to succeed.
The removal of S3 credential parameters from core-site.xml[.tmpl]
also allows users to set up their own credentials there,
the config scripts will not change those settings.
Environment variables carrying AWS security credentials will be set
up according to the following table:
Instance: Running outside EC2 || Running in EC2 |
--------------------+--------+--------++--------+--------+
TARGET_FILESYSTEM | S3 | not S3 || S3 | not S3 |
--------------------+--------+--------++--------+--------+
| | || | |
empty | unset | dummy || unset | unset |
AWS_* | | || | |
env --------------+--------+--------++--------+--------+
var | | || | |
not empty | export | export || export | export |
| | || | |
--------------------+--------+--------++--------+--------+
Legend: unset: the variable is unset
export: the variable is exported with its current value
dummy: the variable is set to a preset dummy value and
exported
Running on an EC2 VM is indicated by setting RUNNING_IN_EC2 to "true" and
exporting it before impala_config.sh is invoked.
The change also moves the logic performing the S3 access checks into a separate
script file: bin/check-s3-access.sh. This file now contains all the S3-specific
logic and network access to check if the requested S3 bucket can be accessed.
Testing:
Performed local builds for HDFS as well as automated builds against
HDFS and S3, using both IAM roles and explicit AWS_* credentials for
authentication.
Verified that FE tests that parse s3a: URLs are still successful in
all these combinations (when they are run).
Change-Id: I14cd9d4453a91baad3c379aa7e4944993fca95ae
Reviewed-on: http://gerrit.cloudera.org:8080/8294
Reviewed-by: Philip Zeyliger <ph...@cloudera.com>
Reviewed-by: Zach Amsden <za...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/e81b7c6b
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/e81b7c6b
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/e81b7c6b
Branch: refs/heads/master
Commit: e81b7c6b682fa4bf06db0fbd30818fe698890b00
Parents: 2e83ba5
Author: Laszlo Gaal <la...@cloudera.com>
Authored: Fri Oct 13 00:36:57 2017 +0200
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Sat Dec 9 01:43:01 2017 +0000
----------------------------------------------------------------------
bin/check-s3-access.sh | 115 +++++++++++++++++++
bin/impala-config.sh | 73 +++++++++---
.../apache/impala/analysis/AnalyzeDDLTest.java | 12 +-
.../impala/analysis/AnalyzeStmtsTest.java | 4 -
.../common/etc/hadoop/conf/core-site.xml.tmpl | 24 +---
5 files changed, 176 insertions(+), 52 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/e81b7c6b/bin/check-s3-access.sh
----------------------------------------------------------------------
diff --git a/bin/check-s3-access.sh b/bin/check-s3-access.sh
new file mode 100755
index 0000000..68df910
--- /dev/null
+++ b/bin/check-s3-access.sh
@@ -0,0 +1,115 @@
+#!/bin/bash
+# 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.
+#
+# This file is invoked if TARGET_FILESYSTEM is set to "s3" to check
+# preconditions for accessing the specified S3 bucket. It inherits the
+# environment variables from its caller and uses them as
+# implicit parameters.
+#
+# The following environment variables are used:
+# TARGET_FILESYSTEM
+# S3_BUCKET
+# AWS_ACCESS_KEY_ID
+# AWS_SECRET_ACCESS_KEY
+#
+# Returns:
+# 0 (success): if preconditions for S3 access are satisfied.
+# 1 (failure): if S3 access is unsuccessful.
+# 2 (error): if the 'aws' executable is not on the path, or other
+# environmental problems cause the script to fail.
+#
+# If tests are to be run against S3 as the backing file system, verify that
+# the assigned S3 bucket can be accessed.
+# Access can be authorized by AWS_ credentials passed in environment variables
+# or an EC2 IAM role assigned to the VM running the tests.
+#
+# If S3 access is granted via an IAM role assigned to the VM instance,
+# then the credentials bound to the IAM role are retrieved automatically
+# both by the Hadoop s3a: provider and by AWSCLI.
+# In this case AWS keys must not be present in environment variables or in
+# core-site.xml because their presence would preempt the IAM-based
+# credentials.
+#
+# For further details on IAM roles refer to the Amazon docs at
+# http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/iam-roles-for-amazon-ec2.html#instance-metadata-security-credentials
+#
+# The assigned IAM role and the security credentials provided
+# by the role can be queried through the AWS instance metadata mechanism.
+# Instance metadata is served through an HTTP connection to the special
+# address 169.254.169.254. Details are described at:
+# http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-metadata.html#instancedata-data-retrieval
+AWS_METADATA_IP_ADDRESS="169.254.169.254"
+
+# safeguard against bad calls: if no S3 access is requested, just succeed
+# silently.
+if [ "${TARGET_FILESYSTEM-}" != "s3" ]; then
+ exit 0
+fi
+
+echo "Checking S3 access"
+
+# Check if the S3 bucket name is NULL.
+if [[ -z ${S3_BUCKET-} ]]; then
+ echo "Error: S3_BUCKET cannot be an empty string"
+ exit 1
+fi
+# S3 access can be granted using access keys passed in via the environment
+# or specifying an IAM role that has S3 access privileges.
+# First check the environment variables, they have precedence over the IAM
+# role: invalid credentials in the environment variables will prevent S3 access
+# even if a valid IAM role is present.
+# Use a subshell to prevent leaking AWS secrets.
+if (set +x; [[ -z ${AWS_ACCESS_KEY_ID-} && -z ${AWS_SECRET_ACCESS_KEY-} ]]); then
+ # If the environment variables are missing check the assumed IAM role.
+ # The IAM role can be queried via http://169.254.169.254/ using instance
+ # properties.
+ # Wget will fail if the address is not present (i.e. the script is not running on
+ # an EC2 VM) or the IAM role cannot be retrieved.
+ # Set short timeouts so the script is not blocked if run outside of EC2.
+
+ WGET_ARGS=(-T 1)
+ WGET_ARGS+=(-t 1)
+ WGET_ARGS+=(-q)
+ WGET_ARGS+=(-o /dev/null)
+ WGET_ARGS+=(http://${AWS_METADATA_IP_ADDRESS}/latest/meta-data/iam/security-credentials/)
+
+ if ! wget "${WGET_ARGS[@]}" ; then
+ echo \
+"Error: missing valid S3 credentials.
+You wanted to access an S3 bucket but you did not supply valid credentials.
+The AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment variables
+have to be assigned valid credentials that belong to the owner of the
+specified S3 bucket, or an IAM role authorized to access the S3 bucket
+has to be assigned to the VM instance if this is run inside an EC2 VM."
+ exit 1
+ fi
+fi
+
+if [ ! -x "$(command -v aws)" ] ; then
+ echo "Error: AWS CLI not found, unable to check S3 access."
+ exit 2
+fi
+
+aws s3 ls "s3://${S3_BUCKET}/" 1>/dev/null
+if [ $? != 0 ]; then
+ echo "Error: accessing S3_BUCKET '${S3_BUCKET}' failed."
+ exit 1
+else
+ exit 0
+fi
+
http://git-wip-us.apache.org/repos/asf/impala/blob/e81b7c6b/bin/impala-config.sh
----------------------------------------------------------------------
diff --git a/bin/impala-config.sh b/bin/impala-config.sh
index 2baac96..958e62e 100755
--- a/bin/impala-config.sh
+++ b/bin/impala-config.sh
@@ -285,8 +285,6 @@ export IMPALA_LZO="${IMPALA_LZO-$IMPALA_HOME/../Impala-lzo}"
export IMPALA_AUX_TEST_HOME="${IMPALA_AUX_TEST_HOME-$IMPALA_HOME/../Impala-auxiliary-tests}"
export TARGET_FILESYSTEM="${TARGET_FILESYSTEM-hdfs}"
export FILESYSTEM_PREFIX="${FILESYSTEM_PREFIX-}"
-export AWS_SECRET_ACCESS_KEY="${AWS_SECRET_ACCESS_KEY-DummySecretAccessKey}"
-export AWS_ACCESS_KEY_ID="${AWS_ACCESS_KEY_ID-DummyAccessKeyId}"
export S3_BUCKET="${S3_BUCKET-}"
export azure_tenant_id="${azure_tenant_id-DummyAdlsTenantId}"
export azure_client_id="${azure_client_id-DummyAdlsClientId}"
@@ -299,27 +297,64 @@ export WAREHOUSE_LOCATION_PREFIX="${WAREHOUSE_LOCATION_PREFIX-}"
export LOCAL_FS="file:${WAREHOUSE_LOCATION_PREFIX}"
export METASTORE_DB="hive_impala"
-if [ "${TARGET_FILESYSTEM}" = "s3" ]; then
- # Basic error checking
- if [[ "${AWS_ACCESS_KEY_ID}" = "DummyAccessKeyId" ||\
- "${AWS_SECRET_ACCESS_KEY}" = "DummySecretAccessKey" ]]; then
- echo "Both AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY
- need to be assigned valid values and belong to the owner of the s3
- bucket in order to access the file system"
- return 1
+# Environment variables carrying AWS security credentials are prepared
+# according to the following rules:
+#
+# Instance: Running outside EC2 || Running in EC2 |
+# --------------------+--------+--------++--------+--------+
+# TARGET_FILESYSTEM | S3 | not S3 || S3 | not S3 |
+# --------------------+--------+--------++--------+--------+
+# | | || | |
+# empty | unset | dummy || unset | unset |
+# AWS_* | | || | |
+# env --------------+--------+--------++--------+--------+
+# var | | || | |
+# not empty | export | export || export | export |
+# | | || | |
+# --------------------+--------+--------++--------+--------+
+#
+# Legend: unset: the variable is unset
+# export: the variable is exported with its current value
+# dummy: the variable is set to a constant dummy value and exported
+#
+# Running on an EC2 VM is indicated by setting RUNNING_IN_EC2 to "true" and
+# exporting it from an script running before this one.
+
+# Checks are performed in a subshell to avoid leaking secrets to log files.
+if (set +x; [[ -n ${AWS_ACCESS_KEY_ID-} ]]); then
+ export AWS_ACCESS_KEY_ID
+else
+ if [[ "${TARGET_FILESYSTEM}" == "s3" || "${RUNNING_IN_EC2:-false}" == "true" ]]; then
+ unset AWS_ACCESS_KEY_ID
+ else
+ export AWS_ACCESS_KEY_ID=DummyAccessKeyId
fi
- # Check if the s3 bucket is NULL.
- if [[ "${S3_BUCKET}" = "" ]]; then
- echo "S3_BUCKET cannot be an empty string for s3"
- return 1
+fi
+
+if (set +x; [[ -n ${AWS_SECRET_ACCESS_KEY-} ]]); then
+ export AWS_SECRET_ACCESS_KEY
+else
+ if [[ "${TARGET_FILESYSTEM}" == "s3" || "${RUNNING_IN_EC2:-false}" == "true" ]]; then
+ unset AWS_SECRET_ACCESS_KEY
+ else
+ export AWS_SECRET_ACCESS_KEY=DummySecretAccessKey
fi
- aws s3 ls "s3://${S3_BUCKET}/" 1>/dev/null
- if [ $? != 0 ]; then
- echo "Access to ${S3_BUCKET} failed."
+fi
+
+# AWS_SESSION_TOKEN is not set to a dummy value, it is not needed by the FE tests
+if (set +x; [[ -n ${AWS_SESSION_TOKEN-} ]]); then
+ export AWS_SESSION_TOKEN
+else
+ unset AWS_SESSION_TOKEN
+fi
+
+if [ "${TARGET_FILESYSTEM}" = "s3" ]; then
+ if ${IMPALA_HOME}/bin/check-s3-access.sh; then
+ export DEFAULT_FS="s3a://${S3_BUCKET}"
+ export FILESYSTEM_PREFIX="${DEFAULT_FS}"
+ else
return 1
fi
- DEFAULT_FS="s3a://${S3_BUCKET}"
- export DEFAULT_FS
elif [ "${TARGET_FILESYSTEM}" = "adls" ]; then
# Basic error checking
if [[ "${azure_client_id}" = "DummyAdlsClientId" ||\
http://git-wip-us.apache.org/repos/asf/impala/blob/e81b7c6b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
index 6f6942a..09f5a73 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
@@ -246,7 +246,7 @@ public class AnalyzeDDLTest extends FrontendTestBase {
"'hdfs://localhost:20500/test-warehouse/alltypes/year=2010/month=10'");
AnalyzesOk("alter table functional.alltypes add " +
"partition(year=2050, month=10) location " +
- "'s3n://bucket/test-warehouse/alltypes/year=2010/month=10'");
+ "'s3a://bucket/test-warehouse/alltypes/year=2010/month=10'");
AnalyzesOk("alter table functional.alltypes add " +
"partition(year=2050, month=10) location " +
"'file:///test-warehouse/alltypes/year=2010/month=10'");
@@ -628,7 +628,7 @@ public class AnalyzeDDLTest extends FrontendTestBase {
AnalyzesOk("alter table functional.alltypes set location " +
"'hdfs://localhost:20500/test-warehouse/a/b'");
AnalyzesOk("alter table functional.alltypes set location " +
- "'s3n://bucket/test-warehouse/a/b'");
+ "'s3a://bucket/test-warehouse/a/b'");
AnalyzesOk("alter table functional.alltypes set location " +
"'file:///test-warehouse/a/b'");
@@ -1391,7 +1391,7 @@ public class AnalyzeDDLTest extends FrontendTestBase {
"CLASS 'com.bar.Foo' API_VERSION 'V1'");
AnalyzesOk("CREATE DATA SOURCE foo LOCATION 'hdfs://localhost:20500/a/b/foo.jar' " +
"CLASS 'com.bar.Foo' API_VERSION 'V1'");
- AnalyzesOk("CREATE DATA SOURCE foo LOCATION 's3n://bucket/a/b/foo.jar' " +
+ AnalyzesOk("CREATE DATA SOURCE foo LOCATION 's3a://bucket/a/b/foo.jar' " +
"CLASS 'com.bar.Foo' API_VERSION 'V1'");
AnalysisError("CREATE DATA SOURCE foo LOCATION 'blah://localhost:20500/foo.jar' " +
@@ -1418,7 +1418,7 @@ public class AnalyzeDDLTest extends FrontendTestBase {
AnalyzesOk("create database new_db location " +
"'hdfs://localhost:50200/test-warehouse/new_db'");
AnalyzesOk("create database new_db location " +
- "'s3n://bucket/test-warehouse/new_db'");
+ "'s3a://bucket/test-warehouse/new_db'");
// Invalid URI.
AnalysisError("create database new_db location " +
"'blah://bucket/test-warehouse/new_db'",
@@ -1673,7 +1673,7 @@ public class AnalyzeDDLTest extends FrontendTestBase {
AnalyzesOk("create table tbl like functional.alltypes location " +
"'file:/test-warehouse/new_table'");
AnalyzesOk("create table tbl like functional.alltypes location " +
- "'s3n://bucket/test-warehouse/new_table'");
+ "'s3a://bucket/test-warehouse/new_table'");
// Invalid URI values.
AnalysisError("create table tbl like functional.alltypes location " +
"'foofs://test-warehouse/new_table'",
@@ -1895,7 +1895,7 @@ public class AnalyzeDDLTest extends FrontendTestBase {
AnalyzesOk("create table tbl (i int) location " +
"'file:///test-warehouse/new_table'");
AnalyzesOk("create table tbl (i int) location " +
- "'s3n://bucket/test-warehouse/new_table'");
+ "'s3a://bucket/test-warehouse/new_table'");
AnalyzesOk("ALTER TABLE functional_seq_snap.alltypes SET LOCATION " +
"'file://test-warehouse/new_table'");
http://git-wip-us.apache.org/repos/asf/impala/blob/e81b7c6b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
----------------------------------------------------------------------
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 87d0761..54aa098 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
@@ -2997,10 +2997,6 @@ public class AnalyzeStmtsTest extends AnalyzerTest {
"tpch.lineitem", "file:///test-warehouse/test.out", overwrite),
"INPATH location 'file:/test-warehouse/test.out' must point to an " +
"HDFS, S3A or ADL filesystem.");
- AnalysisError(String.format("load data inpath '%s' %s into table " +
- "tpch.lineitem", "s3n://bucket/test-warehouse/test.out", overwrite),
- "INPATH location 's3n://bucket/test-warehouse/test.out' must point to an " +
- "HDFS, S3A or ADL filesystem.");
// File type / table type mismatch.
AnalyzesOk(String.format("load data inpath '%s' %s into table " +
http://git-wip-us.apache.org/repos/asf/impala/blob/e81b7c6b/testdata/cluster/node_templates/common/etc/hadoop/conf/core-site.xml.tmpl
----------------------------------------------------------------------
diff --git a/testdata/cluster/node_templates/common/etc/hadoop/conf/core-site.xml.tmpl b/testdata/cluster/node_templates/common/etc/hadoop/conf/core-site.xml.tmpl
index 9ff4ee7..1f53e97 100644
--- a/testdata/cluster/node_templates/common/etc/hadoop/conf/core-site.xml.tmpl
+++ b/testdata/cluster/node_templates/common/etc/hadoop/conf/core-site.xml.tmpl
@@ -56,29 +56,7 @@ DEFAULT</value>
<value>true</value>
</property>
- <!-- Needed by frontend tests that reference s3n paths. -->
- <property>
- <name>fs.s3n.awsAccessKeyId</name>
- <value>${AWS_ACCESS_KEY_ID}</value>
- </property>
-
- <property>
- <name>fs.s3n.awsSecretAccessKey</name>
- <value>${AWS_SECRET_ACCESS_KEY}</value>
- </property>
-
- <!-- Needed when the TARGET_FILESYSTEM is s3 -->
- <property>
- <name>fs.s3a.awsAccessKeyId</name>
- <value>${AWS_ACCESS_KEY_ID}</value>
- </property>
-
- <property>
- <name>fs.s3a.awsSecretAccessKey</name>
- <value>${AWS_SECRET_ACCESS_KEY}</value>
- </property>
-
- <property>
+ <property>
<name>fs.s3a.connection.maximum</name>
<value>1500</value>
</property>
[3/4] impala git commit: IMPALA-6291: disable AVX512 codegen in LLVM
Posted by ta...@apache.org.
IMPALA-6291: disable AVX512 codegen in LLVM
Adds a whitelist of LLVM CPU attributes that I know that we routinely
test Impala with. This excludes the problematic AVX512 attributes as
well as some other flags we don't test with - e.g. AMD-only
instructions, NVM-related instructions, etc. We're unlikely to get
significant benefit from these instruction set extensions without
explicitly using them via instrinsics.
Testing:
Ran core tests on a system with AVX512 support with a prototype patch
that disabled only the AVX512 flags. Added a backend test to make sure
that the whitelisting is working as expected.
Change-Id: Ic7c3ee3e370bafc50d855113485b7e6925f7bf6a
Reviewed-on: http://gerrit.cloudera.org:8080/8802
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/eeb34b65
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/eeb34b65
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/eeb34b65
Branch: refs/heads/master
Commit: eeb34b657105e87bb7ee82a4724bfc6688d54cce
Parents: e81b7c6
Author: Tim Armstrong <ta...@cloudera.com>
Authored: Fri Dec 8 09:16:25 2017 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Sat Dec 9 03:45:01 2017 +0000
----------------------------------------------------------------------
be/src/codegen/llvm-codegen-test.cc | 20 +++++++++++++++
be/src/codegen/llvm-codegen.cc | 44 ++++++++++++++++++++++++++++++--
be/src/codegen/llvm-codegen.h | 9 +++++++
3 files changed, 71 insertions(+), 2 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/eeb34b65/be/src/codegen/llvm-codegen-test.cc
----------------------------------------------------------------------
diff --git a/be/src/codegen/llvm-codegen-test.cc b/be/src/codegen/llvm-codegen-test.cc
index af6245a..cdd2b89 100644
--- a/be/src/codegen/llvm-codegen-test.cc
+++ b/be/src/codegen/llvm-codegen-test.cc
@@ -485,6 +485,26 @@ TEST_F(LlvmCodeGenTest, HandleLinkageError) {
codegen->Close();
}
+// Test that the default whitelisting disables the expected attributes.
+TEST_F(LlvmCodeGenTest, CpuAttrWhitelist) {
+ // Non-existent attributes should be disabled regardless of initial states.
+ // Whitelisted attributes like sse2 and lzcnt should retain their initial
+ // state.
+ EXPECT_EQ(vector<string>(
+ {"-dummy1", "-dummy2", "-dummy3", "-dummy4", "+sse2", "-lzcnt"}),
+ LlvmCodeGen::ApplyCpuAttrWhitelist(
+ {"+dummy1", "+dummy2", "-dummy3", "+dummy4", "+sse2", "-lzcnt"}));
+
+ // IMPALA-6291: Test that all AVX512 attributes are disabled.
+ vector<string> avx512_attrs;
+ EXPECT_EQ(vector<string>(
+ {"-avx512ifma", "-avx512dqavx512er", "-avx512f", "-avx512bw", "-avx512vl",
+ "-avx512cd", "-avx512vbmi", "-avx512pf"}),
+ LlvmCodeGen::ApplyCpuAttrWhitelist(
+ {"+avx512ifma", "+avx512dqavx512er", "+avx512f", "+avx512bw", "+avx512vl",
+ "+avx512cd", "+avx512vbmi", "+avx512pf"}));
+}
+
}
int main(int argc, char **argv) {
http://git-wip-us.apache.org/repos/asf/impala/blob/eeb34b65/be/src/codegen/llvm-codegen.cc
----------------------------------------------------------------------
diff --git a/be/src/codegen/llvm-codegen.cc b/be/src/codegen/llvm-codegen.cc
index 0dcbd1c..ca78bbb 100644
--- a/be/src/codegen/llvm-codegen.cc
+++ b/be/src/codegen/llvm-codegen.cc
@@ -101,6 +101,19 @@ DEFINE_string(opt_module_dir, "",
DEFINE_string(asm_module_dir, "",
"if set, saves disassembly for generated IR modules to the specified directory.");
DECLARE_string(local_library_dir);
+// IMPALA-6291: AVX-512 and other CPU attrs the community doesn't routinely test are
+// disabled. AVX-512 is affected by known bugs in LLVM 3.9.1. The following attrs that
+// exist in LLVM 3.9.1 are disabled: avx512bw,avx512cd,avx512dq,avx512er,avx512f,
+// avx512ifma,avx512pf,avx512vbmi,avx512vl,clflushopt,clwb,fma4,mwaitx.1.2,pcommit,pku,
+// prefetchwt1,sgx,sha,sse4a,tbm,xop,xsavec,xsaves. If new attrs are added to LLVM,
+// they will be disabled until added to this whitelist.
+DEFINE_string_hidden(llvm_cpu_attr_whitelist, "adx,aes,avx,avx2,bmi,bmi2,cmov,cx16,f16c,"
+ "fma,fsgsbase,hle,invpcid,lzcnt,mmx,movbe,pclmul,popcnt,prfchw,rdrnd,rdseed,rtm,smap,"
+ "sse,sse2,sse3,sse4.1,sse4.2,ssse3,xsave,xsaveopt",
+ "(Experimental) a comma-separated list of LLVM CPU attribute flags that are enabled "
+ "for runtime code generation. The default flags are a known-good set that are "
+ "routinely tested. This flag is provided to enable additional LLVM CPU attribute "
+ "flags for testing.");
namespace impala {
@@ -138,8 +151,10 @@ Status LlvmCodeGen::InitializeLlvm(bool load_backend) {
cpu_name_ = llvm::sys::getHostCPUName().str();
LOG(INFO) << "CPU class for runtime code generation: " << cpu_name_;
GetHostCPUAttrs(&cpu_attrs_);
- target_features_attr_ = boost::algorithm::join(cpu_attrs_, ",");
- LOG(INFO) << "CPU flags for runtime code generation: " << target_features_attr_;
+ LOG(INFO) << "Detected CPU flags: " << boost::join(cpu_attrs_, ",");
+ cpu_attrs_ = ApplyCpuAttrWhitelist(cpu_attrs_);
+ target_features_attr_ = boost::join(cpu_attrs_, ",");
+ LOG(INFO) << "CPU flags enabled for runtime code generation: " << target_features_attr_;
// Write an empty map file for perf to find.
if (FLAGS_perf_map) CodegenSymbolEmitter::WritePerfMap();
@@ -1637,6 +1652,31 @@ llvm::Constant* LlvmCodeGen::ConstantsToGVArrayPtr(llvm::Type* element_type,
return ConstantToGVPtr(array_type, array_const, name);
}
+vector<string> LlvmCodeGen::ApplyCpuAttrWhitelist(const vector<string>& cpu_attrs) {
+ vector<string> result;
+ vector<string> attr_whitelist;
+ boost::split(attr_whitelist, FLAGS_llvm_cpu_attr_whitelist, boost::is_any_of(","));
+ for (const string& attr : cpu_attrs) {
+ DCHECK_GE(attr.size(), 1);
+ DCHECK(attr[0] == '-' || attr[0] == '+') << attr;
+ if (attr[0] == '-') {
+ // Already disabled - copy it over unmodified.
+ result.push_back(attr);
+ continue;
+ }
+ const string attr_name = attr.substr(1);
+ auto it = std::find(attr_whitelist.begin(), attr_whitelist.end(), attr_name);
+ if (it != attr_whitelist.end()) {
+ // In whitelist - copy it over unmodified.
+ result.push_back(attr);
+ } else {
+ // Not in whitelist - disable it.
+ result.push_back("-" + attr_name);
+ }
+ }
+ return result;
+}
+
void LlvmCodeGen::DiagnosticHandler::DiagnosticHandlerFn(
const llvm::DiagnosticInfo& info, void* context) {
if (info.getSeverity() == llvm::DiagnosticSeverity::DS_Error) {
http://git-wip-us.apache.org/repos/asf/impala/blob/eeb34b65/be/src/codegen/llvm-codegen.h
----------------------------------------------------------------------
diff --git a/be/src/codegen/llvm-codegen.h b/be/src/codegen/llvm-codegen.h
index bbfda3e..672ed27 100644
--- a/be/src/codegen/llvm-codegen.h
+++ b/be/src/codegen/llvm-codegen.h
@@ -540,6 +540,7 @@ class LlvmCodeGen {
private:
friend class ExprCodegenTest;
friend class LlvmCodeGenTest;
+ friend class LlvmCodeGenTest_CpuAttrWhitelist_Test;
friend class SubExprElimination;
/// Top level codegen object. 'module_id' is used for debugging when outputting the IR.
@@ -659,6 +660,14 @@ class LlvmCodeGen {
/// generated is retained by the execution engine.
void DestroyModule();
+ /// Disable CPU attributes in 'cpu_attrs' that are not present in
+ /// the '--llvm_cpu_attr_whitelist' flag. The same attributes in the input are
+ /// always present in the output, except "+" is flipped to "-" for the disabled
+ /// attributes. E.g. if 'cpu_attrs' is {"+x", "+y", "-z"} and the whitelist is
+ /// {"x", "z"}, returns {"+x", "-y", "-z"}.
+ static std::vector<std::string> ApplyCpuAttrWhitelist(
+ const std::vector<std::string>& cpu_attrs);
+
/// Whether InitializeLlvm() has been called.
static bool llvm_initialized_;
[4/4] impala git commit: IMPALA-6242: Reduce flakiness in
TimerCounterTest
Posted by ta...@apache.org.
IMPALA-6242: Reduce flakiness in TimerCounterTest
The error threshold in TimerCounterTest is 15ms, which is still not
enough in some rare cases. This patch increases it to 30ms.
Change-Id: Ifc038908857060ccbabfe30c46e72fd93907f412
Reviewed-on: http://gerrit.cloudera.org:8080/8670
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/12fa20d1
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/12fa20d1
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/12fa20d1
Branch: refs/heads/master
Commit: 12fa20d1aea854031260e15dbf00fc4a7a29bea6
Parents: eeb34b6
Author: Tianyi Wang <tw...@cloudera.com>
Authored: Tue Nov 28 13:51:51 2017 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Sat Dec 9 03:58:23 2017 +0000
----------------------------------------------------------------------
be/src/util/runtime-profile-test.cc | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/12fa20d1/be/src/util/runtime-profile-test.cc
----------------------------------------------------------------------
diff --git a/be/src/util/runtime-profile-test.cc b/be/src/util/runtime-profile-test.cc
index 1bc7911..f85b473 100644
--- a/be/src/util/runtime-profile-test.cc
+++ b/be/src/util/runtime-profile-test.cc
@@ -185,7 +185,7 @@ TEST(CountersTest, MergeAndUpdate) {
EXPECT_EQ(3, profile->num_counters());
ValidateCounter(profile, "Child3", 30);
} else {
- EXPECT_TRUE(false);
+ FAIL();
}
}
@@ -217,7 +217,7 @@ TEST(CountersTest, MergeAndUpdate) {
EXPECT_EQ(3, profile->num_counters());
ValidateCounter(profile, "Child3", 30);
} else {
- EXPECT_TRUE(false);
+ FAIL();
}
}
@@ -627,8 +627,8 @@ class TimerCounterTest {
SleepForMs(10);
// Each test case should be no more than one second.
// Consider test failed if timer is more than 3 seconds.
- if (csw_.TotalRunningTime() > 3000000000) {
- EXPECT_FALSE(false);
+ if (csw_.TotalRunningTime() > 6000000000) {
+ FAIL();
}
}
}
@@ -660,8 +660,8 @@ class TimerCounterTest {
workers_.clear();
}
- // Allow some timer inaccuracy (15ms) since thread join could take some time.
- static const int MAX_TIMER_ERROR_NS = 15000000;
+ // Allow some timer inaccuracy (30ms) since thread join could take some time.
+ static const int MAX_TIMER_ERROR_NS = 30000000;
vector<DummyWorker> workers_;
ConcurrentStopWatch csw_;
RuntimeProfile::ConcurrentTimerCounter timercounter_;
@@ -692,7 +692,7 @@ TEST(TimerCounterTest, CountersTestOneThread) {
TimerCounterTest tester;
uint64_t start = MonotonicNanos();
tester.StartWorkers(1, 0);
- SleepForMs(250);
+ SleepForMs(500);
ValidateTimerValue(tester, start);
tester.StopWorkers(-1);
ValidateTimerValue(tester, start);
@@ -701,8 +701,8 @@ TEST(TimerCounterTest, CountersTestOneThread) {
TEST(TimerCounterTest, CountersTestTwoThreads) {
TimerCounterTest tester;
uint64_t start = MonotonicNanos();
- tester.StartWorkers(2, 5);
- SleepForMs(250);
+ tester.StartWorkers(2, 10);
+ SleepForMs(500);
ValidateTimerValue(tester, start);
tester.StopWorkers(-1);
ValidateTimerValue(tester, start);
@@ -713,15 +713,15 @@ TEST(TimerCounterTest, CountersTestRandom) {
uint64_t start = MonotonicNanos();
ValidateTimerValue(tester, start);
// First working period
- tester.StartWorkers(5, 5);
+ tester.StartWorkers(5, 10);
ValidateTimerValue(tester, start);
- SleepForMs(200);
+ SleepForMs(400);
tester.StopWorkers(2);
ValidateTimerValue(tester, start);
- SleepForMs(50);
+ SleepForMs(100);
tester.StopWorkers(4);
ValidateTimerValue(tester, start);
- SleepForMs(300);
+ SleepForMs(600);
tester.StopWorkers(-1);
ValidateTimerValue(tester, start);
tester.Reset();
@@ -729,7 +729,7 @@ TEST(TimerCounterTest, CountersTestRandom) {
ValidateLapTime(&tester, MonotonicNanos() - start);
uint64_t first_run_end = MonotonicNanos();
// Adding some idle time. concurrent stopwatch and timer should not count the idle time.
- SleepForMs(100);
+ SleepForMs(200);
start += MonotonicNanos() - first_run_end;
// Second working period
@@ -737,9 +737,9 @@ TEST(TimerCounterTest, CountersTestRandom) {
// We just get lap time after first run finish. so at start of second run, expect lap time == 0
ValidateLapTime(&tester, 0);
uint64_t lap_time_start = MonotonicNanos();
- SleepForMs(100);
+ SleepForMs(200);
ValidateTimerValue(tester, start);
- SleepForMs(100);
+ SleepForMs(200);
tester.StopWorkers(-1);
ValidateTimerValue(tester, start);
ValidateLapTime(&tester, MonotonicNanos() - lap_time_start);