[Impala-ASF-CR] IMPALA-4237: Fix materialization of 4 byte decimals in data source scan node.

[Alex Behm (Code Review)]

Alex Behm has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4585

Change subject: IMPALA-4237: Fix materialization of 4 byte decimals in data 
source scan node.
..

IMPALA-4237: Fix materialization of 4 byte decimals in data source scan node.

There was a missing break in a switch statement leading to bad fallthrough.

An existing test already expected incorrect results. The bug is covered by
expecting correct results.

Change-Id: I5340c2eda813afc032ba72203bd59eb3f2c4f482
---
M be/src/exec/data-source-scan-node.cc
M testdata/workloads/functional-query/queries/QueryTest/data-source-tables.test
2 files changed, 6 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/4585/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4585
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5340c2eda813afc032ba72203bd59eb3f2c4f482
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 

[Impala-ASF-CR] IMPALA-3504: UDF for current timestamp in UTC

[Youwei Wang (Code Review)]

Youwei Wang has posted comments on this change.

Change subject: IMPALA-3504: UDF for current timestamp in UTC
..


Patch Set 8:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/4490/6/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

PS6, Line 4120: cTime();
> forward-lapsing?
Greeting, Matthew.
As a commone sense, time never rewinds, time never goes back. Since I am not a 
native English speaker, would you please share me a more elegant and native way 
to experss this?

Thank you for any idea or hint. :)


PS6, Line 4132: .UtcToLoc
> can you name these to reference the values they're holding, e.g. unixtime_u
Done


PS6, Line 4136: EXPECT_TRUE(now
> This should be true unless there wasn't a value in the TimestampValue, whic
Done


PS6, Line 4137: EXPECT_TRUE(std
> same
Done


http://gerrit.cloudera.org:8080/#/c/4490/7/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

PS7, Line 4137: EXPECT
> missed this before, but this should be EXPECT_TRUE
Done


http://gerrit.cloudera.org:8080/#/c/4490/6/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS6, Line 843: g(TimestampValue::LocalTime().DebugString());
> I'd still vote to not bother with the conversion, but it seems fine.
As you wish, Sir. :)


PS6, Line 845: 
 :   // Creating a random_generator every time is not free, but
> I'm not sure we should check this. While this seems intuitive, there might 
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4490
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5ee6ee192aa469f77c711f27ad324696a42004d1
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Youwei Wang 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-889: Add support for an ISO-SQL compliant trim() function.

[Youwei Wang (Code Review)]

Youwei Wang has posted comments on this change.

Change subject: IMPALA-889: Add support for an ISO-SQL compliant trim() 
function.
..


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4474/3//COMMIT_MSG
Commit Message:

Line 9: Purpose: Removes all instances of one or more characters from the
> We should definitely not invent a new function name and stick to trim(). Th
Done


Line 32:   NULL. Blank argument causes parsing error.
> The 'where' and 'trim_char_set' clauses are optional, so the following are 
Done


http://gerrit.cloudera.org:8080/#/c/4474/3/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

Line 2418:   {: RESULT = c; :}
> Not clear what this rule adds. The copy+paste nature of it means an increas
Done


Line 2449: 
> My understanding is that several of the trim() clauses are optional, e.g., 
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Youwei Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4196: Cross compile bit-byte-functions

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4196: Cross compile bit-byte-functions
..


Patch Set 4: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/4557
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5a1291bfd202b500405a884e4a62f0ca2447244a
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4196: Cross compile bit-byte-functions

[Internal Jenkins (Code Review)]

Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4196: Cross compile bit-byte-functions
..


IMPALA-4196: Cross compile bit-byte-functions

Change-Id: I5a1291bfd202b500405a884e4a62f0ca2447244a
Reviewed-on: http://gerrit.cloudera.org:8080/4557
Reviewed-by: Bharath Vissapragada 
Tested-by: Internal Jenkins
---
M be/src/codegen/impala-ir.cc
M be/src/exprs/CMakeLists.txt
R be/src/exprs/bit-byte-functions-ir.cc
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
4 files changed, 10 insertions(+), 1 deletion(-)

Approvals:
  Bharath Vissapragada: Looks good to me, approved
  Internal Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/4557
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I5a1291bfd202b500405a884e4a62f0ca2447244a
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-1702: Enforce table level consistency accross service

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-1702: Enforce table level consistency accross service
..


Patch Set 3:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/4349/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

Line 294: // Caches all table used in a single query. Use the same 
reference instead of getting
"Caches all tables referenced in a single query"

The comment should explain why we have this local cache. The main motivation is 
to guarantee single table consistency within a query, i.e., that within one 
query the same version of a table is used.

Note that the catalog cache always contains the latest version of a table.


Line 2290: TTableName tblName = new TTableName(dbName, tableName);
I'm not sure if this will deal with case-insensitivity properly. Why not use a 
TableName instead? The equals() and hashCode() definitely do the right thing.


Line 2291: Table table = globalState_.referencedTables_.get(tblName);
How about this:
if (table != null) {
  // Return query-local version of table.
  Preconditions.checkState(table.isLoaded());
  return table;
}


http://gerrit.cloudera.org:8080/#/c/4349/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:

Line 141:   protected final AtomicLong nextTableId_ = new AtomicLong(0);
Add comment that explains the use and behavior of this id, i.e. the id is 
ever-increasing and only gets reset when the service is re-started. Mention at 
which points a table gets assigned a new id.


http://gerrit.cloudera.org:8080/#/c/4349/3/fe/src/main/java/org/apache/impala/catalog/Table.java
File fe/src/main/java/org/apache/impala/catalog/Table.java:

Line 65:   public static final long INVALID_TABLE_ID = -1;
Add comment for these subtly different special ids.


Line 67:   public static final long ERROR_METADATA_LOADING_ID = -3;
I think we should add LOCAL_TABLE_ID and TEST_TABLE_ID as well and describe 
their uses.


http://gerrit.cloudera.org:8080/#/c/4349/3/fe/src/main/java/org/apache/impala/common/Id.java
File fe/src/main/java/org/apache/impala/common/Id.java:

Line 42: return Long.valueOf(id_).hashCode();
revert


Line 47: return Long.toString(id_);
revert


http://gerrit.cloudera.org:8080/#/c/4349/3/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:

Line 1130
I think we should still maintain something like this to check and document our 
new guarantees.

In addition, it would be good to check our invariants in other places if 
possible. For example, when we receive an update for a table from the 
statestore, we should be able to guarantee that the currentTableId <= 
newTableId iff currentCatalogVersion < catalogVersion. Or something along those 
lines, I think you get the idea :)


Line 1134
There is similar code in a few places in the BE. We should remove that, too.


-- 
To view, visit http://gerrit.cloudera.org:8080/4349
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifad648b72684ae495ec387590ab1bc58ce5b39e2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Huaisi Xu 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Huaisi Xu 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4230: ASF policy issues from 2.7.0 rc3.

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4230: ASF policy issues from 2.7.0 rc3.
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4582/2/llvm-ir/test-loop.bc
File llvm-ir/test-loop.bc:

Did you run tests? This file is required by llvm-codegen-test.

IMO it doesn't make sense to remove binary test data. Other apache projects 
check in this kind of thing, e.g. 
https://github.com/apache/avro/tree/master/share/test/data


-- 
To view, visit http://gerrit.cloudera.org:8080/4582
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I30ff77d7ac28ce67511c200764fba19ae69922e0
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4230: ASF policy issues from 2.7.0 rc3.

[Jim Apple (Code Review)]

Jim Apple has uploaded a new patch set (#2).

Change subject: IMPALA-4230: ASF policy issues from 2.7.0 rc3.
..

IMPALA-4230: ASF policy issues from 2.7.0 rc3.

In our IPMC vote to release 2.7.0 rc3, Justing Mclean pointed out a
number of issues of compliance with ASF policy. This patch fixes:

1. Please place build instruction and supported platforms in the
README. The wiki may change over time and that may make it difficult
to build older versions.

2. Remove binary file llvm-ir/test-loop.bc

3. Add be/src/gutil/valgrind.h,
shell/ext-py/sqlparse-0.1.14/sqlparse/pipeline.py and
cmake_modules/FindJNI.cmake, normalize.css (embedded in bootstrap.css)
to LICENSE.txt

4. Fix be/src/thirdparty/squeasel/squeasel* in LICENSE.txt

5. Remove outdated copyright lines from HBase (see
https://issues.apache.org/jira/browse/HBASE-3870)

6. Remove duplicate jquery notice from LICENSE.txt

Change-Id: I30ff77d7ac28ce67511c200764fba19ae69922e0
---
M LICENSE.txt
M README.md
M be/src/codegen/CMakeLists.txt
M bin/rat_exclude_files.txt
M fe/src/test/resources/hbase-site.xml.template
D llvm-ir/test-loop.bc
M testdata/cluster/node_templates/common/etc/hadoop/conf/hadoop-policy.xml
M testdata/cluster/node_templates/common/etc/hadoop/conf/log4j.properties.tmpl
8 files changed, 169 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/82/4582/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4582
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I30ff77d7ac28ce67511c200764fba19ae69922e0
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] Don't assume that AUX exists just because a shell variable is set

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: Don't assume that AUX exists just because a shell variable is 
set
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4563/2/tests/run-custom-cluster-tests.sh
File tests/run-custom-cluster-tests.sh:

PS2, Line 40: then
: impala-py.test custom_cluster/ authorization/ 
"${AUX_CUSTOM_DIR}" \
:
--junitxml="${RESULTS_DIR}/TEST-impala-custom-cluster.xml" \
:
--resultlog="${RESULTS_DIR}/TEST-impala-custom-cluster.log" "$@"
: else
: impala-py.test custom_cluster/ authorization/ \
:
--junitxml="${RESULTS_DIR}/TEST-impala-custom-cluster.xml" \
:
--resultlog="${RESULTS_DIR}/TEST-impala-custom-cluster.log" "$@"
: fi
I don't like the code repetition, and it's not clear right away what the 
difference is between the cases. How about something like:

ARGS=("custom_cluster/ authorization/ ");
if [ -d "${AUX_CUSTOM_DIR}" ] then
  ARGS+=(${AUX_CUSTOM_DIR} )
fi
ARGS+=("--junitxml="${RESULTS_DIR}/TEST-impala-custom-cluster.xml ")
ARGS+=("--resultlog="${RESULTS_DIR}/TEST-impala-custom-cluster.log ")
ARGS+=("$@")
impala-py.test "${ARGS[@]}"


-- 
To view, visit http://gerrit.cloudera.org:8080/4563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4230: ASF policy issues from 2.7.0 rc3.

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-4230: ASF policy issues from 2.7.0 rc3.
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4582/1//COMMIT_MSG
Commit Message:

PS1, Line 12: Please
It's kind of strange that this point starts with the word "Please"


http://gerrit.cloudera.org:8080/#/c/4582/1/LICENSE.txt
File LICENSE.txt:

PS1, Line 269:  
Remove these trailing white spaces.


http://gerrit.cloudera.org:8080/#/c/4582/1/README.md
File README.md:

PS1, Line 30: right now.
at the moment


-- 
To view, visit http://gerrit.cloudera.org:8080/4582
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I30ff77d7ac28ce67511c200764fba19ae69922e0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4232: qgen: Hive does not support aggregates inside specific analytic clauses

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-4232: qgen: Hive does not support aggregates inside 
specific analytic clauses
..


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4581/2//COMMIT_MSG
Commit Message:

Line 20: 
It's still not exactly clear to me what is not allowed. Can you give an example 
query here that is allowed in Impala but not Hive?


http://gerrit.cloudera.org:8080/#/c/4581/2/tests/comparison/query_generator.py
File tests/comparison/query_generator.py:

PS2, Line 584: funcs
It's not clear what the funcs argument means. Can you rename it something more 
descriptive and add it to the function docstring?


Line 797:   child_null_arg_pools = set()
this line and the comment above it should be right above line 805


PS2, Line 803:   
this should be 4 spaces


http://gerrit.cloudera.org:8080/#/c/4581/2/tests/comparison/query_profile.py
File tests/comparison/query_profile.py:

PS2, Line 693: get_analytics_cannot_contain_aggs
how about renaming this to allow_analytics_with_aggs?


-- 
To view, visit http://gerrit.cloudera.org:8080/4581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1096c4cde7ea52a52b39e31cd93242da53b549f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sahil Takiar 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-3725 Support Kudu UPSERT in Impala
..


Patch Set 5:

The latest version moves back to using a single class, InsertStmt, rather than 
having InsertStmt and UpsertStmt, since that separation turned out to be messy. 
Instead, it separates out some of the upsert vs. insert -specific analysis into 
separate methods withing InsertStmt to make it clearer what applies when.

-- 
To view, visit http://gerrit.cloudera.org:8080/4047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables

[Dimitris Tsirogiannis (Code Review)]

Dimitris Tsirogiannis has uploaded a new patch set (#3).

Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
..

IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables

With this commit we simplify the syntax and handling of CREATE TABLE
statements for both managed and external Kudu tables.

Syntax example:
CREATE TABLE foo(a INT, b STRING, PRIMARY KEY (a, b))
DISTRIBUTE BY HASH (a) INTO 3 BUCKETS,
RANGE (b) SPLIT ROWS (('abc', 'def'))
STORED AS KUDU

Changes:
1) Remove the requirement to specify table properties such as key
   columns in tblproperties.
2) Read table schema (column definitions, primary keys, and distribution
   schemes) from Kudu instead of the HMS.
3) For external tables, the Kudu table is now required to exist at the
   time of creation in Impala.
4) Disallow table properties that could conflict with an existing
   table. Ex: key_columns cannot be specified.
5) Add KUDU as a file format.
6) Add a startup flag to impalad to specify the default Kudu master
   addresses. The flag is used as the default value for the table
   property kudu_master_addresses but it can still be overriden
   using TBLPROPERTIES.
7) Fix a post merge issue (IMPALA-3178) where DROP DATABASE CASCADE
   wasn't implemented for Kudu tables and silently ignored. The Kudu
   tables wouldn't be removed in Kudu.
8) Remove DDL delegates. There was only one functional delegate (for
   Kudu) the existence of the other delegate and the use of delegates in
   general has led to confusion. The Kudu delegate only exists to provide
   functionality missing from Hive.
9) Add PRIMARY KEY at the column and table level. This syntax is fairly
   standard. When used at the column level, only one column can be
   marked as a key. When used at the table level, multiple columns can
   be used as a key. Only Kudu tables are allowed to use PRIMARY KEY.
   The old "kudu.key_columns" table property is no longer accepted
   though it is still used internally. "PRIMARY" is now a keyword.
   "KEY" is expected to be common enough that the ident style
   declaration is used instead to avoid conflicts.
10) For managed tables, infer a Kudu table name if none was given.
   The table property "kudu.table_name" is optional for managed tables
   and is required for external tables. If for a managed table a Kudu
   table name is not provided, a table name will be generated based
   on the HMS database and table name. If the database is "default",
   then a Kudu table name will not include the database.
11) Use Kudu master as the source of truth for table metadata instead
   of HMS when a table is loaded or refreshed. Table/column metadata
   are cached in the catalog and are stored in HMS in order to be
   able to use table and column statistics.

Change-Id: I7b9d51b2720ab57649abdb7d5c710ea04ff50dc1
---
M be/src/service/frontend.cc
M bin/start-catalogd.sh
M bin/start-impala-cluster.py
M common/thrift/CatalogObjects.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/cup/sql-parser.cup
A fe/src/main/java/org/apache/impala/analysis/AnalysisUtils.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
A fe/src/main/java/org/apache/impala/analysis/ColumnDefOptions.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableDataSrcStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DistributeParam.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
A fe/src/main/java/org/apache/impala/analysis/TableDataLayout.java
A fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/HdfsFileFormat.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/TableLoader.java
D fe/src/main/java/org/apache/impala/catalog/delegates/DdlDelegate.java
D fe/src/main/java/org/apache/impala/catalog/delegates/KuduDdlDelegate.java
D 
fe/src/main/java/org/apache/impala/catalog/delegates/UnsupportedOpDelegate.java
M fe/src/main/java/org/apache/impala/planner/HdfsPartitionFilter.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A 

[Impala-ASF-CR] IMPALA-4232: qgen: Hive does not support aggregates inside specific analytic clauses

[Sahil Takiar (Code Review)]

Sahil Takiar has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4581

Change subject: IMPALA-4232: qgen: Hive does not support aggregates inside 
specific analytic clauses
..

IMPALA-4232: qgen: Hive does not support aggregates inside specific analytic 
clauses

Hive does not support aggregates inside the following analytic clauses:

* AVG ... OVER
* COUNT ... OVER
* FIRSTVALUE ... OVER
* LAG ... OVER
* LASTVALUE ... OVER
* LEAD ... OVER
* MAX ... OVER
* MIN ... OVER
* SUM ... OVER

This patch modifies the qgen code so that it doesn't create aggregates inside 
the above
analytic clauses, if the HiveProfile is used. A new method called
get_analytics_cannot_contain_aggs() is added to the DefaultProfile and to the
HiveProfile. The implementation in the DefaultProfile returns an empty list. The
implementation in the HiveProfile returns the list of methods above. The
QueryGenerator._create_agg_or_analytic_tree() method is modified so that it 
checks the
get_analytics_cannot_contain_aggs method when populating the possible function 
types of
the next child in the function tree. If it finds any of these functions already 
exist in
the tree, it ensures that the next child function cannot be an aggregate 
function.

Misc Changes:

A few miscellaneous changes were made that popped up during testing:

* Fixed a possible NPE in _create_boolean_func_tree
* Fixed a possible NPE in _populate_func_with_vals
* Disabled ONLY_USE_EQUALITY_JOIN_PREDICATES for the HiveProfile, this should 
have been
done in IMPALA-4101; Hive doesn't support all equality-joins, so specific types 
need to
be disabled, see IMPALA-4101 for more details

Testing:

* Unit tests added: test_query_generator.py and 
test_hive_create_agg_or_analytic_tree.py
* All unit tests pass
* Tested locally against Hive
* Tested against Impala via Leopard
* Tested against Impala via the discrepancy searcher

Change-Id: Ie1096c4cde7ea52a52b39e31cd93242da53b549f
---
M tests/comparison/query_generator.py
M tests/comparison/query_profile.py
A tests/comparison/tests/hive/test_hive_create_agg_or_analytic_tree.py
A tests/comparison/tests/test_query_generator.py
4 files changed, 192 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/4581/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie1096c4cde7ea52a52b39e31cd93242da53b549f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sahil Takiar 

[Impala-ASF-CR] IMPALA-4234: Remove astyle config file, looks outdated.

[Lars Volker (Code Review)]

Lars Volker has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4578

Change subject: IMPALA-4234: Remove astyle config file, looks outdated.
..

IMPALA-4234: Remove astyle config file, looks outdated.

Change-Id: Ibdeba67fcfa538a49d335d42cfdc799753fe7e48
---
D be/.astylerc
1 file changed, 0 insertions(+), 95 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/4578/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4578
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibdeba67fcfa538a49d335d42cfdc799753fe7e48
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 

[Impala-ASF-CR] IMPALA-1702: Enforce table level consistency accross service

[Huaisi Xu (Code Review)]

Huaisi Xu has uploaded a new patch set (#3).

Change subject: IMPALA-1702: Enforce table level consistency accross service
..

IMPALA-1702: Enforce table level consistency accross service

Problems:
1. CatalogServiceCatalog::reset() resets table ID, so same
tables can have different ID and different tables can have
same table ID before/after this. If backend sees these,
it will overwrites each other in an 1:1 table ID to table
descriptor map.

2. 1 can happen because frontend gets table directly
from catalog cache, which is updated concurrently with
other activities, so Analyzer::getTable() can return
different references for the same table when analyzing
the same query.

This commit solves different table having same table ID by
changing table ID counter to an ever increasing 64 bits
integer. It solves same table with different table ID by
keeping a referecedTables_ cache in Analyzer::globalState_
so that calling Analyzer::getTable() on the same table
returns the same references for the same query.

Change-Id: Ifad648b72684ae495ec387590ab1bc58ce5b39e2
---
M common/thrift/Types.thrift
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java
M fe/src/main/java/org/apache/impala/catalog/HBaseTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/IncompleteTable.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
D fe/src/main/java/org/apache/impala/catalog/TableId.java
M fe/src/main/java/org/apache/impala/catalog/TableLoader.java
M fe/src/main/java/org/apache/impala/catalog/TableLoadingMgr.java
M fe/src/main/java/org/apache/impala/catalog/View.java
M fe/src/main/java/org/apache/impala/common/Id.java
M fe/src/main/java/org/apache/impala/planner/HBaseTableSink.java
M fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java
M fe/src/main/java/org/apache/impala/planner/KuduTableSink.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
24 files changed, 82 insertions(+), 157 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/49/4349/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4349
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifad648b72684ae495ec387590ab1bc58ce5b39e2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Huaisi Xu 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Huaisi Xu 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4196: Cross compile bit-byte-functions

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4196: Cross compile bit-byte-functions
..


Patch Set 4: Code-Review+2

Rebased.

-- 
To view, visit http://gerrit.cloudera.org:8080/4557
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5a1291bfd202b500405a884e4a62f0ca2447244a
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4196: Cross compile bit-byte-functions

[Bharath Vissapragada (Code Review)]

Hello Michael Ho, Dan Hecht, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4557

to look at the new patch set (#4).

Change subject: IMPALA-4196: Cross compile bit-byte-functions
..

IMPALA-4196: Cross compile bit-byte-functions

Change-Id: I5a1291bfd202b500405a884e4a62f0ca2447244a
---
M be/src/codegen/impala-ir.cc
M be/src/exprs/CMakeLists.txt
R be/src/exprs/bit-byte-functions-ir.cc
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
4 files changed, 10 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/4557/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4557
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5a1291bfd202b500405a884e4a62f0ca2447244a
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4196: Cross compile bit-byte-functions

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4196: Cross compile bit-byte-functions
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4557/3/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:

Line 2499: select count(shiftleft(int_col, 1)) from functional_parquet.alltypes
> Yes, could you file a JIRA Bharath?
Thanks. Agree with Michael. Opened IMPALA-4233. I'll go ahead and GVM this.


-- 
To view, visit http://gerrit.cloudera.org:8080/4557
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5a1291bfd202b500405a884e4a62f0ca2447244a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Add vim-specific files to .gitignore

[Lars Volker (Code Review)]

Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4562

to look at the new patch set (#2).

Change subject: Add vim-specific files to .gitignore
..

Add vim-specific files to .gitignore

Change-Id: I1abcd8ca0e18178684c916ef6f7d55c25c0814a4
---
M .gitignore
1 file changed, 6 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/4562/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4562
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1abcd8ca0e18178684c916ef6f7d55c25c0814a4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4196: Cross compile bit-byte-functions

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4196: Cross compile bit-byte-functions
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4557/3/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:

Line 2499: select count(shiftleft(int_col, 1)) from functional_parquet.alltypes
> I think that's a valid point that failure to codegen shouldn't be treated a
Yes, could you file a JIRA Bharath?


-- 
To view, visit http://gerrit.cloudera.org:8080/4557
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5a1291bfd202b500405a884e4a62f0ca2447244a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4196: Cross compile bit-byte-functions

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-4196: Cross compile bit-byte-functions
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4557/3/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:

Line 2499: select count(shiftleft(int_col, 1)) from functional_parquet.alltypes
> No, it fails with a warning. Basically I tend to agree with Tim that the ex
I think that's a valid point that failure to codegen shouldn't be treated as a 
fatal error. In general, I think we should rethink on whether codegen should 
happen in ScalarFnCall()::Prepare(). If possible, we should just push the 
codegen to happen at the point when GetCodegenedComputeFn() is called the first 
time like other Expr. It may be slightly more tricky if the external UDF is 
entirely written in LLVM IR in which case there is no way to fall back to 
interpretation.

Anyhow, I don't think we need to address all of these in this patch.


-- 
To view, visit http://gerrit.cloudera.org:8080/4557
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5a1291bfd202b500405a884e4a62f0ca2447244a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4196: Cross compile bit-byte-functions

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4196: Cross compile bit-byte-functions
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4557/3/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:

Line 2499: select count(shiftleft(int_col, 1)) from functional_parquet.alltypes
> You mean it fails with an *error*, not a warning.  Okay.
No, it fails with a warning. Basically I tend to agree with Tim that the 
execution should fall back to interpretation. The problem seems to be in 
ScalarFnCall::Prepare() (pasted the snippet in the bottom) which fails if the 
codegen path fails for some reason. I think, we should ideally have something 
like 

if (skip_codegen || codegen_fails) {
 // Interpret.
}

Should that be fixed too? Thoughts?

 SNIPPET ==
if (skip_codegen) {
// Builtins with char arguments must still have <= 8 arguments.
// TODO: delete when we have codegen for char arguments
if (has_char_arg_or_result) {
  DCHECK(NumFixedArgs() <= 8 && fn_.binary_type == 
TFunctionBinaryType::BUILTIN);
}
Status status = LibCache::instance()->GetSoFunctionPtr(
fn_.hdfs_location, fn_.scalar_fn.symbol, _fn_, _entry_);
if (!status.ok()) {
  if (fn_.binary_type == TFunctionBinaryType::BUILTIN) {
// Builtins symbols should exist unless there is a version mismatch.
return Status(TErrorCode::MISSING_BUILTIN, fn_.name.function_name,
fn_.scalar_fn.symbol);
  } else {
DCHECK_EQ(fn_.binary_type, TFunctionBinaryType::NATIVE);
return Status(Substitute("Problem loading UDF '$0':\n$1",
fn_.name.function_name, status.GetDetail()));
return status;
  }
}
  } else {
// If we got here, either codegen is enabled or we need codegen to run this 
function.
LlvmCodeGen* codegen;
RETURN_IF_ERROR(state->GetCodegen());

if (fn_.binary_type == TFunctionBinaryType::IR) {
  string local_path;
  RETURN_IF_ERROR(LibCache::instance()->GetLocalLibPath(
  fn_.hdfs_location, LibCache::TYPE_IR, _path));
  // Link the UDF module into this query's main module (essentially copy 
the UDF
  // module into the main module) so the UDF's functions are available in 
the main
  // module.
  RETURN_IF_ERROR(codegen->LinkModule(local_path));
}


-- 
To view, visit http://gerrit.cloudera.org:8080/4557
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5a1291bfd202b500405a884e4a62f0ca2447244a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4135: Thrift threaded server times-out connections during high load

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has uploaded a new patch set (#5).

Change subject: IMPALA-4135: Thrift threaded server times-out connections 
during high load
..

IMPALA-4135: Thrift threaded server times-out connections during high load

During times of high load, Thrift's TThreadedServer can't keep up with the
rate of new socket connections, causing some to time out.

This patch creates TAcceptQueueServer, which is a modified version of
TThreadedServer that calls accept() and then hands the returned TTransport
off to a thread pool to handle setting up the connection. This ensures that
accept() is called as quickly as possible, preventing connections from timing
out while waiting.

This patch has been tested locally with the repro shown in IMPALA-4135, but
it still needs to be tested in a real cluster.

Change-Id: Ie50e728974ef31a9d49132a0b3f7cde2a4f3356d
---
M be/src/common/global-flags.cc
M be/src/rpc/CMakeLists.txt
A be/src/rpc/TAcceptQueueServer.cpp
A be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
6 files changed, 459 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/4519/5
-- 
To view, visit http://gerrit.cloudera.org:8080/4519
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie50e728974ef31a9d49132a0b3f7cde2a4f3356d
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Juan Yu 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] Don't assume that AUX exists just because a shell variable is set

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Don't assume that AUX exists just because a shell variable is 
set
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4563/1/tests/run-custom-cluster-tests.sh
File tests/run-custom-cluster-tests.sh:

PS1, Line 42: TEST-impala-custom-
pytest sees the zero-length argument and tries to use it, eventually indexing 
into it at location 0, causing an IndexError. PS2 does things differently. I 
have testing both with and without an aux directory.


-- 
To view, visit http://gerrit.cloudera.org:8080/4563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Don't assume that AUX exists just because a shell variable is set

[Jim Apple (Code Review)]

Hello Michael Brown, Alex Behm,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4563

to look at the new patch set (#2).

Change subject: Don't assume that AUX exists just because a shell variable is 
set
..

Don't assume that AUX exists just because a shell variable is set

This caused breakage of the custom cluster tests -
bin/impala-config.sh sets IMPALA_AUX_TEST_HOME to
$IMPALA_HOME/../Impala-auxiliary-tests, unless it is already set, but
this directory is not guaranteed to
exist. bin/run-custom-cluster-tests.sh was expecting that if
IMPALA_AUX_TEST_HOME is set, then that directory must exist.

While I'm changing bin/run-custom-cluster-tests.sh, add some quotes to
protect against paths with spaces in them.

Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867
---
M tests/run-custom-cluster-tests.sh
1 file changed, 17 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/4563/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieffb6d52c5f95d3257de8c47a039ccb0d45f0867
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 

[Impala-ASF-CR] IMPALA-4118: extract encryption utils from BufferedBlockMgr

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4118: extract encryption utils from BufferedBlockMgr
..


Patch Set 13:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4389/10/be/src/runtime/buffered-block-mgr.cc
File be/src/runtime/buffered-block-mgr.cc:

Line 1342:   // Encrypt to a temporary buffer since so that the original data 
is still in the buffer
> The comment wasn't clear at all - reworded to make it clear that the proble
Thanks. Yeah, I realized when I woke up what the comment was trying to say. 
Agree it's not worth it to fix in bufferblockmgr.


-- 
To view, visit http://gerrit.cloudera.org:8080/4389
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibebe431f69c3c569cbff68171beaa32ef2ab03b6
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4118: extract encryption utils from BufferedBlockMgr

[Internal Jenkins (Code Review)]

Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4118: extract encryption utils from BufferedBlockMgr
..


IMPALA-4118: extract encryption utils from BufferedBlockMgr

As groundwork for IMPALA-4118, extract encryption and integrity
verification routines into a separate module that can be used
by the new BufferPool.

Simplify the logic in BufferedBlockMgr by not distinguishing between the
integrity check and encryption options, which are controlled by the same
command line flag anyway.

This patch changes how the OpenSSL RNG is seeded. I consulted with the
original author of the code (Michael Yoder), who suggested that the new
approach would be appropriate: "I'd pick whatever implementation is
easiest for you. You could add entropy once per query, or once every X
keys (100 keys seems reasonable), or once every "convenient place". 4kb
is probably overkill, you could use 512b for example."

Testing:
Added some unit tests for the utilities. We already have coverage of the
BufferedBlockMgr with encryption in buffered-block-mgr-test. Also reduce
the number of iterations in the buffered-block-mgr-test to save some
testing time.

Change-Id: Ibebe431f69c3c569cbff68171beaa32ef2ab03b6
Reviewed-on: http://gerrit.cloudera.org:8080/4389
Reviewed-by: Tim Armstrong 
Tested-by: Internal Jenkins
---
M be/src/benchmarks/bswap-benchmark.cc
M be/src/bufferpool/buffer-pool-test.cc
M be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/reservation-tracker-test.cc
M be/src/bufferpool/reservation-tracker.cc
M be/src/codegen/codegen-symbol-emitter.cc
M be/src/common/init.cc
M be/src/common/names.h
M be/src/exec/catalog-op-executor.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/kudu-scan-node-test.cc
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-scanner.cc
M be/src/exec/parquet-column-readers.cc
M be/src/exec/parquet-metadata-utils.cc
M be/src/exprs/bit-byte-functions.cc
M be/src/exprs/operators-ir.cc
M be/src/exprs/timezone_db.cc
M be/src/runtime/buffered-block-mgr-test.cc
M be/src/runtime/buffered-block-mgr.cc
M be/src/runtime/buffered-block-mgr.h
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/hdfs-fs-cache-test.cc
M be/src/scheduling/request-pool-service.cc
M be/src/util/CMakeLists.txt
M be/src/util/cpu-info.cc
M be/src/util/hdr-histogram.cc
A be/src/util/openssl-util-test.cc
A be/src/util/openssl-util.cc
A be/src/util/openssl-util.h
M be/src/util/redactor-config-parser-test.cc
M be/src/util/redactor.cc
32 files changed, 505 insertions(+), 266 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/4389
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ibebe431f69c3c569cbff68171beaa32ef2ab03b6
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4118: extract encryption utils from BufferedBlockMgr

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4118: extract encryption utils from BufferedBlockMgr
..


Patch Set 12: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/4389
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibebe431f69c3c569cbff68171beaa32ef2ab03b6
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4216: Test became flaky: TestTpchMemLimitError.test low mem limit q20

[Attila Jeges (Code Review)]

Attila Jeges has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4572

Change subject: IMPALA-4216: Test became flaky: 
TestTpchMemLimitError.test_low_mem_limit_q20
..

IMPALA-4216: Test became flaky:
TestTpchMemLimitError.test_low_mem_limit_q20

Changed the capitalisation of "Memory limit exceeded" error report
message because it was inconsistent with the capitalisation
elsewhere.

This will make tests that expect a generic "Memory limit exceeded"
error more robust.

Change-Id: I471a90c8e3bdecb4fed08fbae3436c50b8618471
---
M be/src/runtime/runtime-state.cc
1 file changed, 1 insertion(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/4572/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4572
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I471a90c8e3bdecb4fed08fbae3436c50b8618471
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges 

[Impala-ASF-CR] IMPALA-889: Add support for an ISO-SQL compliant trim() function.

[Youwei Wang (Code Review)]

Youwei Wang has uploaded a new patch set (#6).

Change subject: IMPALA-889: Add support for an ISO-SQL compliant trim() 
function.
..

IMPALA-889: Add support for an ISO-SQL compliant trim() function.

Purpose: Removes all instances of one or more characters from the
  specified direction(s) of a STRING value.
Syntax #1 TRIM(where FROM string_to_be_trimmed);
Syntax #2 TRIM(trim_char_set FROM string_to_be_trimmed);
Syntax #3 TRIM(where trim_char_set FROM string_to_be_trimmed);
Syntax #3 confirms the standard SQL syntax (Core SQL feature ID
  E021-09).

"where": Case-insensitive trim direction. Valid options are:
  leading|trailing|both. leading means trimming characters from the
  start; trailing means trimming characters from the end; both means
  trimming characters from both sides. These options should be
  provided without single/double quotation marks since they are not
  string literals but identifiers. NULL or empty argument implies
  "both" option. If an invalid option is specified, returns target
  untouched.
"trim_char_set": Case-sensitive characters to be removed. This
  argument is regarded as a character set going to be removed. So the
  occurrence order of each character doesn't matter and duplicated
  instance of same character will be ignored. NULL argument implies
  " "(space) by default. Empty argument return string_to_be_trimmed
  untouched.
"target": Case-sensitive target string to trim. This argument can be
  NULL. Blank argument causes parsing error.
Note: For Syntax #1, since no "characters" is specified, it trims
" "(space) by default. For Syntax #2, since no "where" is specified,
the option both is implied by default.
Return type: string

Examples:
Syntax #1:
trim(both 'abfg%' from 'abc%%defg%'); returns 'c%%de';
trim(leading FROM ' 123 '); returns '123 ';
trim(trailing FROM ' 123 '); returns ' 123';
Syntax #2:
trim('&@' FROM '&@&@&@&127+1-@&@&@&@')"; returns "127+1-";
Syntax #3:
trim(leading 'ab%' from 'abc%%defg%'); returns 'c%%defg%';
trim(trailing 'bfg%' from 'abc%%defg%'); returns 'abc%%de';
trim(both 'abfg%' from 'abc%%defg%')"; returns "c%%de";

Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
---
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/string-functions.h
M common/function-registry/impala_functions.py
M common/thrift/Exprs.thrift
M fe/src/main/cup/sql-parser.cup
A fe/src/main/java/org/apache/impala/analysis/TrimExpr.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
9 files changed, 427 insertions(+), 30 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/4474/6
-- 
To view, visit http://gerrit.cloudera.org:8080/4474
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4753c608b0b00569bf8c5e95b132df6df358e602
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Youwei Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Youwei Wang 

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Internal Jenkins (Code Review)]

Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue
..


IMPALA-4026: Implement double-buffering for BlockingQueue

With recent changes to improve the parquet scanner's efficency,
row batches are produced more quickly, leading to higher contention
in the blocking queue shared between scanner threads and the scan
node. The contention happens between different producers (i.e.
the scanner threads) and also to a lesser extent, between the
scanner threads and the scan node.

This change addresses the contention between the scanner threads
and the scan node by splitting the queue into a 'get_list_' and
a 'put_list_'. The consumers will consume from 'get_list_' until
it's exhausted while the producers will enqueue into 'put_list_'
until it's full. When 'get_list_' is exhausted, the consumer will
atomically swap the 'get_list_' with 'put_list_'. This reduces
the contention: 'get_list_' and 'put_list_' are protected by two
different locks so callers of BlockingGet() only contends for the
'put_lock_' when 'put_list_' is empty.

With this change, primitive_filter_bigint_non_selective improves
by 33.9%, going from 1.60s to 1.06s

Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
Reviewed-on: http://gerrit.cloudera.org:8080/4350
Reviewed-by: Michael Ho 
Tested-by: Internal Jenkins
---
M be/src/common/compiler-util.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/util/blocking-queue.h
A be/src/util/condition-variable.h
M be/src/util/thread-pool.h
6 files changed, 220 insertions(+), 66 deletions(-)

Approvals:
  Michael Ho: Looks good to me, approved
  Internal Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/4350
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Juan Yu 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue
..


Patch Set 12: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/4350
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Juan Yu 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4118: extract encryption utils from BufferedBlockMgr

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4118: extract encryption utils from BufferedBlockMgr
..


Patch Set 12:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4389/10/be/src/runtime/buffered-block-mgr.cc
File be/src/runtime/buffered-block-mgr.cc:

Line 1342:   // Encrypt to a temporary buffer since so that the original data 
is still in the buffer
> Are blocks really shared across threads like that?  Anyway, fine to leave t
The comment wasn't clear at all - reworded to make it clear that the problem is 
re-pinning the block before the write is finished. I think this scenario will 
be easy to avoid. Probably wouldn't be too hard to fix here but doesn't seem 
worth the effort.


http://gerrit.cloudera.org:8080/#/c/4389/10/be/src/runtime/buffered-block-mgr.h
File be/src/runtime/buffered-block-mgr.h:

PS10, Line 255: , we can
  : /// instead decrypt data i
> I didn't know what this was trying to say until I saw the clearer comment i
Done


PS10, Line 656: 
> is this talking about encrypted_write_buffer_, or something else?
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4389
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibebe431f69c3c569cbff68171beaa32ef2ab03b6
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4118: extract encryption utils from BufferedBlockMgr

[Tim Armstrong (Code Review)]

Hello Alex Behm, Dan Hecht,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4389

to look at the new patch set (#11).

Change subject: IMPALA-4118: extract encryption utils from BufferedBlockMgr
..

IMPALA-4118: extract encryption utils from BufferedBlockMgr

As groundwork for IMPALA-4118, extract encryption and integrity
verification routines into a separate module that can be used
by the new BufferPool.

Simplify the logic in BufferedBlockMgr by not distinguishing between the
integrity check and encryption options, which are controlled by the same
command line flag anyway.

This patch changes how the OpenSSL RNG is seeded. I consulted with the
original author of the code (Michael Yoder), who suggested that the new
approach would be appropriate: "I'd pick whatever implementation is
easiest for you. You could add entropy once per query, or once every X
keys (100 keys seems reasonable), or once every "convenient place". 4kb
is probably overkill, you could use 512b for example."

Testing:
Added some unit tests for the utilities. We already have coverage of the
BufferedBlockMgr with encryption in buffered-block-mgr-test. Also reduce
the number of iterations in the buffered-block-mgr-test to save some
testing time.

Change-Id: Ibebe431f69c3c569cbff68171beaa32ef2ab03b6
---
M be/src/benchmarks/bswap-benchmark.cc
M be/src/bufferpool/buffer-pool-test.cc
M be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/reservation-tracker-test.cc
M be/src/bufferpool/reservation-tracker.cc
M be/src/codegen/codegen-symbol-emitter.cc
M be/src/common/init.cc
M be/src/common/names.h
M be/src/exec/catalog-op-executor.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/kudu-scan-node-test.cc
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-scanner.cc
M be/src/exec/parquet-column-readers.cc
M be/src/exec/parquet-metadata-utils.cc
M be/src/exprs/bit-byte-functions.cc
M be/src/exprs/operators-ir.cc
M be/src/exprs/timezone_db.cc
M be/src/runtime/buffered-block-mgr-test.cc
M be/src/runtime/buffered-block-mgr.cc
M be/src/runtime/buffered-block-mgr.h
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/hdfs-fs-cache-test.cc
M be/src/scheduling/request-pool-service.cc
M be/src/util/CMakeLists.txt
M be/src/util/cpu-info.cc
M be/src/util/hdr-histogram.cc
A be/src/util/openssl-util-test.cc
A be/src/util/openssl-util.cc
A be/src/util/openssl-util.h
M be/src/util/redactor-config-parser-test.cc
M be/src/util/redactor.cc
32 files changed, 505 insertions(+), 266 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/4389/11
-- 
To view, visit http://gerrit.cloudera.org:8080/4389
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibebe431f69c3c569cbff68171beaa32ef2ab03b6
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong