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/03/03 02:01:09 UTC
[1/4] incubator-impala git commit: IMPALA-4899: Fix parquet table
writer dictionary leak
Repository: incubator-impala
Updated Branches:
refs/heads/master 545bfcfe6 -> 157da298d
IMPALA-4899: Fix parquet table writer dictionary leak
Currently, in HdfsTableSink, OutputPartitions are added to the RuntimeState
object pool to be freed at the end of the query. However, for clustered inserts
into a partitioned table, the OutputPartitions are only used one at a time.
They can be immediately freed once done writing to that partition.
In addition, the HdfsParquetTableWriter's ColumnWriters are also added to
this object pool. These constitute a significant amount of memory, as they
contain the dictionaries for Parquet encoding.
This change makes HdfsParquetTableWriter's ColumnWriters use unique_ptrs so
that they are cleaned up when the HdfsParquetTableWriter is deleted. It also
uses a unique_ptr on the PartitionPair for the OutputPartition.
The table writers maintain a pointer to the OutputPartition. This remains a
raw pointer. This is safe, because OutputPartition has a scoped_ptr to the
table writer. The table writer will never outlive the OutputPartition.
Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Reviewed-on: http://gerrit.cloudera.org:8080/6181
Reviewed-by: Marcel Kornacker <ma...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/642b8f1b
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/642b8f1b
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/642b8f1b
Branch: refs/heads/master
Commit: 642b8f1b5d5493dc9e3aa55a973ef92094d4dbc9
Parents: 545bfcf
Author: Joe McDonnell <jo...@cloudera.com>
Authored: Mon Feb 27 16:13:38 2017 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Fri Mar 3 01:20:04 2017 +0000
----------------------------------------------------------------------
be/src/exec/hdfs-parquet-table-writer.cc | 2 +-
be/src/exec/hdfs-parquet-table-writer.h | 6 ++++--
be/src/exec/hdfs-table-sink.cc | 18 ++++++++++--------
be/src/exec/hdfs-table-sink.h | 11 ++++++++---
be/src/exec/hdfs-table-writer.h | 3 +++
5 files changed, 26 insertions(+), 14 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/642b8f1b/be/src/exec/hdfs-parquet-table-writer.cc
----------------------------------------------------------------------
diff --git a/be/src/exec/hdfs-parquet-table-writer.cc b/be/src/exec/hdfs-parquet-table-writer.cc
index 6114f4e..890aa5d 100644
--- a/be/src/exec/hdfs-parquet-table-writer.cc
+++ b/be/src/exec/hdfs-parquet-table-writer.cc
@@ -799,7 +799,7 @@ Status HdfsParquetTableWriter::Init() {
default:
DCHECK(false);
}
- columns_[i] = state_->obj_pool()->Add(writer);
+ columns_[i].reset(writer);
columns_[i]->Reset();
}
RETURN_IF_ERROR(CreateSchema());
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/642b8f1b/be/src/exec/hdfs-parquet-table-writer.h
----------------------------------------------------------------------
diff --git a/be/src/exec/hdfs-parquet-table-writer.h b/be/src/exec/hdfs-parquet-table-writer.h
index 94ad932..d4fbd94 100644
--- a/be/src/exec/hdfs-parquet-table-writer.h
+++ b/be/src/exec/hdfs-parquet-table-writer.h
@@ -146,8 +146,10 @@ class HdfsParquetTableWriter : public HdfsTableWriter {
/// The current row group being written to.
parquet::RowGroup* current_row_group_;
- /// array of pointers to column information.
- std::vector<BaseColumnWriter*> columns_;
+ /// Array of pointers to column information. The column writers are owned by the
+ /// table writer, as there is no reason for the column writers to outlive the table
+ /// writer.
+ std::vector<std::unique_ptr<BaseColumnWriter>> columns_;
/// Number of rows in current file
int64_t row_count_;
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/642b8f1b/be/src/exec/hdfs-table-sink.cc
----------------------------------------------------------------------
diff --git a/be/src/exec/hdfs-table-sink.cc b/be/src/exec/hdfs-table-sink.cc
index ddda05b..8d64683 100644
--- a/be/src/exec/hdfs-table-sink.cc
+++ b/be/src/exec/hdfs-table-sink.cc
@@ -276,7 +276,7 @@ Status HdfsTableSink::WriteRowsToPartition(
// set.
bool new_file;
while (true) {
- OutputPartition* output_partition = partition_pair->first;
+ OutputPartition* output_partition = partition_pair->first.get();
RETURN_IF_ERROR(
output_partition->writer->AppendRows(batch, partition_pair->second, &new_file));
if (!new_file) break;
@@ -327,7 +327,8 @@ Status HdfsTableSink::WriteClusteredRowBatch(RuntimeState* state, RowBatch* batc
RETURN_IF_ERROR(WriteRowsToPartition(state, batch, current_clustered_partition_));
current_clustered_partition_->second.clear();
}
- RETURN_IF_ERROR(FinalizePartitionFile(state, current_clustered_partition_->first));
+ RETURN_IF_ERROR(FinalizePartitionFile(state,
+ current_clustered_partition_->first.get()));
if (current_clustered_partition_->first->writer.get() != nullptr) {
current_clustered_partition_->first->writer->Close();
}
@@ -571,9 +572,10 @@ inline Status HdfsTableSink::GetOutputPartition(RuntimeState* state, const Tuple
partition_descriptor = it->second;
}
- OutputPartition* partition = state->obj_pool()->Add(new OutputPartition());
+ std::unique_ptr<OutputPartition> partition(new OutputPartition());
Status status =
- InitOutputPartition(state, *partition_descriptor, row, partition, no_more_rows);
+ InitOutputPartition(state, *partition_descriptor, row, partition.get(),
+ no_more_rows);
if (!status.ok()) {
// We failed to create the output partition successfully. Clean it up now
// as it is not added to partition_keys_to_output_partitions_ so won't be
@@ -594,12 +596,12 @@ inline Status HdfsTableSink::GetOutputPartition(RuntimeState* state, const Tuple
state->per_partition_status()->insert(
make_pair(partition->partition_name, partition_status));
- if (!no_more_rows && !ShouldSkipStaging(state, partition)) {
+ if (!no_more_rows && !ShouldSkipStaging(state, partition.get())) {
// Indicate that temporary directory is to be deleted after execution
(*state->hdfs_files_to_move())[partition->tmp_hdfs_dir_name] = "";
}
- partition_keys_to_output_partitions_[key].first = partition;
+ partition_keys_to_output_partitions_[key].first = std::move(partition);
*partition_pair = &partition_keys_to_output_partitions_[key];
} else {
// Use existing output_partition partition.
@@ -701,7 +703,7 @@ Status HdfsTableSink::FlushFinal(RuntimeState* state) {
partition_keys_to_output_partitions_.begin();
cur_partition != partition_keys_to_output_partitions_.end();
++cur_partition) {
- RETURN_IF_ERROR(FinalizePartitionFile(state, cur_partition->second.first));
+ RETURN_IF_ERROR(FinalizePartitionFile(state, cur_partition->second.first.get()));
}
return Status::OK();
@@ -717,7 +719,7 @@ void HdfsTableSink::Close(RuntimeState* state) {
if (cur_partition->second.first->writer.get() != nullptr) {
cur_partition->second.first->writer->Close();
}
- Status close_status = ClosePartitionFile(state, cur_partition->second.first);
+ Status close_status = ClosePartitionFile(state, cur_partition->second.first.get());
if (!close_status.ok()) state->LogError(close_status.msg());
}
partition_keys_to_output_partitions_.clear();
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/642b8f1b/be/src/exec/hdfs-table-sink.h
----------------------------------------------------------------------
diff --git a/be/src/exec/hdfs-table-sink.h b/be/src/exec/hdfs-table-sink.h
index 22b6a44..65c1798 100644
--- a/be/src/exec/hdfs-table-sink.h
+++ b/be/src/exec/hdfs-table-sink.h
@@ -179,9 +179,14 @@ class HdfsTableSink : public DataSink {
Status CreateNewTmpFile(RuntimeState* state, OutputPartition* output_partition);
/// Key is the concatenation of the evaluated dynamic_partition_key_exprs_ generated by
- /// GetHashTblKey(). Maps to an OutputPartition, which are owned by the object pool, and
- /// a vector of indices of the rows in the current batch to insert into the partition.
- typedef std::pair<OutputPartition*, std::vector<int32_t>> PartitionPair;
+ /// GetHashTblKey(). Maps to an OutputPartition and a vector of indices of the rows in
+ /// the current batch to insert into the partition. The PartitionPair owns the
+ /// OutputPartition via a unique_ptr so that the memory is freed as soon as the
+ /// PartitionPair is removed from the map. This is important, because the
+ /// PartitionPairs can have different lifetimes. For example, a clustered insert into a
+ /// partitioned table iterates over the partitions, so only one PartitionPairs is
+ /// in the map at any given time.
+ typedef std::pair<std::unique_ptr<OutputPartition>, std::vector<int32_t>> PartitionPair;
typedef boost::unordered_map<std::string, PartitionPair> PartitionMap;
/// Generates string key for hash_tbl_ as a concatenation of all evaluated exprs,
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/642b8f1b/be/src/exec/hdfs-table-writer.h
----------------------------------------------------------------------
diff --git a/be/src/exec/hdfs-table-writer.h b/be/src/exec/hdfs-table-writer.h
index 8066315..86fe56c 100644
--- a/be/src/exec/hdfs-table-writer.h
+++ b/be/src/exec/hdfs-table-writer.h
@@ -115,6 +115,9 @@ class HdfsTableWriter {
RuntimeState* state_;
/// Structure describing partition written to by this writer.
+ /// NOTE: OutputPartition is usually accessed with a unique_ptr. It is safe to use
+ /// a raw pointer here, because the OutputPartition maintains a scoped_ptr to
+ /// the HdfsTableWriter objects. This will never outlive the OutputPartition.
OutputPartition* output_;
/// Table descriptor of table to be written.
[3/4] incubator-impala git commit: IMPALA-4810: add DECIMAL test case
to strict_mode tests
Posted by ta...@apache.org.
IMPALA-4810: add DECIMAL test case to strict_mode tests
The string parsing code already errors if the decimal column either
overflows or underflows (i.e. loses scale). Let's just add a test
case.
Change-Id: Idd66c0fb5a4d201919d39f73dea08b87339d6469
Reviewed-on: http://gerrit.cloudera.org:8080/6150
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/bf2e8972
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/bf2e8972
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/bf2e8972
Branch: refs/heads/master
Commit: bf2e897209f1672073380665f84a4209be04b349
Parents: 3a2a380
Author: Dan Hecht <dh...@cloudera.com>
Authored: Fri Feb 24 14:54:32 2017 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Fri Mar 3 01:43:42 2017 +0000
----------------------------------------------------------------------
testdata/data/overflow.txt | 6 +++---
.../functional/functional_schema_template.sql | 3 +++
.../functional-query/queries/QueryTest/overflow.test | 6 ++++--
.../queries/QueryTest/strict-mode-abort.test | 15 +++++++++++++++
.../queries/QueryTest/strict-mode.test | 14 ++++++++++----
5 files changed, 35 insertions(+), 9 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bf2e8972/testdata/data/overflow.txt
----------------------------------------------------------------------
diff --git a/testdata/data/overflow.txt b/testdata/data/overflow.txt
index ded5d49..5186f2c 100644
--- a/testdata/data/overflow.txt
+++ b/testdata/data/overflow.txt
@@ -1,3 +1,3 @@
-1,2,3,4,5.5,6.6
-1000,100000,10000000000000000,10000000000000000000,1e1000000,1e10000
--1000,-100000,-10000000000000000,-10000000000000000000,-1e1000000,-1e10000
+1,2,3,4,5.5,6.6,123456789.1234,99999999999999999999999999999999999999,0.00000000000000000000000000000000000001
+1000,100000,10000000000000000,10000000000000000000,1e1000000,1e10000,1234567890.1234,100000000000000000000000000000000000000,0.000000000000000000000000000000000000009
+-1000,-100000,-10000000000000000,-10000000000000000000,-1e1000000,-1e10000,-123456789.12341,-100000000000000000000000000000000000000,-0.000000000000000000000000000000000000009
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bf2e8972/testdata/datasets/functional/functional_schema_template.sql
----------------------------------------------------------------------
diff --git a/testdata/datasets/functional/functional_schema_template.sql b/testdata/datasets/functional/functional_schema_template.sql
index 97b40f4..1aacf43 100644
--- a/testdata/datasets/functional/functional_schema_template.sql
+++ b/testdata/datasets/functional/functional_schema_template.sql
@@ -1078,6 +1078,9 @@ int_col int
bigint_col bigint
float_col float
double_col double
+decimal0_col DECIMAL(13,4)
+decimal1_col DECIMAL(38,0)
+decimal2_col DECIMAL(38,38)
---- ROW_FORMAT
delimited fields terminated by ',' escaped by '\\'
---- LOAD
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bf2e8972/testdata/workloads/functional-query/queries/QueryTest/overflow.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/overflow.test b/testdata/workloads/functional-query/queries/QueryTest/overflow.test
index 075a4f6..0ce1157 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/overflow.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/overflow.test
@@ -1,7 +1,9 @@
====
---- QUERY
-# Tests overflow
-select * from Overflow
+# Tests overflow. Note that decimal columns produce error on overflow
+# even without --strict_mode, so we don't select the decimal columns in
+# this test case.
+select tinyint_col, smallint_col, int_col, bigint_col, float_col, double_col from Overflow
---- TYPES
tinyint, smallint, int, bigint, float, double
---- RESULTS
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bf2e8972/testdata/workloads/functional-query/queries/QueryTest/strict-mode-abort.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/strict-mode-abort.test b/testdata/workloads/functional-query/queries/QueryTest/strict-mode-abort.test
index 808346f..eb5e82e 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/strict-mode-abort.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/strict-mode-abort.test
@@ -29,3 +29,18 @@ select double_col from overflow
---- CATCH
Error converting column: 5 to DOUBLE
====
+---- QUERY
+select decimal0_col from overflow
+---- CATCH
+Error converting column: 6 to DECIMAL
+====
+---- QUERY
+select decimal1_col from overflow
+---- CATCH
+Error converting column: 7 to DECIMAL
+====
+---- QUERY
+select decimal2_col from overflow
+---- CATCH
+Error converting column: 8 to DECIMAL
+====
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bf2e8972/testdata/workloads/functional-query/queries/QueryTest/strict-mode.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/strict-mode.test b/testdata/workloads/functional-query/queries/QueryTest/strict-mode.test
index b70c272..27f6fc3 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/strict-mode.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/strict-mode.test
@@ -8,6 +8,9 @@ Error converting column: 2 to INT
Error converting column: 3 to BIGINT
Error converting column: 4 to FLOAT
Error converting column: 5 to DOUBLE
+Error converting column: 6 to DECIMAL(13,4)
+Error converting column: 7 to DECIMAL(38,0)
+Error converting column: 8 to DECIMAL(38,38)
row_regex: .*Error parsing row: file: $NAMENODE/.* before offset: \d+
Error converting column: 0 to TINYINT
Error converting column: 1 to SMALLINT
@@ -15,11 +18,14 @@ Error converting column: 2 to INT
Error converting column: 3 to BIGINT
Error converting column: 4 to FLOAT
Error converting column: 5 to DOUBLE
+Error converting column: 6 to DECIMAL(13,4)
+Error converting column: 7 to DECIMAL(38,0)
+Error converting column: 8 to DECIMAL(38,38)
row_regex: .*Error parsing row: file: $NAMENODE/.* before offset: \d+
---- RESULTS
-1,2,3,4,5.5,6.6
-NULL,NULL,NULL,NULL,NULL,NULL
-NULL,NULL,NULL,NULL,NULL,NULL
+1,2,3,4,5.5,6.6,123456789.1234,99999999999999999999999999999999999999,0.00000000000000000000000000000000000001
+NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL
+NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL
---- TYPES
-tinyint, smallint, int, bigint, float, double
+tinyint, smallint, int, bigint, float, double, decimal, decimal, decimal
====
[2/4] incubator-impala git commit: IMPALA-4616: Add missing Kudu
column options
Posted by ta...@apache.org.
IMPALA-4616: Add missing Kudu column options
Adds support for missing Kudu column options in ALTER TABLE
(it was there in CREATE TABLE already):
* encoding
* compression
* block_size
Also adds support for adding nullable columns with default
values.
All of the above was not originally implemented due to
limitations in the Kudu client, but have since been fixed:
KUDU-1746, KUDU-1747
Testing: Updates and adds relevant test cases.
Change-Id: I96a0fce7f6bc0c086b259d3119daa72c94b0af7b
Reviewed-on: http://gerrit.cloudera.org:8080/6220
Reviewed-by: Marcel Kornacker <ma...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/3a2a380c
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/3a2a380c
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/3a2a380c
Branch: refs/heads/master
Commit: 3a2a380cf73c55e55f4025c008c50accda056f85
Parents: 642b8f1
Author: Matthew Jacobs <mj...@cloudera.com>
Authored: Wed Mar 1 15:58:49 2017 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Fri Mar 3 01:29:14 2017 +0000
----------------------------------------------------------------------
.../analysis/AlterTableAddReplaceColsStmt.java | 9 +--
.../org/apache/impala/analysis/ColumnDef.java | 10 ++-
.../org/apache/impala/analysis/TableDef.java | 2 +-
.../impala/service/KuduCatalogOpExecutor.java | 84 ++++++++------------
.../apache/impala/analysis/AnalyzeDDLTest.java | 17 ++--
.../queries/QueryTest/kudu_alter.test | 56 ++++++-------
6 files changed, 78 insertions(+), 100 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3a2a380c/fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java b/fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java
index 9b2d7c0..598b47f 100644
--- a/fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java
+++ b/fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java
@@ -120,12 +120,9 @@ public class AlterTableAddReplaceColsStmt extends AlterTableStmt {
throw new AnalysisException("Cannot add a primary key using an ALTER TABLE " +
"ADD COLUMNS statement: " + c.toString());
}
- if (c.hasEncoding() || c.hasCompression() || c.hasBlockSize()) {
- // Kudu API doesn't support specifying encoding, compression and desired
- // block size on a newly added column (see KUDU-1746).
- throw new AnalysisException("ENCODING, COMPRESSION and " +
- "BLOCK_SIZE options cannot be specified in an ALTER TABLE ADD COLUMNS " +
- "statement: " + c.toString());
+ if (c.isExplicitNotNullable() && !c.hasDefaultValue()) {
+ throw new AnalysisException("A new non-null column must have a default " +
+ "value: " + c.toString());
}
} else if (c.hasKuduOptions()) {
throw new AnalysisException("The specified column options are only supported " +
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3a2a380c/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java b/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
index d7217f7..42304e6 100644
--- a/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
+++ b/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
@@ -162,7 +162,15 @@ public class ColumnDef {
public boolean hasCompression() { return compressionVal_ != null; }
public boolean hasBlockSize() { return blockSize_ != null; }
public boolean isNullabilitySet() { return isNullable_ != null; }
- public boolean isNullable() { return isNullabilitySet() && isNullable_; }
+
+ // True if the column was explicitly set to be nullable (may differ from the default
+ // behavior if not explicitly set).
+ public boolean isExplicitNullable() { return isNullabilitySet() && isNullable_; }
+
+ // True if the column was explicitly set to be not nullable (may differ from the default
+ // behavior if not explicitly set).
+ public boolean isExplicitNotNullable() { return isNullabilitySet() && !isNullable_; }
+
public boolean hasDefaultValue() { return defaultValue_ != null; }
public void analyze(Analyzer analyzer) throws AnalysisException {
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3a2a380c/fe/src/main/java/org/apache/impala/analysis/TableDef.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/TableDef.java b/fe/src/main/java/org/apache/impala/analysis/TableDef.java
index ae314c3..cad45b7 100644
--- a/fe/src/main/java/org/apache/impala/analysis/TableDef.java
+++ b/fe/src/main/java/org/apache/impala/analysis/TableDef.java
@@ -251,7 +251,7 @@ class TableDef {
throw new AnalysisException(String.format(
"PRIMARY KEY column '%s' does not exist in the table", colName));
}
- if (colDef.isNullable()) {
+ if (colDef.isExplicitNullable()) {
throw new AnalysisException("Primary key columns cannot be nullable: " +
colDef.toString());
}
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3a2a380c/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java b/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
index 6fc5674..f1a9135 100644
--- a/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
+++ b/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
@@ -91,6 +91,36 @@ public class KuduCatalogOpExecutor {
}
}
+ private static ColumnSchema createColumnSchema(TColumn column, boolean isKey)
+ throws ImpalaRuntimeException {
+ Type type = Type.fromThrift(column.getColumnType());
+ Preconditions.checkState(type != null);
+ org.apache.kudu.Type kuduType = KuduUtil.fromImpalaType(type);
+
+ ColumnSchemaBuilder csb = new ColumnSchemaBuilder(column.getColumnName(), kuduType);
+ csb.key(isKey);
+ if (column.isSetIs_nullable()) {
+ Preconditions.checkArgument(!isKey);
+ csb.nullable(column.isIs_nullable());
+ } else {
+ // Non-key columns are by default nullable unless the user explicitly sets its
+ // nullability. Key columns are not nullable.
+ csb.nullable(!isKey);
+ }
+ if (column.isSetDefault_value()) {
+ csb.defaultValue(KuduUtil.getKuduDefaultValue(column.getDefault_value(), kuduType,
+ column.getColumnName()));
+ }
+ if (column.isSetBlock_size()) csb.desiredBlockSize(column.getBlock_size());
+ if (column.isSetEncoding()) {
+ csb.encoding(KuduUtil.fromThrift(column.getEncoding()));
+ }
+ if (column.isSetCompression()) {
+ csb.compressionAlgorithm(KuduUtil.fromThrift(column.getCompression()));
+ }
+ return csb.build();
+ }
+
/**
* Creates the schema of a new Kudu table.
*/
@@ -100,33 +130,8 @@ public class KuduCatalogOpExecutor {
Preconditions.checkState(!keyColNames.isEmpty());
List<ColumnSchema> colSchemas = new ArrayList<>(params.getColumnsSize());
for (TColumn column: params.getColumns()) {
- Type type = Type.fromThrift(column.getColumnType());
- Preconditions.checkState(type != null);
- org.apache.kudu.Type kuduType = KuduUtil.fromImpalaType(type);
- // Create the actual column and check if the column is a key column
- ColumnSchemaBuilder csb =
- new ColumnSchemaBuilder(column.getColumnName(), kuduType);
boolean isKey = keyColNames.contains(column.getColumnName());
- csb.key(isKey);
- if (column.isSetIs_nullable()) {
- csb.nullable(column.isIs_nullable());
- } else if (!isKey) {
- // Non-key columns are by default nullable unless the user explicitly sets their
- // nullability.
- csb.nullable(true);
- }
- if (column.isSetDefault_value()) {
- csb.defaultValue(KuduUtil.getKuduDefaultValue(column.getDefault_value(), kuduType,
- column.getColumnName()));
- }
- if (column.isSetBlock_size()) csb.desiredBlockSize(column.getBlock_size());
- if (column.isSetEncoding()) {
- csb.encoding(KuduUtil.fromThrift(column.getEncoding()));
- }
- if (column.isSetCompression()) {
- csb.compressionAlgorithm(KuduUtil.fromThrift(column.getCompression()));
- }
- colSchemas.add(csb.build());
+ colSchemas.add(createColumnSchema(column, isKey));
}
return new Schema(colSchemas);
}
@@ -369,32 +374,7 @@ public class KuduCatalogOpExecutor {
throws ImpalaRuntimeException {
AlterTableOptions alterTableOptions = new AlterTableOptions();
for (TColumn column: columns) {
- Type type = Type.fromThrift(column.getColumnType());
- Preconditions.checkState(type != null);
- org.apache.kudu.Type kuduType = KuduUtil.fromImpalaType(type);
- boolean isNullable = !column.isSetIs_nullable() ? true : column.isIs_nullable();
- if (isNullable) {
- if (column.isSetDefault_value()) {
- // See KUDU-1747
- throw new ImpalaRuntimeException(String.format("Error adding nullable " +
- "column to Kudu table %s. Cannot specify a default value for a nullable " +
- "column", tbl.getKuduTableName()));
- }
- alterTableOptions.addNullableColumn(column.getColumnName(), kuduType);
- } else {
- Object defaultValue = null;
- if (column.isSetDefault_value()) {
- defaultValue = KuduUtil.getKuduDefaultValue(column.getDefault_value(), kuduType,
- column.getColumnName());
- }
- try {
- alterTableOptions.addColumn(column.getColumnName(), kuduType, defaultValue);
- } catch (IllegalArgumentException e) {
- // TODO: Remove this when KUDU-1747 is fixed
- throw new ImpalaRuntimeException("Error adding non-nullable column to " +
- "Kudu table " + tbl.getKuduTableName(), e);
- }
- }
+ alterTableOptions.addColumn(createColumnSchema(column, false));
}
String errMsg = "Error adding columns to Kudu table " + tbl.getName();
alterKuduTable(tbl, alterTableOptions, errMsg);
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3a2a380c/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 c0703eb..757b857 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
@@ -1896,17 +1896,16 @@ public class AnalyzeDDLTest extends FrontendTestBase {
AnalysisError("alter table functional_kudu.testtbl add columns (a int primary key)",
"Cannot add a primary key using an ALTER TABLE ADD COLUMNS statement: " +
"a INT PRIMARY KEY");
- // Non-nullable columns require a default value
+ // Columns requiring a default value
AnalyzesOk("alter table functional_kudu.testtbl add columns (a1 int not null " +
"default 10)");
- // Unsupported column options
- String[] unsupportedColOptions = {"encoding rle", "compression lz4", "block_size 10"};
- for (String colOption: unsupportedColOptions) {
- AnalysisError(String.format("alter table functional_kudu.testtbl add columns " +
- "(a1 int %s)", colOption), String.format("ENCODING, COMPRESSION and " +
- "BLOCK_SIZE options cannot be specified in an ALTER TABLE ADD COLUMNS " +
- "statement: a1 INT %s", colOption.toUpperCase()));
- }
+ AnalyzesOk("alter table functional_kudu.testtbl add columns (a1 int null " +
+ "default 10)");
+ // Other Kudu column options
+ AnalyzesOk("alter table functional_kudu.testtbl add columns (a int encoding rle)");
+ AnalyzesOk("alter table functional_kudu.testtbl add columns (a int compression lz4)");
+ AnalyzesOk("alter table functional_kudu.testtbl add columns (a int block_size 10)");
+
// REPLACE columns is not supported for Kudu tables
AnalysisError("alter table functional_kudu.testtbl replace columns (a int null)",
"ALTER TABLE REPLACE COLUMNS is not supported on Kudu tables");
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3a2a380c/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test b/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
index f959a42..369eecf 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
@@ -258,56 +258,50 @@ ID, NAME, VALI, NEW_COL1, NEW_COL2
INT,STRING,BIGINT,INT,BIGINT
====
---- QUERY
-# Add a nullable column
-alter table tbl_to_alter add columns (new_col3 string null)
+# Add nullable columns: with and without a default
+alter table tbl_to_alter add columns (new_col3 string null, new_col4 int null default -1)
---- RESULTS
====
---- QUERY
# Add a row
-insert into tbl_to_alter values ((4, 'test', 300, 1, 100, null),
- (5, 'test', 400, 2, 200, 'names'))
+insert into tbl_to_alter values ((4, 'test', 300, 1, 100, null, null),
+ (5, 'test', 400, 2, 200, 'names', 1))
---- RUNTIME_PROFILE
NumModifiedRows: 2
NumRowErrors: 0
---- LABELS
-ID, NAME, VALI, NEW_COL1, NEW_COL2, NEW_COL3
+ID, NAME, VALI, NEW_COL1, NEW_COL2, NEW_COL3, NEW_COL4
---- DML_RESULTS: tbl_to_alter
-2,'test',100,1,100,'NULL'
-3,'test',200,10,1000,'NULL'
-4,'test',300,1,100,'NULL'
-5,'test',400,2,200,'names'
+2,'test',100,1,100,'NULL',-1
+3,'test',200,10,1000,'NULL',-1
+4,'test',300,1,100,'NULL',NULL
+5,'test',400,2,200,'names',1
---- TYPES
-INT,STRING,BIGINT,INT,BIGINT,STRING
+INT,STRING,BIGINT,INT,BIGINT,STRING,INT
====
---- QUERY
-# Add a row that doesn't have a value for the last added column
+# Add a row that doesn't have a value for the last added columns
insert into tbl_to_alter (id, name, vali, new_col1, new_col2)
values (6, 'test', 500, 3, 300)
---- RUNTIME_PROFILE
NumModifiedRows: 1
NumRowErrors: 0
---- LABELS
-ID, NAME, VALI, NEW_COL1, NEW_COL2, NEW_COL3
+ID, NAME, VALI, NEW_COL1, NEW_COL2, NEW_COL3, NEW_COL4
---- DML_RESULTS: tbl_to_alter
-2,'test',100,1,100,'NULL'
-3,'test',200,10,1000,'NULL'
-4,'test',300,1,100,'NULL'
-5,'test',400,2,200,'names'
-6,'test',500,3,300,'NULL'
+2,'test',100,1,100,'NULL',-1
+3,'test',200,10,1000,'NULL',-1
+4,'test',300,1,100,'NULL',NULL
+5,'test',400,2,200,'names',1
+6,'test',500,3,300,'NULL',-1
---- TYPES
-INT,STRING,BIGINT,INT,BIGINT,STRING
-====
----- QUERY
-# Add a nullable column with a default value
-alter table tbl_to_alter add columns (invalid_col int null default 10)
----- CATCH
-Error adding nullable column to Kudu table
+INT,STRING,BIGINT,INT,BIGINT,STRING,INT
====
---- QUERY
# Add a non-nullable column without a default value
alter table tbl_to_alter add columns (invalid_col int not null)
---- CATCH
-Error adding non-nullable column to Kudu table
+A new non-null column must have a default value
====
---- QUERY
# Drop a column
@@ -318,13 +312,13 @@ alter table tbl_to_alter drop column vali
# Retrieve table rows after column got dropped
select * from tbl_to_alter
---- RESULTS
-2,'test',1,100,'NULL'
-3,'test',10,1000,'NULL'
-4,'test',1,100,'NULL'
-5,'test',2,200,'names'
-6,'test',3,300,'NULL'
+2,'test',1,100,'NULL',-1
+3,'test',10,1000,'NULL',-1
+4,'test',1,100,'NULL',NULL
+5,'test',2,200,'names',1
+6,'test',3,300,'NULL',-1
---- TYPES
-INT,STRING,INT,BIGINT,STRING
+INT,STRING,INT,BIGINT,STRING,INT
====
---- QUERY
# Try to drop a primary key column
[4/4] incubator-impala git commit: Update parameter order in
test_mt_dop.py
Posted by ta...@apache.org.
Update parameter order in test_mt_dop.py
The rest of the code base uses them in a different order. This is purely
for consistency. Maybe it'll help prevent a bug in the future.
Change-Id: I925711eb4d4334c179d336f6ebcc91d26ce8879c
Reviewed-on: http://gerrit.cloudera.org:8080/6228
Reviewed-by: Lars Volker <lv...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/157da298
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/157da298
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/157da298
Branch: refs/heads/master
Commit: 157da298d194cc4fa298e3750d87bf102c3295bb
Parents: bf2e897
Author: Lars Volker <lv...@cloudera.com>
Authored: Thu Mar 2 13:09:38 2017 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Fri Mar 3 01:55:50 2017 +0000
----------------------------------------------------------------------
tests/query_test/test_mt_dop.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/157da298/tests/query_test/test_mt_dop.py
----------------------------------------------------------------------
diff --git a/tests/query_test/test_mt_dop.py b/tests/query_test/test_mt_dop.py
index 5dddfc0..ddef617 100644
--- a/tests/query_test/test_mt_dop.py
+++ b/tests/query_test/test_mt_dop.py
@@ -43,7 +43,7 @@ class TestMtDop(ImpalaTestSuite):
vector.get_value('exec_option')['mt_dop'] = vector.get_value('mt_dop')
self.run_test_case('QueryTest/mt-dop', vector)
- def test_compute_stats(self, unique_database, vector):
+ def test_compute_stats(self, vector, unique_database):
vector.get_value('exec_option')['mt_dop'] = vector.get_value('mt_dop')
file_format = vector.get_value('table_format').file_format
fq_table_name = "%s.mt_dop" % unique_database