[Impala-ASF-CR] IMPALA-9990: Support SET OWNER for Kudu tables

2020-08-18 Thread Fang-Yu Rao (Code Review)
Fang-Yu Rao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16273 )

Change subject: IMPALA-9990: Support SET OWNER for Kudu tables
..


Patch Set 1:

(3 comments)

Hi Andrew and Attila, I have replied your comments provided in the previous 
iteration and think it may be good to have a discussion about how Impala should 
implement SET OWNER for a Kudu table under different scenarios (Kudu-HMS 
integration enabled v.s. disabled).

I also added Vihang as an additional reviewer since Vihang also worked on the 
functionality of creating Kudu tables in Impala and thus may be able to offer 
some insight into it.

http://gerrit.cloudera.org:8080/#/c/16273/1/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/16273/1/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@551
PS1, Line 551: alterKuduTable(tbl, alterTableOptions, errMsg);
> This may be more a question for Kudu, but in what versions of Kudu is nativ
Thanks Andrew!

I do not have concrete answers to both of your questions, i.e., 1) what the 
behavior should be when HMS synchronization is disabled v.s. enabled, and 2) 
what the behavior should be when the Kudu table is external v.s. internal from 
Impala's perspective. But I did carry out an investigation of how a DDL 
statement of a Kudu table is implemented in Impala.

According to CatalogOpExecutor#alterTable(), if this DDL statement is for a 
Kudu table, it looks like Impala would not contact HMS to apply the metadata 
change, which can be seen at 
https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L680.
 To be specific, after finishing CatalogOpExecutor#alterKuduTable(), 
CatalogOpExecutor#alterTable() returns immediately without going into the 
following switch block 
(https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L682-L845).

Recall that for a DDL statement of a Kudu table,  
CatalogOpExecutor#alterKuduTable() would call the corresponding method in 
KuduCatalogOpExecutor.java 
(https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java#L682-L845),
 which in turn would call KuduCatalogOpExecutor#alterKuduTable() at 
https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java#L550-L561.

Therefore, it is possible that the owner of a Kudu table from Kudu's 
perspective would be different than the owner from HMS's perspective if Kudu 
does not update HMS accordingly after the DDL statement. To verify this, I 
tried adding the following two statements before the return statement at 
https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L680
 and found that the owner of a Kudu table could indeed be different if Kudu 
does not update the HMS on the ownership change.

org.apache.hadoop.hive.metastore.api.Table msTbl = 
tbl.getMetaStoreTable().deepCopy();
String oldOwner = msTbl.getOwner();

To address this issue, we need a way to determine whether Kudu and HMS are 
synchronized. I found there is a method 
CatalogOpExecutor#isKuduHmsIntegrationEnabled() in CatalogOpExecutor.java at 
https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java#L1755-L1764,
 which could/may be used to determine whether the ownership information is 
synchronized between Kudu and HMS. Hence, based on this method, we could decide 
whether or not to update HMS on the ownership change of the Kudu table when 
Kudu and HMS is not synchronized.

On the other hand, I have checked that using the current patch, alterSetOwner() 
could effectively alter the owner of the specified Kudu table no matter the 
Kudu table is external or internal. This could be manually verified by a 
revised Python program provided at 
https://github.com/apache/kudu/blob/master/examples/python/basic-python-example/basic_example.py.


http://gerrit.cloudera.org:8080/#/c/16273/1/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
File testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test:

http://gerrit.cloudera.org:8080/#/c/16273/1/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test@51
PS1, Line 51: alter table simple set owner user non_owner
> is it possible to check if the owner of "simple" is actually "non_owner"? I
Thanks Attila!

It looks currently there is no command in Impala that allows a user to check 
the owner of a given table.

A possible way to verify the owner of a Kudu table is through the Kudu-Python 
client currently adopted in Impala's end-to-end tests. For example, kudu.client 
at 

[Impala-ASF-CR] IMPALA-9544 Replace Intel's SSE instructions with ARM's NEON instructions

2020-08-18 Thread Anonymous Coward (Code Review)
zhaoren...@hotmail.com has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15531 )

Change subject: IMPALA-9544 Replace Intel's SSE instructions with ARM's NEON 
instructions
..


Patch Set 46:

Hi, Tim, sorry, there is a small bug on bit-util-test.cc, now I fixed it.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7dfe17125b2910ece54e7dd18b4e4b25d7de8b9
Gerrit-Change-Number: 15531
Gerrit-PatchSet: 46
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: RuiChen 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 18 Aug 2020 09:39:58 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-9544 Replace Intel's SSE instructions with ARM's NEON instructions

2020-08-18 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15531 )

Change subject: IMPALA-9544 Replace Intel's SSE instructions with ARM's NEON 
instructions
..


Patch Set 46:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/15531/46/be/src/util/sse2neon.h
File be/src/util/sse2neon.h:

http://gerrit.cloudera.org:8080/#/c/15531/46/be/src/util/sse2neon.h@213
PS46, Line 213: // 
https://msdn.microsoft.com/en-us/library/bb514059%28v=vs.120%29.aspx?f=255=-2147217396
line too long (99 > 90)


http://gerrit.cloudera.org:8080/#/c/15531/46/be/src/util/sse2neon.h@406
PS46, Line 406: // 
https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2010/whtfzhzk(v=vs.100)
line too long (104 > 90)


http://gerrit.cloudera.org:8080/#/c/15531/46/be/src/util/sse2neon.h@413
PS46, Line 413: // 
https://software.intel.com/sites/landingpage/IntrinsicsGuide/#text=_mm_set1_epi64x=4961
line too long (97 > 90)


http://gerrit.cloudera.org:8080/#/c/15531/46/be/src/util/sse2neon.h@1054
PS46, Line 1054: // 
https://software.intel.com/sites/landingpage/IntrinsicsGuide/#text=_mm_shuffle_epi8=5146
line too long (98 > 90)


http://gerrit.cloudera.org:8080/#/c/15531/46/be/src/util/sse2neon.h@1199
PS46, Line 1199: // 
https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2010/y41dkk37(v=vs.100)
line too long (104 > 90)


http://gerrit.cloudera.org:8080/#/c/15531/46/be/src/util/sse2neon.h@1645
PS46, Line 1645: // 
https://software.intel.com/sites/landingpage/IntrinsicsGuide/#text=_mm_test_all_zeros=5871
line too long (100 > 90)


http://gerrit.cloudera.org:8080/#/c/15531/46/be/src/util/sse2neon.h@3581
PS46, Line 3581: // 
https://github.com/ColinIanKing/linux-next-mirror/blob/b5f466091e130caaf0735976648f72bd5e09aa84/crypto/aegis128-neon-inner.c#L52
line too long (131 > 90)


http://gerrit.cloudera.org:8080/#/c/15531/46/be/src/util/sse2neon.h@3681
PS46, Line 3681: // 
cpp-compiler-developer-guide-and-reference-allocating-and-freeing-aligned-memory-blocks
line too long (98 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7dfe17125b2910ece54e7dd18b4e4b25d7de8b9
Gerrit-Change-Number: 15531
Gerrit-PatchSet: 46
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: RuiChen 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 18 Aug 2020 09:39:51 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-9544 Replace Intel's SSE instructions with ARM's NEON instructions

2020-08-18 Thread Anonymous Coward (Code Review)
zhaoren...@hotmail.com has uploaded a new patch set (#46). ( 
http://gerrit.cloudera.org:8080/15531 )

Change subject: IMPALA-9544 Replace Intel's SSE instructions with ARM's NEON 
instructions
..

IMPALA-9544 Replace Intel's SSE instructions with ARM's NEON instructions

Replace Intel's SSE instructions with ARM's NEON instructions
Replace Intel's crc32 instructions with ARM's instructions
Replace Intel's popcntq instruction with ARM's mechanism
Replace Intel's pcmpestri and pcmpestrm instructions
with ARM mechanism

Change-Id: Id7dfe17125b2910ece54e7dd18b4e4b25d7de8b9
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/benchmarks/bswap-benchmark.cc
M be/src/benchmarks/int-hash-benchmark.cc
M be/src/codegen/CMakeLists.txt
M be/src/codegen/llvm-codegen-test.cc
M be/src/exec/delimited-text-parser.inline.h
M be/src/kudu/util/block_bloom_filter.cc
M be/src/kudu/util/group_varint-inl.h
M be/src/kudu/util/group_varint-test.cc
A be/src/kudu/util/sse2neon.h
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.cc
M be/src/util/bit-util.h
M be/src/util/bloom-filter.cc
M be/src/util/bloom-filter.h
M be/src/util/hash-util-ir.cc
M be/src/util/hash-util.h
M be/src/util/sse-util.h
A be/src/util/sse2neon.h
M bin/rat_exclude_files.txt
21 files changed, 3,966 insertions(+), 35 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/15531/46
--
To view, visit http://gerrit.cloudera.org:8080/15531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id7dfe17125b2910ece54e7dd18b4e4b25d7de8b9
Gerrit-Change-Number: 15531
Gerrit-PatchSet: 46
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: RuiChen 
Gerrit-Reviewer: Tim Armstrong 


[Impala-ASF-CR] IMPALA-9544 Replace Intel's SSE instructions with ARM's NEON instructions

2020-08-18 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/15531 )

Change subject: IMPALA-9544 Replace Intel's SSE instructions with ARM's NEON 
instructions
..


Patch Set 46:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/6946/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7dfe17125b2910ece54e7dd18b4e4b25d7de8b9
Gerrit-Change-Number: 15531
Gerrit-PatchSet: 46
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: RuiChen 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 18 Aug 2020 09:59:58 +
Gerrit-HasComments: No