Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HIVE-26314: Support alter function in Hive DDL statement #3360

Closed
wants to merge 5 commits into from

Conversation

wecharyu
Copy link
Contributor

What changes were proposed in this pull request?

Hive SQL does not support ALTER FUNCTION yet, we can refer to the CREATE [OR REPLACE] FUNCTION of Spark to implement the alter function .

Why are the changes needed?

To support alter udf.

Does this PR introduce any user-facing change?

Yes, need change the DDL statement document in the Hive WIKI.

How was this patch tested?

We add a qtest create_or_replace_function.q, can be test by command:

mvn test -Dtest=TestMiniLlapLocalCliDriver -Dqfile=create_or_replace_function.q

@github-actions
Copy link

github-actions bot commented Jun 14, 2022

@check-spelling-bot Report

🔴 Please review

See the files view or the action log for details.

Unrecognized words (3)

api
esri
wkid

Previously acknowledged words that are now absent aarry timestamplocal yyyy
Available dictionaries could cover words not in the dictionary

cspell:cpp/cpp.txt (104293) covers 81 of them
cspell:django/django.txt (2342) covers 14 of them
cspell:golang/go.txt (7745) covers 12 of them
cspell:java/java.txt (33524) covers 11 of them
cspell:filetypes/filetypes.txt (337) covers 10 of them
cspell:aws/aws.txt (1485) covers 10 of them
cspell:css/css.txt (993) covers 9 of them
cspell:rust/rust.txt (112) covers 8 of them
cspell:npm/npm.txt (671) covers 8 of them
cspell:html/html.txt (542) covers 8 of them
cspell:scala/scala.txt (2752) covers 7 of them
cspell:php/php.txt (9785) covers 6 of them
cspell:fullstack/fullstack.txt (181) covers 5 of them
cspell:csharp/csharp.txt (123) covers 5 of them
cspell:python/python.txt (364) covers 3 of them
cspell:lua/lua.txt (391) covers 3 of them
cspell:dotnet/dotnet.txt (9824) covers 2 of them
cspell:ruby/ruby.txt (354) covers 1 of them
cspell:bash/bash-words.txt (22) covers 1 of them

Consider adding them using:

      with:
        extra_dictionaries:
          cspell:cpp/cpp.txt
          cspell:django/django.txt
          cspell:golang/go.txt
          cspell:java/java.txt
          cspell:filetypes/filetypes.txt
          cspell:aws/aws.txt
          cspell:css/css.txt
          cspell:rust/rust.txt
          cspell:npm/npm.txt
          cspell:html/html.txt
          cspell:scala/scala.txt
          cspell:php/php.txt
          cspell:fullstack/fullstack.txt
          cspell:csharp/csharp.txt
          cspell:python/python.txt
          cspell:lua/lua.txt
          cspell:dotnet/dotnet.txt
          cspell:ruby/ruby.txt
          cspell:bash/bash-words.txt

To stop checking additional dictionaries, add:

      with:
        check_extra_dictionaries: ''
To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:wecharyu/hive.git repository
on the master branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
-H "Content-Type: application/json" \
"https://api.github.com/repos/apache/hive/issues/comments/1155409159" > "$comment_json"
comment_body=$(mktemp)
jq -r ".body // empty" "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")

patch_add=$(perl -e '$/=undef; $_=<>; if (m{Unrecognized words[^<]*</summary>\n*```\n*([^<]*)```\n*</details>$}m) { print "$1" } elsif (m{Unrecognized words[^<]*\n\n((?:\w.*\n)+)\n}m) { print "$1" };' < "$comment_body")

update_files
rm $comment_body
git add -u
If the flagged items do not appear to be text

If items relate to a ...

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

  • binary file.

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

@wecharyu
Copy link
Contributor Author

@pvary @nrg4878 : Could you also review this PR?

boolean addToMetastoreSuccess = addToMetastore(dbName, functionName, registeredName);
if (!addToMetastoreSuccess) {
// TODO: should this use getUserFromAuthenticator instead of SessionState.get().getUserName()?
Function function = new Function(functionName, dbName, desc.getClassName(), SessionState.get().getUserName(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How security is handled for altering the functions?
Malicious users could wreak havoc if they change a function implementation behind the scenes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add OWNER_PRIV for altering function? Then only the owner of this function can do alter operation.

@@ -1613,10 +1613,10 @@ resourceType
createFunctionStatement
@init { pushMsg("create function statement", state); }
@after { popMsg(state); }
: KW_CREATE (temp=KW_TEMPORARY)? KW_FUNCTION functionIdentifier KW_AS StringLiteral
: KW_CREATE (temp=KW_TEMPORARY)? KW_FUNCTION orReplace? ifNotExists? functionIdentifier KW_AS StringLiteral
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wecharyu This grammer syntax seems inconsistent with what we have for views.
"create or replace view " where as for functions it would be
"create function or replace "

also the grammer supports the syntax below
create function or replace if not exists

Is this supported? "if not exists" should only apply when the function does not exist. It would not apply for replacing existing one. Would this be confusing for users?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nrg4878 The reason I use "create function or replace" is that “create or replace" can only be used for one statement, I will get error like below when I use "create or replace function":

org.apache.hadoop.hive.ql.parse.ParseException: line 3:18 mismatched input 'function' expecting KW_VIEW near 'replace' in create view statement

I am not sure if antlr3 does not support this syntax in two statements, if you have any idea please let me know.

boolean replace = (root.getFirstChildWithType(HiveParser.TOK_ORREPLACE) != null);
boolean ifNotExists = (root.getFirstChildWithType(HiveParser.TOK_IFNOTEXISTS) != null);
if (ifNotExists && replace) {
throw new SemanticException("CREATE FUNCTION with both IF NOT EXISTS and REPLACE is not allowed.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we are throwing a SemanticException when replace and if not exists are used. This is what I was eluding to in the earlier comment. Should we prevent this in the grammer instead of doing this? This probably provides a better error message though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both syntaxes look good to me, currently this syntax is just similar to Spark.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.

@github-actions github-actions bot added the stale label Aug 23, 2022
@github-actions github-actions bot closed this Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants