date:20180123

[Impala-ASF-CR] IMPALA-5903: Inconsistent specification of result set and result set metadata

2018-01-23 Thread Zoltan Borok-Nagy (Code Review)

Hello Dimitris Tsirogiannis, Tim Armstrong,

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

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

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

Change subject: IMPALA-5903: Inconsistent specification of result set and 
result set metadata
..

IMPALA-5903: Inconsistent specification of result set and result set metadata

Some statements that return a result set declare it in
Frontend.java, but others do not. E.g. COMPUTE STATS returns
a result set, but in Frontend.createCatalogOpRequest() we
declare an empty schema for the result set.

I found inconsistencies amongst the ALTER TABLE statements.
They all handled the same way in Frontend.java, but in
CatalogOpExecutor.alterTable() around half of the various
ALTER TABLE statements return a result set, the other half
don't. Now in Frontend.java we declare the appropriate result
set schema for the ALTER TABLE statements.

Change-Id: Ic542fb8e49e850052416ac663ee329ee3974e3b9
---
M 
fe/src/main/java/org/apache/impala/analysis/AlterTableAddDropRangePartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableDropColStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewRenameStmt.java
M 
fe/src/main/java/org/apache/impala/analysis/AlterTableRecoverPartitionsStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetCachedStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetColumnStats.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetFileFormatStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetLocationStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSortByStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
19 files changed, 68 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic542fb8e49e850052416ac663ee329ee3974e3b9
Gerrit-Change-Number: 9090
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-5903: Inconsistent specification of result set and result set metadata

2018-01-23 Thread Zoltan Borok-Nagy (Code Review)

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9090 )

Change subject: IMPALA-5903: Inconsistent specification of result set and 
result set metadata
..


Patch Set 1:

Thanks for looking at this.

Now that I've run all the tests it turned out COMPUTE STATS in most cases does 
return a result set, but in some rare cases (COMPUTE INCREMENTAL STATS with no 
new partitions) it doesn't return anything. Fortunately both cases can be 
identified at parse-time, so we can set the correct schema in Frontend.java.

As far as I can tell the current behavior is that when the statement can 
produce meaningful information about its execution, we put this information 
into the result set. I don't think we want to throw away this information, so 
the appropriate behavior might be to always return a 'summary'. Do you think 
would it be OK?

About breaking some clients: I think the current behavior is rather a bug. We 
tell the client that the statement doesn't have a result set, but then we 
magically return one. The clients have to know which statements return a result 
set and which statements don't. As Tim pointed out it is hard to imagine that 
clients rely on invalid metadata, but it's possible. So, maybe this change 
could go into Impala v3 / CDH6 only.

Anyway, I'll upload a new PS with minor fixes, and after further discussion 
we'll see how to move forward.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic542fb8e49e850052416ac663ee329ee3974e3b9
Gerrit-Change-Number: 9090
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Tue, 23 Jan 2018 15:30:45 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3942: Fix wrongly escaped string literal in front-end

2018-01-23 Thread Kim Jin Chul (Code Review)

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8818 )

Change subject: IMPALA-3942: Fix wrongly escaped string literal in front-end
..


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8818/6/tests/query_test/test_queries.py
File tests/query_test/test_queries.py:

http://gerrit.cloudera.org:8080/#/c/8818/6/tests/query_test/test_queries.py@310
PS6, Line 310:
> I just realized this comment has not been addressed, sorry. Can you move th
As you mentioned in the previous comment, our unit test can fully cover the 
issue, so I just remove the E2E test.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc4b5f5d8ffaa8feb96a466959427a04b3b06fec
Gerrit-Change-Number: 8818
Gerrit-PatchSet: 10
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 23 Jan 2018 15:28:33 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3942: Fix wrongly escaped string literal in front-end

2018-01-23 Thread Kim Jin Chul (Code Review)

Hello Thomas Tauber-Marshall, Gabor Kaszab, Jim Apple, Tim Armstrong, Alex Behm,

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

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

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

Change subject: IMPALA-3942: Fix wrongly escaped string literal in front-end
..

IMPALA-3942: Fix wrongly escaped string literal in front-end

String literal can be wrapped by either single or double quotes.
There are some holes in escaping the string literal:
- toSql() always returns strings that are single quoted, resulting in
improper escaping in the output if the original string was actually
double quoted.
- It is not always possible to determine if a string "should" be single
or double quoted. e.g. concat('a', "b")

The solution is to normalize any string which comes from user's given
string or a generated string (e.g. constant fold by the rewritter rule)
to single quotes.

Testing:
Added unit tests into expr-test, ToSqlTest

Change-Id: Ibc4b5f5d8ffaa8feb96a466959427a04b3b06fec
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
3 files changed, 93 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/8818/10
--
To view, visit http://gerrit.cloudera.org:8080/8818
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibc4b5f5d8ffaa8feb96a466959427a04b3b06fec
Gerrit-Change-Number: 8818
Gerrit-PatchSet: 10
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-01-23 Thread Kim Jin Chul (Code Review)

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 6:

(22 comments)

Applied the update.

http://gerrit.cloudera.org:8080/#/c/8851/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8851/6//COMMIT_MSG@13
PS6, Line 13: table's comment can be displayed if catalog table
: owns Hive metastore table(HMS) object
> There is no notion of ownership here. A loaded table is always associated w
Done


http://gerrit.cloudera.org:8080/#/c/8851/6//COMMIT_MSG@14
PS6, Line 14: There is not any HMS call
: due to performance concern, so table's comment can be empty even
: though table has a comment.
> Rewrite as: "If the table is not loaded, an empty comment is returned."
Done


http://gerrit.cloudera.org:8080/#/c/8851/6//COMMIT_MSG@18
PS6, Line 18: There is a change in thrift part: a list of table comments is 
added to
: TGetTablesResult struct as an optional element.
> Remove this sentence.
Done


http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog-server.cc@381
PS6, Line 381: for (const TTableInfo& table_info: 
get_tables_info_result.tableinfos) {
 :   Value table_obj(kObjectType);
 :   Value fq_name(Substitute("$0.$1", db.db_name, 
table_info.tbl_name).c_str(),
 :   document->GetAllocator());
 :   table_obj.AddMember("fqtn", fq_name, 
document->GetAllocator());
 :   Value table_name(table_info.tbl_name.c_str(), 
document->GetAllocator());
 :   table_obj.AddMember("name", table_name, 
document->GetAllocator());
 :   table_array.PushBack(table_obj, document->GetAllocator());
 : }
> Why not returning the table comments as well?
Done


http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog.h
File be/src/catalog/catalog.h:

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog.h@83
PS6, Line 83: Hive metastore was not loaded for a table
> "table is not loaded".
Done


http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog.cc@140
PS6, Line 140:   return JniUtil::CallJniMethod(catalog_, get_table_names_id_, 
params, tables_info_result);
> long line
Done


http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/client-request-state.cc@275
PS6, Line 275: for (const TTableInfo& tbl_info: tables_info_result.tableinfos) {
 : names.push_back(tbl_info.tbl_name);
 : comments.push_back(tbl_info.comment);
 :   }
 :   SetResultSet(names, comments);
> This code will result to DCHECK being hit if some table doesn't have the co
Done


http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/frontend.cc
File be/src/service/frontend.cc:

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/frontend.cc@139
PS6, Line 139: Status Frontend::GetTableNames(const string& db, const string* 
pattern,
 : const TSessionState* session, TGetTablesInfoResult* 
tables_info_result) {
 :   TGetTablesInfoParams params;
 :   params.__set_db(db);
 :   if (pattern != NULL) params.__set_pattern(*pattern);
 :   if (session != NULL) params.__set_session(*session);
 :   return JniUtil::CallJniMethod(fe_, get_table_names_id_, 
params, tables_info_result);
 : }
 :
 : Status Frontend::GetTableNamesAndComments(const string& db, 
const string* pattern,
 : const TSessionState* session, TGetTablesInfoResult* 
tables_info_result) {
 :   TGetTablesInfoParams params;
 :   params.__set_db(db);
 :   if (pattern != NULL) params.__set_pattern(*pattern);
 :   if (session != NULL) params.__set_session(*session);
 :   return JniUtil::CallJniMethod(fe_, 
get_table_names_and_comments_id_, params,
 :   tables_info_result);
 : }
> Should be a single call Frontend::GetTablesInfo()
Done


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

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/impala-http-handler.cc@522
PS6, Line 522: for (const TTableInfo& tbl_info: 
get_tables_info_result.tableinfos) {
 :   Value table_obj(kObjectType);
 :   Value fq_name(Substitute("$0.$1", db.db_name, 
tbl_info.tbl_name).c_str(),

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

2018-01-23 Thread Kim Jin Chul (Code Review)

Hello Dimitris Tsirogiannis,

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

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

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

Change subject: IMPALA-3193: Show table's comment on show tables
..

IMPALA-3193: Show table's comment on show tables

Currently SHOW TABLES command does not display given comment
when creating a table. DESC  shows column's comment and
SHOW DATABASES also shows database's comment.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test. TestShowTables.testTableCommentVisibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/Table.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
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M tests/metadata/test_metadata_query_statements.py
17 files changed, 190 insertions(+), 90 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/7
--
To view, visit http://gerrit.cloudera.org:8080/8851
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 7
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul

[Impala-ASF-CR] IMPALA-5801: [draft] Clean up codegen GetType() interface

2018-01-23 Thread Csaba Ringhofer (Code Review)

Hello Lars Volker, Tim Armstrong,

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

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

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

Change subject: IMPALA-5801: [draft] Clean up codegen GetType() interface
..

IMPALA-5801: [draft] Clean up codegen GetType() interface

First draft - my goal with this patch is create a review
where we can decide on the scope of the refactor and
agree on the names of the functions.

Change-Id: Ib146ca21af82023b0b460f813eae3435b4b2eb77
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/text-converter.cc
M be/src/exprs/compound-predicates.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/slot-ref.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/tuple.cc
M be/src/runtime/types.cc
M be/src/util/tuple-row-compare.cc
21 files changed, 224 insertions(+), 210 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib146ca21af82023b0b460f813eae3435b4b2eb77
Gerrit-Change-Number: 9063
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5801: [draft] Clean up codegen GetType() interface

2018-01-23 Thread Csaba Ringhofer (Code Review)

Hello Lars Volker, Tim Armstrong,

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

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

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

Change subject: IMPALA-5801: [draft] Clean up codegen GetType() interface
..

IMPALA-5801: [draft] Clean up codegen GetType() interface

First draft - my goal with this patch is create a review
where we can decide on the scope of the refactor and
agree on the names of the functions.

Change-Id: Ib146ca21af82023b0b460f813eae3435b4b2eb77
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/text-converter.cc
M be/src/exprs/compound-predicates.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/slot-ref.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/tuple.cc
M be/src/runtime/types.cc
M be/src/util/tuple-row-compare.cc
21 files changed, 224 insertions(+), 210 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib146ca21af82023b0b460f813eae3435b4b2eb77
Gerrit-Change-Number: 9063
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5903: Inconsistent specification of result set and result set metadata

[Impala-ASF-CR] IMPALA-5903: Inconsistent specification of result set and result set metadata

[Impala-ASF-CR] IMPALA-3942: Fix wrongly escaped string literal in front-end

[Impala-ASF-CR] IMPALA-3942: Fix wrongly escaped string literal in front-end

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

[Impala-ASF-CR] IMPALA-5801: [draft] Clean up codegen GetType() interface

[Impala-ASF-CR] IMPALA-5801: [draft] Clean up codegen GetType() interface

< 1 2

101 - 108 of 108 matches

Site Navigation

Mail list logo

Footer information