You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2018/07/26 01:40:15 UTC

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Hello Bharath Vissapragada, Tianyi Wang, Vuk Ercegovac,

I'd like you to do a code review. Please visit

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

to review the following change.


Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................

IMPALA-7203. Support UDFs in LocalCatalog

This adds support to LocalCatalog to load persistent UDFs (both Java and
native) from the HMS. Transient UDFs are not supported, since, without a
central component to store them between restarts, there isn't any clear
path to doing so.

The various UDF tests do not yet pass reliably with this change since so
many of them rely on the transient Java UDF functionality. For now, I
tested manually that I could create, drop, and run some UDFs and that
they show up in 'show functions'.

Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
---
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
M fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
A fe/src/main/java/org/apache/impala/util/FunctionUtils.java
M fe/src/main/java/org/apache/impala/util/PatternMatcher.java
9 files changed, 542 insertions(+), 255 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................


Patch Set 3:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java:

http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@75
PS3, Line 75: If the value is 'null', indicates that the function exists but
            :    * has not been loaded yet.
seems to have been replaced by the bit in FunctionOverloads that specifies if loading is needed. perhaps stale?


http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@190
PS3, Line 190:   private void loadFunctionNames() {
lemme see if I've got this correct. each query has its own LocalCatalog, therefore its own LocalDb. the provider has a lifetime that spans all LocalCatalogs. As a result, any query that uses any udf will wind up fetching all function names. a caching provider can help with this.


http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
File fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java:

http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java@64
PS3, Line 64: Load
Retrieve? Unclear if this has a side-effect.


http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java@69
PS3, Line 69:  from the HMS
remove since its specific for the provider implementation?


http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java@71
PS3, Line 71: org.apache.hadoop.hive.metastore.api.
remove the fully qualified name for consistency with the other api's here.


http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/util/FunctionUtils.java
File fe/src/main/java/org/apache/impala/util/FunctionUtils.java:

http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/util/FunctionUtils.java@84
PS3, Line 84: // TODO(todd): cache these jars based on the mtime and file ID of the
            :       // remote JAR? Can we share a cache with the backend?
> agreed, we should re-use FeSupport CacheJar and related methods. however, a
clarification.... the code that was refactored seems to be used on the catalogd end. the localdb otoh lives in the impalad. in that case, there's a lib-cache available so we should use it so that we avoid copying jars around multiple times to the same host when functions are used in the frontend and the backend. this redundancy will only be there when an impalad is both a coordinator and an executor. while this should be less likely, the combo is still supported.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Aug 2018 16:41:51 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................


Patch Set 5:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2969/ DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Aug 2018 22:12:13 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Tianyi Wang, Impala Public Jenkins, Vuk Ercegovac, 

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

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

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

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................

IMPALA-7203. Support UDFs in LocalCatalog

This adds support to LocalCatalog to load persistent UDFs (both Java and
native) from the HMS. Transient UDFs are not supported, since, without a
central component to store them between restarts, there isn't any clear
path to doing so.

The various UDF tests do not yet pass reliably with this change since so
many of them rely on the transient Java UDF functionality. This is a
legacy feature that isn't commonly used today, and we may not be able to
support it in LocalCatalog.

For now, I tested manually that I could create, drop, and run some UDFs
and that they show up in 'show functions'.

Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
---
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
M fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
A fe/src/main/java/org/apache/impala/util/FunctionUtils.java
M fe/src/main/java/org/apache/impala/util/PatternMatcher.java
9 files changed, 539 insertions(+), 255 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java:

http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@75
PS3, Line 75: If the value is 'null', indicates that the function exists but
            :    * has not been loaded yet.
> seems to have been replaced by the bit in FunctionOverloads that specifies 
yea, that's from an earlier version, will remove


http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@190
PS3, Line 190:   private void loadFunctionNames() {
> lemme see if I've got this correct. each query has its own LocalCatalog, th
yea, that's correct. Assumption is that all caching should happen within the provider, so that we don't have to think about concurrency elsewhere in the frontend


http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/util/FunctionUtils.java
File fe/src/main/java/org/apache/impala/util/FunctionUtils.java:

http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/util/FunctionUtils.java@84
PS3, Line 84: // TODO(todd): cache these jars based on the mtime and file ID of the
            :       // remote JAR? Can we share a cache with the backend?
> clarification.... the code that was refactored seems to be used on the cata
that's right, this now runs on the coordinator, and downloads a new copy of the jar for each query that references it. If it's alright with you, I'd like to address this as a TODO rather than in this same patch, since this code may also be less relevant if we switch to the fetch-from-catalogd approach.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Aug 2018 07:35:32 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................


Patch Set 3:

(4 comments)

r4 addresses the comments (so you can look at an r3..r4 diff). Will push r5 as a rebase on trunk.

http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java:

http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@75
PS3, Line 75: If the value is 'null', indicates that the function exists but
            :    * has not been loaded yet.
> yea, that's from an earlier version, will remove
Done


http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
File fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java:

http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java@64
PS3, Line 64: Load
> Retrieve? Unclear if this has a side-effect.
Done


http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java@69
PS3, Line 69:  from the HMS
> remove since its specific for the provider implementation?
Done


http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java@71
PS3, Line 71: org.apache.hadoop.hive.metastore.api.
> remove the fully qualified name for consistency with the other api's here.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Aug 2018 21:47:08 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Aug 2018 21:49:16 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................


Patch Set 1:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/11053/1//COMMIT_MSG@1
PS1, Line 1: Parent:     0af4661c (IMPALA-7320. Avoid calling getFileStatus() for each partition when table is loaded)
> In my understanding the transient UDF is a legacy feature and is not widely
that's correct


http://gerrit.cloudera.org:8080/#/c/11053/1/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
File fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java:

http://gerrit.cloudera.org:8080/#/c/11053/1/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@166
PS1, Line 166: MetaException
> No need to declare MetaException because it extends TException. There are m
fixed the new ones in this patch



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Aug 2018 22:24:18 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................

IMPALA-7203. Support UDFs in LocalCatalog

This adds support to LocalCatalog to load persistent UDFs (both Java and
native) from the HMS. Transient UDFs are not supported, since, without a
central component to store them between restarts, there isn't any clear
path to doing so.

The various UDF tests do not yet pass reliably with this change since so
many of them rely on the transient Java UDF functionality. This is a
legacy feature that isn't commonly used today, and we may not be able to
support it in LocalCatalog.

For now, I tested manually that I could create, drop, and run some UDFs
and that they show up in 'show functions'.

Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Reviewed-on: http://gerrit.cloudera.org:8080/11053
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
M fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
A fe/src/main/java/org/apache/impala/util/FunctionUtils.java
M fe/src/main/java/org/apache/impala/util/PatternMatcher.java
9 files changed, 539 insertions(+), 255 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Todd Lipcon: Looks good to me, approved

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................


Patch Set 3:

(1 comment)

looks fine. still reviewing LocalDb.

http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/util/FunctionUtils.java
File fe/src/main/java/org/apache/impala/util/FunctionUtils.java:

http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/util/FunctionUtils.java@84
PS3, Line 84: // TODO(todd): cache these jars based on the mtime and file ID of the
            :       // remote JAR? Can we share a cache with the backend?
agreed, we should re-use FeSupport CacheJar and related methods. however, afaict, this is just a method move, so preserves the existing implementation.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Aug 2018 16:59:15 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Aug 2018 01:32:57 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/144/ : 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/11053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Aug 2018 23:43:59 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/70/ : 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/11053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jul 2018 02:23:10 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................


Patch Set 1: Code-Review+2

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/11053/1//COMMIT_MSG@1
PS1, Line 1: Parent:     0af4661c (IMPALA-7320. Avoid calling getFileStatus() for each partition when table is loaded)
In my understanding the transient UDF is a legacy feature and is not widely used. Is that correct?


http://gerrit.cloudera.org:8080/#/c/11053/1/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
File fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java:

http://gerrit.cloudera.org:8080/#/c/11053/1/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@166
PS1, Line 166: MetaException
No need to declare MetaException because it extends TException. There are many other instances of this redundancy.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Jul 2018 18:03:09 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................


Patch Set 5: Code-Review+2

Forwarding +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Aug 2018 05:13:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/274/ : 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/11053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Aug 2018 22:21:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................


Patch Set 3: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/util/FunctionUtils.java
File fe/src/main/java/org/apache/impala/util/FunctionUtils.java:

http://gerrit.cloudera.org:8080/#/c/11053/3/fe/src/main/java/org/apache/impala/util/FunctionUtils.java@84
PS3, Line 84: // TODO(todd): cache these jars based on the mtime and file ID of the
            :       // remote JAR? Can we share a cache with the backend?
> that's right, this now runs on the coordinator, and downloads a new copy of
makes sense, so it would only be used by implementations that use direct or caching providers.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Aug 2018 16:44:50 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................


Patch Set 1:

Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/70/ 

Running initial code review checks. This is experimental - please report any issues to tarmstrong@cloudera.com or on this JIRA: IMPALA-7317


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jul 2018 01:40:26 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Tianyi Wang, Impala Public Jenkins, Vuk Ercegovac, 

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

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

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

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................

IMPALA-7203. Support UDFs in LocalCatalog

This adds support to LocalCatalog to load persistent UDFs (both Java and
native) from the HMS. Transient UDFs are not supported, since, without a
central component to store them between restarts, there isn't any clear
path to doing so.

The various UDF tests do not yet pass reliably with this change since so
many of them rely on the transient Java UDF functionality. This is a
legacy feature that isn't commonly used today, and we may not be able to
support it in LocalCatalog.

For now, I tested manually that I could create, drop, and run some UDFs
and that they show up in 'show functions'.

Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
---
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
M fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
A fe/src/main/java/org/apache/impala/util/FunctionUtils.java
M fe/src/main/java/org/apache/impala/util/PatternMatcher.java
9 files changed, 541 insertions(+), 255 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7203. Support UDFs in LocalCatalog

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11053 )

Change subject: IMPALA-7203. Support UDFs in LocalCatalog
......................................................................


Patch Set 5: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6130d07b9c641525382a618a9f8da048c7ae75ed
Gerrit-Change-Number: 11053
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Aug 2018 01:24:36 +0000
Gerrit-HasComments: No