Skip to content

[SPARK-25760][SQL] Set AddJarCommand return empty #22747

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[SPARK-25760][SQL] Set AddJarCommand return empty #22747

wants to merge 1 commit into from

Conversation

@wangyum
Copy link
Member

@wangyum wangyum commented Oct 16, 2018 •
edited
Loading

What changes were proposed in this pull request?

Only AddJarCommand return 0, the user will be confused about what it means. This PR sets it to empty.

spark-sql> add jar /Users/yumwang/spark/sql/hive/src/test/resources/TestUDTF.jar;
ADD JAR /Users/yumwang/spark/sql/hive/src/test/resources/TestUDTF.jar
0
spark-sql>

How was this patch tested?

manual tests

spark-sql> add jar /Users/yumwang/spark/sql/hive/src/test/resources/TestUDTF.jar;
ADD JAR /Users/yumwang/spark/sql/hive/src/test/resources/TestUDTF.jar
spark-sql>

@srowen
Copy link
Member

srowen commented Oct 16, 2018

I tend to agree, but I wonder if it returns a row with 0 to emulate something else like Hive?

@wangyum
Copy link
Member Author

wangyum commented Oct 16, 2018

Hive will not return 0:

hive> add jar /Users/yumwang/spark/sql/hive/src/test/resources/TestUDTF.jar;
Added [/Users/yumwang/spark/sql/hive/src/test/resources/TestUDTF.jar] to class path
Added resources: [/Users/yumwang/spark/sql/hive/src/test/resources/TestUDTF.jar]
hive>

@srowen
Copy link
Member

srowen commented Oct 16, 2018

Interesting, this was changed a long time ago, with the claim that it was needed to match Hive: #4586 (comment) Maybe it's changed again? CC @adrian-wang

@SparkQA
Copy link

SparkQA commented Oct 16, 2018

Test build #97456 has finished for PR 22747 at commit 0a5b0dd.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

HyukjinKwon
HyukjinKwon approved these changes Oct 17, 2018
View reviewed changes
Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

I checked the Hive side code. I think its return code (that indicates the command was successful or not) is misunderstood. Looks not returning rows in Hive side and the current fix looks coherent to me.

dongjoon-hyun
dongjoon-hyun approved these changes Oct 17, 2018
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM, too.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Oct 17, 2018

BTW, @wangyum . Please create a minor JIRA issue. This is a behavior change for users.

cc @gatorsmile

@wangyum wangyum changed the title [MINOR][SQL] Set AddJarCommand return empty [SPARK-25760][SQL] Set AddJarCommand return empty Oct 18, 2018
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Oct 18, 2018

Merged to master.

@asfgit asfgit closed this in 1117fc3 Oct 18, 2018
@HyukjinKwon
Copy link
Member

HyukjinKwon commented Oct 25, 2018 •
edited
Loading

This looks also external changes to existing application users. Shall we update migration guide?

cc @cloud-fan and @gatorsmile

@srowen
Copy link
Member

srowen commented Oct 25, 2018

It seems like a bug fix more than anything, and I assume we wouldn't document every single one, but don't object to mentioning it if that's more standard.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Oct 25, 2018

Yup, that's similar argument I had in #22773 (comment) I think we should clarify what to document there.

@srowen
Copy link
Member

srowen commented Oct 25, 2018

#22826

jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@wangyum
[SPARK-25760][SQL] Set AddJarCommand return empty
1afd91f 
## What changes were proposed in this pull request?

Only `AddJarCommand` return `0`, the user will be confused about what it means. This PR sets it to empty.

```sql
spark-sql> add jar /Users/yumwang/spark/sql/hive/src/test/resources/TestUDTF.jar;
ADD JAR /Users/yumwang/spark/sql/hive/src/test/resources/TestUDTF.jar
0
spark-sql>
```

## How was this patch tested?

manual tests
```sql
spark-sql> add jar /Users/yumwang/spark/sql/hive/src/test/resources/TestUDTF.jar;
ADD JAR /Users/yumwang/spark/sql/hive/src/test/resources/TestUDTF.jar
spark-sql>
```

Closes apache#22747 from wangyum/AddJarCommand.

Authored-by: Yuming Wang <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
@wangyum wangyum deleted the AddJarCommand branch April 6, 2020 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@wangyum @srowen @SparkQA @dongjoon-hyun @HyukjinKwon