Skip to content

Conversation

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Oct 8, 2016

What changes were proposed in this pull request?

Currently, Spark Thrift Server ignores the default database in URI. This PR supports that like the following.

$ bin/beeline -u jdbc:hive2://localhost:10000 -e "create database testdb"
$ bin/beeline -u jdbc:hive2://localhost:10000/testdb -e "create table t(a int)"
$ bin/beeline -u jdbc:hive2://localhost:10000/testdb -e "show tables"
...
+------------+--------------+--+
| tableName  | isTemporary  |
+------------+--------------+--+
| t          | false        |
+------------+--------------+--+
1 row selected (0.347 seconds)
$ bin/beeline -u jdbc:hive2://localhost:10000 -e "show tables"
...
+------------+--------------+--+
| tableName  | isTemporary  |
+------------+--------------+--+
+------------+--------------+--+
No rows selected (0.098 seconds)

How was this patch tested?

Manual.

Note: I tried to add a test case for this, but I cannot found a suitable testsuite for this. I'll add the testcase if some advice is given.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-17819][SQL] Specified database in JDBC URL is ignored when con… [SPARK-17819][SQL] Support default database in connection URIs for Spark Thrift Server Oct 8, 2016
@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Oct 8, 2016

Hi, @rxin .
Sorry, but could you give me some advice about a proper testsuite for this kind of PR?

@SparkQA
Copy link

SparkQA commented Oct 8, 2016

Test build #66551 has finished for PR 15399 at commit f9a294e.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member Author

Oops. Sorry. I'll fix soon.

@SparkQA
Copy link

SparkQA commented Oct 8, 2016

Test build #66555 has finished for PR 15399 at commit 26f4b25.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member Author

Retest this please.

@SparkQA
Copy link

SparkQA commented Oct 8, 2016

Test build #66557 has finished for PR 15399 at commit 26f4b25.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 8, 2016

Test build #66560 has finished for PR 15399 at commit a832760.

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

@dongjoon-hyun
Copy link
Member Author

Hi, @gatorsmile .
Could you review this PR when you have sometime?

@SparkQA
Copy link

SparkQA commented Oct 10, 2016

Test build #66617 has finished for PR 15399 at commit d027421.

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

@gatorsmile
Copy link
Member

First, I am not familiar with the code in this component. Thus, I am not the right person to review it.

Second, when I going over the pending JIRA list, I found many bugs that are reported in Thrift Server.

Third, I do not know what is the current strategy for supporting beeline and spark-sql. If this is the focus, I expect at least 50+ bug fixes in this area

@dongjoon-hyun
Copy link
Member Author

Thank you for commenting, @gatorsmile . So far, I asked @rxin and you. I'll find another committer more closer this part.

@dongjoon-hyun
Copy link
Member Author

Hi, @liancheng .
Could you review this PR about Thrift Server?

@rxin
Copy link
Contributor

rxin commented Oct 10, 2016

Can we add a test case using the jdbc client?

@dongjoon-hyun
Copy link
Member Author

Thank you for your advice, @rxin !!
Which suite do you mean? The suites inside HiveThriftServer2Suites.scala look not proper to me for this kind of cases.

@dongjoon-hyun
Copy link
Member Author

Or, should I create a new testsuite for this kind of testcases?

@SparkQA
Copy link

SparkQA commented Oct 12, 2016

Test build #66841 has finished for PR 15399 at commit 96633d1.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class JdbcConnectionUriSuite extends HiveThriftServer2Test

@dongjoon-hyun
Copy link
Member Author

Hi, @rxin .
Since this kind of tests need to change their URI and also assume that the non-default database exists prior, I made a new testsuite for this.
Could you review this again?

@dongjoon-hyun
Copy link
Member Author

Hi, @rxin .
Could you review this again?

@dongjoon-hyun
Copy link
Member Author

Hi, @rxin or @liancheng .
Could you review this PR when you have some time?

@SparkQA
Copy link

SparkQA commented Oct 17, 2016

Test build #67041 has finished for PR 15399 at commit 7ef307a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class JdbcConnectionUriSuite extends HiveThriftServer2Test

@rxin
Copy link
Contributor

rxin commented Oct 17, 2016

Thanks - merging in master.

@rxin
Copy link
Contributor

rxin commented Oct 17, 2016

Can you create a backport for branch-2.0?

@dongjoon-hyun
Copy link
Member Author

Thank you, @rxin !
Sure. I'll create a backport PR.

@asfgit asfgit closed this in 59e3eb5 Oct 17, 2016
robert3005 pushed a commit to palantir/spark that referenced this pull request Nov 1, 2016
…ark Thrift Server

## What changes were proposed in this pull request?

Currently, Spark Thrift Server ignores the default database in URI. This PR supports that like the following.

```sql
$ bin/beeline -u jdbc:hive2://localhost:10000 -e "create database testdb"
$ bin/beeline -u jdbc:hive2://localhost:10000/testdb -e "create table t(a int)"
$ bin/beeline -u jdbc:hive2://localhost:10000/testdb -e "show tables"
...
+------------+--------------+--+
| tableName  | isTemporary  |
+------------+--------------+--+
| t          | false        |
+------------+--------------+--+
1 row selected (0.347 seconds)
$ bin/beeline -u jdbc:hive2://localhost:10000 -e "show tables"
...
+------------+--------------+--+
| tableName  | isTemporary  |
+------------+--------------+--+
+------------+--------------+--+
No rows selected (0.098 seconds)
```

## How was this patch tested?

Manual.

Note: I tried to add a test case for this, but I cannot found a suitable testsuite for this. I'll add the testcase if some advice is given.

Author: Dongjoon Hyun <dongjoon@apache.org>

Closes apache#15399 from dongjoon-hyun/SPARK-17819.
@dongjoon-hyun dongjoon-hyun deleted the SPARK-17819 branch November 7, 2016 00:49
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
…ark Thrift Server

## What changes were proposed in this pull request?

Currently, Spark Thrift Server ignores the default database in URI. This PR supports that like the following.

```sql
$ bin/beeline -u jdbc:hive2://localhost:10000 -e "create database testdb"
$ bin/beeline -u jdbc:hive2://localhost:10000/testdb -e "create table t(a int)"
$ bin/beeline -u jdbc:hive2://localhost:10000/testdb -e "show tables"
...
+------------+--------------+--+
| tableName  | isTemporary  |
+------------+--------------+--+
| t          | false        |
+------------+--------------+--+
1 row selected (0.347 seconds)
$ bin/beeline -u jdbc:hive2://localhost:10000 -e "show tables"
...
+------------+--------------+--+
| tableName  | isTemporary  |
+------------+--------------+--+
+------------+--------------+--+
No rows selected (0.098 seconds)
```

## How was this patch tested?

Manual.

Note: I tried to add a test case for this, but I cannot found a suitable testsuite for this. I'll add the testcase if some advice is given.

Author: Dongjoon Hyun <dongjoon@apache.org>

Closes apache#15399 from dongjoon-hyun/SPARK-17819.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants