You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/09/20 11:13:29 UTC
[GitHub] [incubator-kyuubi] Yikf opened a new pull request, #3530: [KYUUBI #3529] Add tests to check for consistency with V1 in hive connector
Yikf opened a new pull request, #3530:
URL: https://github.com/apache/incubator-kyuubi/pull/3530
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/CONTRIBUTING.html
2. If the PR is related to an issue in https://github.com/apache/incubator-kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
Fix https://github.com/apache/incubator-kyuubi/issues/3529
Add tests to check for consistency with V1
### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [ ] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3530: [KYUUBI #3529] [TEST] Supplement the tests related namespace, showtables w/ Initialize the DDL test framework and fix minor consistency issue with v1 for hive connector
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3530:
URL: https://github.com/apache/incubator-kyuubi/pull/3530#discussion_r990934507
##########
extensions/spark/kyuubi-spark-connector-hive/src/test/scala/org/apache/kyuubi/spark/connector/hive/HiveCatalogSuite.scala:
##########
@@ -51,4 +124,203 @@ class HiveCatalogSuite extends KyuubiHiveTest {
assert(exception.message === "Table or view not found: hive.ns1.nonexistent_table")
}
}
+
+ test("listTables") {
+
+ val ident1 = Identifier.of(Array("ns"), "test_table_1")
+ val ident2 = Identifier.of(Array("ns"), "test_table_2")
+ val ident3 = Identifier.of(Array("ns2"), "test_table_1")
+
+ assert(catalog.listTables(Array("ns")).isEmpty)
+
+ catalog.createTable(ident1, schema, Array.empty, emptyProps)
+
+ assert(catalog.listTables(Array("ns")).toSet == Set(ident1))
+ assert(catalog.listTables(Array("ns2")).isEmpty)
+
+ catalog.createTable(ident3, schema, Array.empty, emptyProps)
+ catalog.createTable(ident2, schema, Array.empty, emptyProps)
+
+ assert(catalog.listTables(Array("ns")).toSet == Set(ident1, ident2))
+ assert(catalog.listTables(Array("ns2")).toSet == Set(ident3))
+
+ catalog.dropTable(ident1)
+
+ assert(catalog.listTables(Array("ns")).toSet == Set(ident2))
+
+ catalog.dropTable(ident2)
+
+ assert(catalog.listTables(Array("ns")).isEmpty)
+ assert(catalog.listTables(Array("ns2")).toSet == Set(ident3))
+
+ catalog.dropTable(ident3)
+ }
+
+ test("createTable") {
+ assert(!catalog.tableExists(testIdent))
+
+ val table = catalog.createTable(testIdent, schema, Array.empty, emptyProps)
+
+ val parsed = CatalystSqlParser.parseMultipartIdentifier(table.name)
+ assert(parsed == Seq("db", "test_table"))
+ assert(table.schema == schema)
+ assert(filterV2TableProperties(table.properties) == Map())
+
+ assert(catalog.tableExists(testIdent))
+ catalog.dropTable(testIdent)
+ }
+
+ test("createTable: with properties") {
+ val properties = new util.HashMap[String, String]()
+ properties.put("property", "value")
+
+ assert(!catalog.tableExists(testIdent))
+
+ val table = catalog.createTable(testIdent, schema, Array.empty, properties)
+
+ val parsed = CatalystSqlParser.parseMultipartIdentifier(table.name)
+ assert(parsed == Seq("db", "test_table"))
+ assert(table.schema == schema)
+ assert(filterV2TableProperties(table.properties).asJava == properties)
+
+ assert(catalog.tableExists(testIdent))
+ catalog.dropTable(testIdent)
+ }
+
+ test("createTable: table already exists") {
+ assert(!catalog.tableExists(testIdent))
+
+ val table = catalog.createTable(testIdent, schema, Array.empty, emptyProps)
+
+ val exc = intercept[TableAlreadyExistsException] {
+ catalog.createTable(testIdent, schema, Array.empty, emptyProps)
+ }
+
+ assert(exc.message.contains(table.name()))
+ assert(exc.message.contains("already exists"))
+
+ assert(catalog.tableExists(testIdent))
+ catalog.dropTable(testIdent)
+ }
+
+ test("tableExists") {
+ assert(!catalog.tableExists(testIdent))
+
+ catalog.createTable(testIdent, schema, Array.empty, emptyProps)
+
+ assert(catalog.tableExists(testIdent))
+
+ catalog.dropTable(testIdent)
+
+ assert(!catalog.tableExists(testIdent))
+ }
+
+ test("createTable: location") {
+ val properties = new util.HashMap[String, String]()
+ assert(!catalog.tableExists(testIdent))
+
+ // default location
+ val t1 = catalog.createTable(testIdent, schema, Array.empty, properties).asInstanceOf[HiveTable]
+ assert(t1.catalogTable.location ===
+ catalog.catalog.defaultTablePath(testIdent.asTableIdentifier))
+ catalog.dropTable(testIdent)
+
+ // relative path
+ properties.put(TableCatalog.PROP_LOCATION, "relative/path")
+ val t2 = catalog.createTable(testIdent, schema, Array.empty, properties).asInstanceOf[HiveTable]
+ assert(t2.catalogTable.location === makeQualifiedPathWithWarehouse("db.db/relative/path"))
+ catalog.dropTable(testIdent)
+
+ // absolute path without scheme
+ properties.put(TableCatalog.PROP_LOCATION, "/absolute/path")
+ val t3 = catalog.createTable(testIdent, schema, Array.empty, properties).asInstanceOf[HiveTable]
+ assert(t3.catalogTable.location.toString === "file:/absolute/path")
Review Comment:
the canonicalized path should be "file:///absolute/path", while "file://" is a prefix, like "hdfs://"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3530: [KYUUBI #3529] [TEST] Add tests to check for consistency with V1 in hive connector
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3530:
URL: https://github.com/apache/incubator-kyuubi/pull/3530#issuecomment-1263134185
We may need to extend or build a test framework like SPARK-33382
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] Yikf commented on a diff in pull request #3530: [KYUUBI #3529] [TEST] Supplement the tests related namespace, showtables w/ Initialize the DDL test framework and fix minor consistency issue with v1 for hive connector
Posted by GitBox <gi...@apache.org>.
Yikf commented on code in PR #3530:
URL: https://github.com/apache/incubator-kyuubi/pull/3530#discussion_r994649261
##########
extensions/spark/kyuubi-spark-connector-hive/src/test/scala/org/apache/kyuubi/spark/connector/hive/HiveCatalogSuite.scala:
##########
@@ -51,4 +124,203 @@ class HiveCatalogSuite extends KyuubiHiveTest {
assert(exception.message === "Table or view not found: hive.ns1.nonexistent_table")
}
}
+
+ test("listTables") {
+
+ val ident1 = Identifier.of(Array("ns"), "test_table_1")
+ val ident2 = Identifier.of(Array("ns"), "test_table_2")
+ val ident3 = Identifier.of(Array("ns2"), "test_table_1")
+
+ assert(catalog.listTables(Array("ns")).isEmpty)
+
+ catalog.createTable(ident1, schema, Array.empty, emptyProps)
+
+ assert(catalog.listTables(Array("ns")).toSet == Set(ident1))
+ assert(catalog.listTables(Array("ns2")).isEmpty)
+
+ catalog.createTable(ident3, schema, Array.empty, emptyProps)
+ catalog.createTable(ident2, schema, Array.empty, emptyProps)
+
+ assert(catalog.listTables(Array("ns")).toSet == Set(ident1, ident2))
+ assert(catalog.listTables(Array("ns2")).toSet == Set(ident3))
+
+ catalog.dropTable(ident1)
+
+ assert(catalog.listTables(Array("ns")).toSet == Set(ident2))
+
+ catalog.dropTable(ident2)
+
+ assert(catalog.listTables(Array("ns")).isEmpty)
+ assert(catalog.listTables(Array("ns2")).toSet == Set(ident3))
+
+ catalog.dropTable(ident3)
+ }
+
+ test("createTable") {
+ assert(!catalog.tableExists(testIdent))
+
+ val table = catalog.createTable(testIdent, schema, Array.empty, emptyProps)
+
+ val parsed = CatalystSqlParser.parseMultipartIdentifier(table.name)
+ assert(parsed == Seq("db", "test_table"))
+ assert(table.schema == schema)
+ assert(filterV2TableProperties(table.properties) == Map())
+
+ assert(catalog.tableExists(testIdent))
+ catalog.dropTable(testIdent)
+ }
+
+ test("createTable: with properties") {
+ val properties = new util.HashMap[String, String]()
+ properties.put("property", "value")
+
+ assert(!catalog.tableExists(testIdent))
+
+ val table = catalog.createTable(testIdent, schema, Array.empty, properties)
+
+ val parsed = CatalystSqlParser.parseMultipartIdentifier(table.name)
+ assert(parsed == Seq("db", "test_table"))
+ assert(table.schema == schema)
+ assert(filterV2TableProperties(table.properties).asJava == properties)
+
+ assert(catalog.tableExists(testIdent))
+ catalog.dropTable(testIdent)
+ }
+
+ test("createTable: table already exists") {
+ assert(!catalog.tableExists(testIdent))
+
+ val table = catalog.createTable(testIdent, schema, Array.empty, emptyProps)
+
+ val exc = intercept[TableAlreadyExistsException] {
+ catalog.createTable(testIdent, schema, Array.empty, emptyProps)
+ }
+
+ assert(exc.message.contains(table.name()))
+ assert(exc.message.contains("already exists"))
+
+ assert(catalog.tableExists(testIdent))
+ catalog.dropTable(testIdent)
+ }
+
+ test("tableExists") {
+ assert(!catalog.tableExists(testIdent))
+
+ catalog.createTable(testIdent, schema, Array.empty, emptyProps)
+
+ assert(catalog.tableExists(testIdent))
+
+ catalog.dropTable(testIdent)
+
+ assert(!catalog.tableExists(testIdent))
+ }
+
+ test("createTable: location") {
+ val properties = new util.HashMap[String, String]()
+ assert(!catalog.tableExists(testIdent))
+
+ // default location
+ val t1 = catalog.createTable(testIdent, schema, Array.empty, properties).asInstanceOf[HiveTable]
+ assert(t1.catalogTable.location ===
+ catalog.catalog.defaultTablePath(testIdent.asTableIdentifier))
+ catalog.dropTable(testIdent)
+
+ // relative path
+ properties.put(TableCatalog.PROP_LOCATION, "relative/path")
+ val t2 = catalog.createTable(testIdent, schema, Array.empty, properties).asInstanceOf[HiveTable]
+ assert(t2.catalogTable.location === makeQualifiedPathWithWarehouse("db.db/relative/path"))
+ catalog.dropTable(testIdent)
+
+ // absolute path without scheme
+ properties.put(TableCatalog.PROP_LOCATION, "/absolute/path")
+ val t3 = catalog.createTable(testIdent, schema, Array.empty, properties).asInstanceOf[HiveTable]
+ assert(t3.catalogTable.location.toString === "file:/absolute/path")
Review Comment:
I think this should be Spark and Hive's issue, It [decode uri and toString](https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/ExternalCatalogUtils.scala#L247) via hadoop's `Path`, This will cause problems with toString, for example:
```scala
val location = "file:///a/b"
val uri = new URI(location)
uri.toString -> file:///a/b
new Path(uri).toString -> file:/a/b
```
This resulted in the value stored in the meta database being `file:/a/b`. I tried to solve this problem using `URL.decode` on the spark side and found it also not feasible. It seems that the Hive side also has this problem, but I think the problem is trivial
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] Yikf commented on a diff in pull request #3530: [KYUUBI #3529] Supple DDL tests for Spark Hive connector and fix consistent issues w/ V1 implementation
Posted by GitBox <gi...@apache.org>.
Yikf commented on code in PR #3530:
URL: https://github.com/apache/incubator-kyuubi/pull/3530#discussion_r996313730
##########
extensions/spark/kyuubi-spark-connector-hive/src/test/scala/org/apache/kyuubi/spark/connector/hive/HiveCatalogSuite.scala:
##########
@@ -17,11 +17,83 @@
package org.apache.kyuubi.spark.connector.hive
+import java.net.URI
+import java.util
+import java.util.Collections
+
+import scala.collection.JavaConverters._
+import scala.util.Try
+
+import com.google.common.collect.Maps
+import org.apache.hadoop.fs.Path
import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.analysis.{NoSuchNamespaceException, NoSuchTableException, TableAlreadyExistsException}
+import org.apache.spark.sql.catalyst.parser.CatalystSqlParser
+import org.apache.spark.sql.connector.catalog.{Identifier, TableCatalog}
+import org.apache.spark.sql.hive.kyuubi.connector.HiveBridgeHelper._
+import org.apache.spark.sql.types.{IntegerType, StringType, StructType}
import org.apache.spark.sql.util.CaseInsensitiveStringMap
+import org.apache.kyuubi.spark.connector.hive.HiveTableCatalog.IdentifierHelper
+
class HiveCatalogSuite extends KyuubiHiveTest {
+ val emptyProps: util.Map[String, String] = Collections.emptyMap[String, String]
+ val schema: StructType = new StructType()
+ .add("id", IntegerType)
+ .add("data", StringType)
+
+ val testNs: Array[String] = Array("db")
+ val defaultNs: Array[String] = Array("default")
+ val testIdent: Identifier = Identifier.of(testNs, "test_table")
+
+ var catalog: HiveTableCatalog = _
+
+ private def newCatalog(): HiveTableCatalog = {
+ val catalog = new HiveTableCatalog
+ val catalogName = "hive"
+ val properties = Maps.newHashMap[String, String]()
+ properties.put("javax.jdo.option.ConnectionURL", "jdbc:derby:memory:memorydb;create=true")
+ properties.put("javax.jdo.option.ConnectionDriverName", "org.apache.derby.jdbc.EmbeddedDriver")
+ catalog.initialize(catalogName, new CaseInsensitiveStringMap(properties))
+ catalog
+ }
+
+ private def filterV2TableProperties(
+ properties: util.Map[String, String]): Map[String, String] = {
+ properties.asScala.filter(kv => !TABLE_RESERVED_PROPERTIES.contains(kv._1))
+ .filter(!_._1.startsWith(TableCatalog.OPTION_PREFIX)).toMap
+ }
+
+ def makeQualifiedPathWithWarehouse(path: String): URI = {
+ val p = new Path(catalog.conf.warehousePath, path)
+ val fs = p.getFileSystem(catalog.hadoopConfiguration())
+ fs.makeQualified(p).toUri
+ }
+
+ private def checkMetadata(expected: Map[String, String], actual: Map[String, String]): Unit = {
+ // remove location and comment that are automatically added by HMS unless they are expected
+ val toRemove =
+ NAMESPACE_RESERVED_PROPERTIES.filter(expected.contains)
+ assert(expected -- toRemove === actual)
+ }
+
+ override def beforeEach(): Unit = {
Review Comment:
Thanks @pan3793
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 closed pull request #3530: [KYUUBI #3529] Supple DDL tests for Spark Hive connector and fix consistent issues w/ V1 implementation
Posted by GitBox <gi...@apache.org>.
pan3793 closed pull request #3530: [KYUUBI #3529] Supple DDL tests for Spark Hive connector and fix consistent issues w/ V1 implementation
URL: https://github.com/apache/incubator-kyuubi/pull/3530
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #3530: [KYUUBI #3529] [TEST] Add tests to check for consistency with V1 in hive connector
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3530:
URL: https://github.com/apache/incubator-kyuubi/pull/3530#issuecomment-1272509391
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/3530?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3530](https://codecov.io/gh/apache/incubator-kyuubi/pull/3530?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cfbef99) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/73612b7750264b6d43e48ee0277a89bd9de0b82a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (73612b7) will **decrease** coverage by `0.01%`.
> The diff coverage is `n/a`.
> :exclamation: Current head cfbef99 differs from pull request most recent head 2e83d83. Consider uploading reports for the commit 2e83d83 to get more accurate results
```diff
@@ Coverage Diff @@
## master #3530 +/- ##
============================================
- Coverage 51.67% 51.65% -0.02%
Complexity 13 13
============================================
Files 482 482
Lines 26933 26933
Branches 3760 3760
============================================
- Hits 13917 13912 -5
- Misses 11661 11665 +4
- Partials 1355 1356 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/3530?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...apache/kyuubi/engine/JpsApplicationOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3530/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvSnBzQXBwbGljYXRpb25PcGVyYXRpb24uc2NhbGE=) | `77.41% <0.00%> (-3.23%)` | :arrow_down: |
| [...rg/apache/kyuubi/ctl/cmd/log/LogBatchCommand.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3530/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWN0bC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jdGwvY21kL2xvZy9Mb2dCYXRjaENvbW1hbmQuc2NhbGE=) | `78.00% <0.00%> (-2.00%)` | :arrow_down: |
| [...che/kyuubi/server/KyuubiTHttpFrontendService.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3530/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpVEh0dHBGcm9udGVuZFNlcnZpY2Uuc2NhbGE=) | `64.23% <0.00%> (-0.73%)` | :arrow_down: |
| [...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3530/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvY0J1aWxkZXIuc2NhbGE=) | `80.74% <0.00%> (-0.63%)` | :arrow_down: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3530/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `97.23% <0.00%> (-0.08%)` | :arrow_down: |
| [...g/apache/kyuubi/operation/BatchJobSubmission.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3530/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vQmF0Y2hKb2JTdWJtaXNzaW9uLnNjYWxh) | `74.64% <0.00%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3530: [KYUUBI #3529] [TEST] Supplement the tests related namespace, showtables w/ Initialize the DDL test framework and fix minor consistency issue with v1 for hive connector
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3530:
URL: https://github.com/apache/incubator-kyuubi/pull/3530#discussion_r996249400
##########
extensions/spark/kyuubi-spark-connector-hive/src/test/scala/org/apache/kyuubi/spark/connector/hive/HiveCatalogSuite.scala:
##########
@@ -51,4 +124,203 @@ class HiveCatalogSuite extends KyuubiHiveTest {
assert(exception.message === "Table or view not found: hive.ns1.nonexistent_table")
}
}
+
+ test("listTables") {
+
+ val ident1 = Identifier.of(Array("ns"), "test_table_1")
+ val ident2 = Identifier.of(Array("ns"), "test_table_2")
+ val ident3 = Identifier.of(Array("ns2"), "test_table_1")
+
+ assert(catalog.listTables(Array("ns")).isEmpty)
+
+ catalog.createTable(ident1, schema, Array.empty, emptyProps)
+
+ assert(catalog.listTables(Array("ns")).toSet == Set(ident1))
+ assert(catalog.listTables(Array("ns2")).isEmpty)
+
+ catalog.createTable(ident3, schema, Array.empty, emptyProps)
+ catalog.createTable(ident2, schema, Array.empty, emptyProps)
+
+ assert(catalog.listTables(Array("ns")).toSet == Set(ident1, ident2))
+ assert(catalog.listTables(Array("ns2")).toSet == Set(ident3))
+
+ catalog.dropTable(ident1)
+
+ assert(catalog.listTables(Array("ns")).toSet == Set(ident2))
+
+ catalog.dropTable(ident2)
+
+ assert(catalog.listTables(Array("ns")).isEmpty)
+ assert(catalog.listTables(Array("ns2")).toSet == Set(ident3))
+
+ catalog.dropTable(ident3)
+ }
+
+ test("createTable") {
+ assert(!catalog.tableExists(testIdent))
+
+ val table = catalog.createTable(testIdent, schema, Array.empty, emptyProps)
+
+ val parsed = CatalystSqlParser.parseMultipartIdentifier(table.name)
+ assert(parsed == Seq("db", "test_table"))
+ assert(table.schema == schema)
+ assert(filterV2TableProperties(table.properties) == Map())
+
+ assert(catalog.tableExists(testIdent))
+ catalog.dropTable(testIdent)
+ }
+
+ test("createTable: with properties") {
+ val properties = new util.HashMap[String, String]()
+ properties.put("property", "value")
+
+ assert(!catalog.tableExists(testIdent))
+
+ val table = catalog.createTable(testIdent, schema, Array.empty, properties)
+
+ val parsed = CatalystSqlParser.parseMultipartIdentifier(table.name)
+ assert(parsed == Seq("db", "test_table"))
+ assert(table.schema == schema)
+ assert(filterV2TableProperties(table.properties).asJava == properties)
+
+ assert(catalog.tableExists(testIdent))
+ catalog.dropTable(testIdent)
+ }
+
+ test("createTable: table already exists") {
+ assert(!catalog.tableExists(testIdent))
+
+ val table = catalog.createTable(testIdent, schema, Array.empty, emptyProps)
+
+ val exc = intercept[TableAlreadyExistsException] {
+ catalog.createTable(testIdent, schema, Array.empty, emptyProps)
+ }
+
+ assert(exc.message.contains(table.name()))
+ assert(exc.message.contains("already exists"))
+
+ assert(catalog.tableExists(testIdent))
+ catalog.dropTable(testIdent)
+ }
+
+ test("tableExists") {
+ assert(!catalog.tableExists(testIdent))
+
+ catalog.createTable(testIdent, schema, Array.empty, emptyProps)
+
+ assert(catalog.tableExists(testIdent))
+
+ catalog.dropTable(testIdent)
+
+ assert(!catalog.tableExists(testIdent))
+ }
+
+ test("createTable: location") {
+ val properties = new util.HashMap[String, String]()
+ assert(!catalog.tableExists(testIdent))
+
+ // default location
+ val t1 = catalog.createTable(testIdent, schema, Array.empty, properties).asInstanceOf[HiveTable]
+ assert(t1.catalogTable.location ===
+ catalog.catalog.defaultTablePath(testIdent.asTableIdentifier))
+ catalog.dropTable(testIdent)
+
+ // relative path
+ properties.put(TableCatalog.PROP_LOCATION, "relative/path")
+ val t2 = catalog.createTable(testIdent, schema, Array.empty, properties).asInstanceOf[HiveTable]
+ assert(t2.catalogTable.location === makeQualifiedPathWithWarehouse("db.db/relative/path"))
+ catalog.dropTable(testIdent)
+
+ // absolute path without scheme
+ properties.put(TableCatalog.PROP_LOCATION, "/absolute/path")
+ val t3 = catalog.createTable(testIdent, schema, Array.empty, properties).asInstanceOf[HiveTable]
+ assert(t3.catalogTable.location.toString === "file:/absolute/path")
Review Comment:
Thanks for the explanation, it makes sense if it is consistent w/ the V1 implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3530: [KYUUBI #3529] Supple DDL tests for Spark Hive connector and fix consistent issues w/ V1 implementation
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3530:
URL: https://github.com/apache/incubator-kyuubi/pull/3530#issuecomment-1279759609
Thanks, merging to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] Yikf commented on pull request #3530: [KYUUBI #3529] Add tests to check for consistency with V1 in hive connector
Posted by GitBox <gi...@apache.org>.
Yikf commented on PR #3530:
URL: https://github.com/apache/incubator-kyuubi/pull/3530#issuecomment-1252325276
Could you please take a look when you have a time, thans @pan3793
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] Yikf commented on a diff in pull request #3530: [KYUUBI #3529] [TEST] Supplement the tests related namespace, showtables w/ Initialize the DDL test framework and fix minor consistency issue with v1 for hive connector
Posted by GitBox <gi...@apache.org>.
Yikf commented on code in PR #3530:
URL: https://github.com/apache/incubator-kyuubi/pull/3530#discussion_r994639031
##########
extensions/spark/kyuubi-spark-connector-hive/src/test/scala/org/apache/kyuubi/spark/connector/hive/command/CreateNamespaceSuite.scala:
##########
@@ -0,0 +1,139 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.spark.connector.hive.command
+
+import scala.collection.JavaConverters._
+import scala.util.Try
+
+import org.apache.hadoop.fs.Path
+import org.apache.spark.sql.catalyst.analysis.NamespaceAlreadyExistsException
+import org.apache.spark.sql.catalyst.parser.ParseException
+import org.apache.spark.sql.connector.catalog.SupportsNamespaces
+import org.apache.spark.sql.internal.SQLConf
+
+import org.apache.kyuubi.spark.connector.hive.command.DDLCommandTestUtils.{V1_COMMAND_VERSION, V2_COMMAND_VERSION}
+
+trait CreateNamespaceSuiteBase extends DDLCommandTestUtils {
+ override protected def command: String = "CREATE NAMESPACE"
+
+ protected def namespaceArray: Array[String] = namespace.split('.')
+
+ protected def notFoundMsgPrefix: String =
+ if (commandVersion == V1_COMMAND_VERSION) "Database" else "Namespace"
+
+ private def namespace: String = "fakens"
+
+ override def afterEach(): Unit = {
+ sql(s"DROP NAMESPACE IF EXISTS $catalogName.$namespace")
+ super.afterEach()
+ }
+
+ test("basic test") {
+ val ns = s"$catalogName.$namespace"
+ withNamespace(ns) {
+ sql(s"CREATE NAMESPACE $ns")
+ assert(getCatalog(catalogName).asNamespaceCatalog.namespaceExists(namespaceArray))
+ }
+ }
+
+ test("namespace with location") {
+ val ns = s"$catalogName.$namespace"
+ withNamespace(ns) {
+ withTempDir { tmpDir =>
+ // The generated temp path is not qualified.
+ val path = tmpDir.getCanonicalPath
+ assert(!path.startsWith("file:/"))
+
+ val e = intercept[IllegalArgumentException] {
+ sql(s"CREATE NAMESPACE $ns LOCATION ''")
+ }
+ assert(e.getMessage.contains("Can not create a Path from an empty string"))
+
+ val uri = new Path(path).toUri
+ sql(s"CREATE NAMESPACE $ns LOCATION '$uri'")
+
+ // Make sure the location is qualified.
+ val expected = makeQualifiedPath(tmpDir.toString)
+ assert("file" === expected.getScheme)
+ assert(new Path(getNamespaceLocation(catalogName, namespaceArray)).toUri === expected)
+ }
+ }
+ }
+
+ test("Namespace already exists") {
+ val ns = s"$catalogName.$namespace"
+ withNamespace(ns) {
+ sql(s"CREATE NAMESPACE $ns")
+
+ val e = intercept[NamespaceAlreadyExistsException] {
+ sql(s"CREATE NAMESPACE $ns")
+ }
+ assert(e.getMessage.contains(s"$notFoundMsgPrefix '$namespace' already exists"))
+
+ // The following will be no-op since the namespace already exists.
+ Try { sql(s"CREATE NAMESPACE IF NOT EXISTS $ns") }.isSuccess
+ }
+ }
+
+ test("CreateNameSpace: reserved properties") {
+ import SupportsNamespaces._
+ val ns = s"$catalogName.$namespace"
+ withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "false")) {
+ NAMESPACE_RESERVED_PROPERTIES.filterNot(_ == PROP_COMMENT).foreach { key =>
+ val exception = intercept[ParseException] {
+ sql(s"CREATE NAMESPACE $ns WITH DBPROPERTIES('$key'='dummyVal')")
+ }
+ assert(exception.getMessage.contains(s"$key is a reserved namespace property"))
+ }
+ }
+ withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "true")) {
+ NAMESPACE_RESERVED_PROPERTIES.filterNot(_ == PROP_COMMENT).foreach { key =>
+ withNamespace(ns) {
+ sql(s"CREATE NAMESPACE $ns WITH DBPROPERTIES('$key'='foo')")
+ assert(
+ sql(s"DESC NAMESPACE EXTENDED $ns")
+ .toDF("k", "v")
+ .where("k='Properties'")
+ .where("v=''")
+ .count == 1,
+ s"$key is a reserved namespace property and ignored")
+ val meta =
+ getCatalog(catalogName).asNamespaceCatalog.loadNamespaceMetadata(namespaceArray)
+ assert(
+ meta.get(key) == null || !meta.get(key).contains("foo"),
+ "reserved properties should not have side effects")
+ }
+ }
+ }
+ }
+
+ protected def getNamespaceLocation(catalog: String, namespace: Array[String]): String = {
+ val metadata = getCatalog(catalog).asNamespaceCatalog
+ .loadNamespaceMetadata(namespace).asScala
+ metadata(SupportsNamespaces.PROP_LOCATION)
+ }
+}
+
+class CreateNamespaceSuite extends CreateNamespaceSuiteBase {
Review Comment:
Sure
Sorry for late reply, I'm kind of busy this week.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3530: [KYUUBI #3529] [TEST] Supplement the tests related namespace, showtables w/ Initialize the DDL test framework and fix minor consistency issue with v1 for hive connector
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3530:
URL: https://github.com/apache/incubator-kyuubi/pull/3530#discussion_r990933605
##########
extensions/spark/kyuubi-spark-connector-hive/src/test/scala/org/apache/kyuubi/spark/connector/hive/command/CreateNamespaceSuite.scala:
##########
@@ -0,0 +1,139 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.spark.connector.hive.command
+
+import scala.collection.JavaConverters._
+import scala.util.Try
+
+import org.apache.hadoop.fs.Path
+import org.apache.spark.sql.catalyst.analysis.NamespaceAlreadyExistsException
+import org.apache.spark.sql.catalyst.parser.ParseException
+import org.apache.spark.sql.connector.catalog.SupportsNamespaces
+import org.apache.spark.sql.internal.SQLConf
+
+import org.apache.kyuubi.spark.connector.hive.command.DDLCommandTestUtils.{V1_COMMAND_VERSION, V2_COMMAND_VERSION}
+
+trait CreateNamespaceSuiteBase extends DDLCommandTestUtils {
+ override protected def command: String = "CREATE NAMESPACE"
+
+ protected def namespaceArray: Array[String] = namespace.split('.')
+
+ protected def notFoundMsgPrefix: String =
+ if (commandVersion == V1_COMMAND_VERSION) "Database" else "Namespace"
+
+ private def namespace: String = "fakens"
+
+ override def afterEach(): Unit = {
+ sql(s"DROP NAMESPACE IF EXISTS $catalogName.$namespace")
+ super.afterEach()
+ }
+
+ test("basic test") {
+ val ns = s"$catalogName.$namespace"
+ withNamespace(ns) {
+ sql(s"CREATE NAMESPACE $ns")
+ assert(getCatalog(catalogName).asNamespaceCatalog.namespaceExists(namespaceArray))
+ }
+ }
+
+ test("namespace with location") {
+ val ns = s"$catalogName.$namespace"
+ withNamespace(ns) {
+ withTempDir { tmpDir =>
+ // The generated temp path is not qualified.
+ val path = tmpDir.getCanonicalPath
+ assert(!path.startsWith("file:/"))
+
+ val e = intercept[IllegalArgumentException] {
+ sql(s"CREATE NAMESPACE $ns LOCATION ''")
+ }
+ assert(e.getMessage.contains("Can not create a Path from an empty string"))
+
+ val uri = new Path(path).toUri
+ sql(s"CREATE NAMESPACE $ns LOCATION '$uri'")
+
+ // Make sure the location is qualified.
+ val expected = makeQualifiedPath(tmpDir.toString)
+ assert("file" === expected.getScheme)
+ assert(new Path(getNamespaceLocation(catalogName, namespaceArray)).toUri === expected)
+ }
+ }
+ }
+
+ test("Namespace already exists") {
+ val ns = s"$catalogName.$namespace"
+ withNamespace(ns) {
+ sql(s"CREATE NAMESPACE $ns")
+
+ val e = intercept[NamespaceAlreadyExistsException] {
+ sql(s"CREATE NAMESPACE $ns")
+ }
+ assert(e.getMessage.contains(s"$notFoundMsgPrefix '$namespace' already exists"))
+
+ // The following will be no-op since the namespace already exists.
+ Try { sql(s"CREATE NAMESPACE IF NOT EXISTS $ns") }.isSuccess
+ }
+ }
+
+ test("CreateNameSpace: reserved properties") {
+ import SupportsNamespaces._
+ val ns = s"$catalogName.$namespace"
+ withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "false")) {
+ NAMESPACE_RESERVED_PROPERTIES.filterNot(_ == PROP_COMMENT).foreach { key =>
+ val exception = intercept[ParseException] {
+ sql(s"CREATE NAMESPACE $ns WITH DBPROPERTIES('$key'='dummyVal')")
+ }
+ assert(exception.getMessage.contains(s"$key is a reserved namespace property"))
+ }
+ }
+ withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "true")) {
+ NAMESPACE_RESERVED_PROPERTIES.filterNot(_ == PROP_COMMENT).foreach { key =>
+ withNamespace(ns) {
+ sql(s"CREATE NAMESPACE $ns WITH DBPROPERTIES('$key'='foo')")
+ assert(
+ sql(s"DESC NAMESPACE EXTENDED $ns")
+ .toDF("k", "v")
+ .where("k='Properties'")
+ .where("v=''")
+ .count == 1,
+ s"$key is a reserved namespace property and ignored")
+ val meta =
+ getCatalog(catalogName).asNamespaceCatalog.loadNamespaceMetadata(namespaceArray)
+ assert(
+ meta.get(key) == null || !meta.get(key).contains("foo"),
+ "reserved properties should not have side effects")
+ }
+ }
+ }
+ }
+
+ protected def getNamespaceLocation(catalog: String, namespace: Array[String]): String = {
+ val metadata = getCatalog(catalog).asNamespaceCatalog
+ .loadNamespaceMetadata(namespace).asScala
+ metadata(SupportsNamespaces.PROP_LOCATION)
+ }
+}
+
+class CreateNamespaceSuite extends CreateNamespaceSuiteBase {
Review Comment:
Thanks for building this framework, the purpose is to make sure that the V2 implementation should work as same(or have meaningful different behaviors) as the Spark built-in one, could you please test that as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3530: [KYUUBI #3529] [TEST] Supplement the tests related namespace, showtables w/ Initialize the DDL test framework and fix minor consistency issue with v1 for hive connector
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3530:
URL: https://github.com/apache/incubator-kyuubi/pull/3530#discussion_r990932633
##########
extensions/spark/kyuubi-spark-connector-hive/src/test/scala/org/apache/kyuubi/spark/connector/hive/HiveCatalogSuite.scala:
##########
@@ -17,11 +17,85 @@
package org.apache.kyuubi.spark.connector.hive
+import java.net.URI
+import java.util
+import java.util.Collections
+
+import scala.collection.JavaConverters._
+import scala.util.Try
+
+import com.google.common.collect.Maps
+import org.apache.hadoop.fs.Path
import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.analysis.{NoSuchNamespaceException, NoSuchTableException, TableAlreadyExistsException}
+import org.apache.spark.sql.catalyst.parser.CatalystSqlParser
+import org.apache.spark.sql.connector.catalog.{Identifier, TableCatalog}
+import org.apache.spark.sql.hive.kyuubi.connector.HiveBridgeHelper._
+import org.apache.spark.sql.types.{IntegerType, StringType, StructType}
import org.apache.spark.sql.util.CaseInsensitiveStringMap
+import org.apache.kyuubi.spark.connector.hive.HiveTableCatalog.IdentifierHelper
+
class HiveCatalogSuite extends KyuubiHiveTest {
+ val emptyProps: util.Map[String, String] = Collections.emptyMap[String, String]
+ val schema: StructType = new StructType()
+ .add("id", IntegerType)
+ .add("data", StringType)
+
+ val testNs: Array[String] = Array("db")
+ val defaultNs: Array[String] = Array("default")
+ val testIdent: Identifier = Identifier.of(testNs, "test_table")
+
+ var catalog: HiveTableCatalog = _
+
+ private def newCatalog(): HiveTableCatalog = {
+ val catalog = new HiveTableCatalog
+ val catalogName = "hive"
+ val properties = Maps.newHashMap[String, String]()
+ properties.put("javax.jdo.option.ConnectionURL", "jdbc:derby:memory:memorydb;create=true")
+ properties.put("javax.jdo.option.ConnectionDriverName", "org.apache.derby.jdbc.EmbeddedDriver")
+ catalog.initialize(catalogName, new CaseInsensitiveStringMap(properties))
+ catalog
+ }
+
+ private def filterV2TableProperties(
+ properties: util.Map[String, String]): Map[String, String] = {
+ properties.asScala.filter(kv => !TABLE_RESERVED_PROPERTIES.contains(kv._1))
+ .filter(!_._1.startsWith(TableCatalog.OPTION_PREFIX)).toMap
+ }
+
+ def makeQualifiedPathWithWarehouse(path: String): URI = {
+ val p = new Path(catalog.conf.warehousePath, path)
+ val fs = p.getFileSystem(catalog.hadoopConfiguration())
+ fs.makeQualified(p).toUri
+ }
+
+ private def checkMetadata(
+ expected: scala.collection.Map[String, String],
Review Comment:
`scala.collection.Map` => `Map`(prefered) or `mutable.Map`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3530: [KYUUBI #3529] [TEST] Supplement the tests related namespace, showtables w/ Initialize the DDL test framework and fix minor consistency issue with v1 for hive connector
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3530:
URL: https://github.com/apache/incubator-kyuubi/pull/3530#discussion_r996250662
##########
extensions/spark/kyuubi-spark-connector-hive/src/test/scala/org/apache/kyuubi/spark/connector/hive/HiveCatalogSuite.scala:
##########
@@ -17,11 +17,83 @@
package org.apache.kyuubi.spark.connector.hive
+import java.net.URI
+import java.util
+import java.util.Collections
+
+import scala.collection.JavaConverters._
+import scala.util.Try
+
+import com.google.common.collect.Maps
+import org.apache.hadoop.fs.Path
import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.analysis.{NoSuchNamespaceException, NoSuchTableException, TableAlreadyExistsException}
+import org.apache.spark.sql.catalyst.parser.CatalystSqlParser
+import org.apache.spark.sql.connector.catalog.{Identifier, TableCatalog}
+import org.apache.spark.sql.hive.kyuubi.connector.HiveBridgeHelper._
+import org.apache.spark.sql.types.{IntegerType, StringType, StructType}
import org.apache.spark.sql.util.CaseInsensitiveStringMap
+import org.apache.kyuubi.spark.connector.hive.HiveTableCatalog.IdentifierHelper
+
class HiveCatalogSuite extends KyuubiHiveTest {
+ val emptyProps: util.Map[String, String] = Collections.emptyMap[String, String]
+ val schema: StructType = new StructType()
+ .add("id", IntegerType)
+ .add("data", StringType)
+
+ val testNs: Array[String] = Array("db")
+ val defaultNs: Array[String] = Array("default")
+ val testIdent: Identifier = Identifier.of(testNs, "test_table")
+
+ var catalog: HiveTableCatalog = _
+
+ private def newCatalog(): HiveTableCatalog = {
+ val catalog = new HiveTableCatalog
+ val catalogName = "hive"
+ val properties = Maps.newHashMap[String, String]()
+ properties.put("javax.jdo.option.ConnectionURL", "jdbc:derby:memory:memorydb;create=true")
+ properties.put("javax.jdo.option.ConnectionDriverName", "org.apache.derby.jdbc.EmbeddedDriver")
+ catalog.initialize(catalogName, new CaseInsensitiveStringMap(properties))
+ catalog
+ }
+
+ private def filterV2TableProperties(
+ properties: util.Map[String, String]): Map[String, String] = {
+ properties.asScala.filter(kv => !TABLE_RESERVED_PROPERTIES.contains(kv._1))
+ .filter(!_._1.startsWith(TableCatalog.OPTION_PREFIX)).toMap
+ }
+
+ def makeQualifiedPathWithWarehouse(path: String): URI = {
+ val p = new Path(catalog.conf.warehousePath, path)
+ val fs = p.getFileSystem(catalog.hadoopConfiguration())
+ fs.makeQualified(p).toUri
+ }
+
+ private def checkMetadata(expected: Map[String, String], actual: Map[String, String]): Unit = {
+ // remove location and comment that are automatically added by HMS unless they are expected
+ val toRemove =
+ NAMESPACE_RESERVED_PROPERTIES.filter(expected.contains)
+ assert(expected -- toRemove === actual)
+ }
+
+ override def beforeEach(): Unit = {
Review Comment:
usually, the pattern should be symmetrical, e.g.
```
def beforeEach(): Unit = {
super.beforeEach()
...
}
def afterEach(): Unit = {
...
super.afterEach()
}
```
or
```
def beforeEach(): Unit = {
...
super.beforeEach()
}
def afterEach(): Unit = {
super.afterEach()
...
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3530: [KYUUBI #3529] [TEST] Supplement the tests related namespace, showtables w/ Initialize the DDL test framework and fix minor consistency issue with v1 for hive connector
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3530:
URL: https://github.com/apache/incubator-kyuubi/pull/3530#discussion_r996250662
##########
extensions/spark/kyuubi-spark-connector-hive/src/test/scala/org/apache/kyuubi/spark/connector/hive/HiveCatalogSuite.scala:
##########
@@ -17,11 +17,83 @@
package org.apache.kyuubi.spark.connector.hive
+import java.net.URI
+import java.util
+import java.util.Collections
+
+import scala.collection.JavaConverters._
+import scala.util.Try
+
+import com.google.common.collect.Maps
+import org.apache.hadoop.fs.Path
import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.analysis.{NoSuchNamespaceException, NoSuchTableException, TableAlreadyExistsException}
+import org.apache.spark.sql.catalyst.parser.CatalystSqlParser
+import org.apache.spark.sql.connector.catalog.{Identifier, TableCatalog}
+import org.apache.spark.sql.hive.kyuubi.connector.HiveBridgeHelper._
+import org.apache.spark.sql.types.{IntegerType, StringType, StructType}
import org.apache.spark.sql.util.CaseInsensitiveStringMap
+import org.apache.kyuubi.spark.connector.hive.HiveTableCatalog.IdentifierHelper
+
class HiveCatalogSuite extends KyuubiHiveTest {
+ val emptyProps: util.Map[String, String] = Collections.emptyMap[String, String]
+ val schema: StructType = new StructType()
+ .add("id", IntegerType)
+ .add("data", StringType)
+
+ val testNs: Array[String] = Array("db")
+ val defaultNs: Array[String] = Array("default")
+ val testIdent: Identifier = Identifier.of(testNs, "test_table")
+
+ var catalog: HiveTableCatalog = _
+
+ private def newCatalog(): HiveTableCatalog = {
+ val catalog = new HiveTableCatalog
+ val catalogName = "hive"
+ val properties = Maps.newHashMap[String, String]()
+ properties.put("javax.jdo.option.ConnectionURL", "jdbc:derby:memory:memorydb;create=true")
+ properties.put("javax.jdo.option.ConnectionDriverName", "org.apache.derby.jdbc.EmbeddedDriver")
+ catalog.initialize(catalogName, new CaseInsensitiveStringMap(properties))
+ catalog
+ }
+
+ private def filterV2TableProperties(
+ properties: util.Map[String, String]): Map[String, String] = {
+ properties.asScala.filter(kv => !TABLE_RESERVED_PROPERTIES.contains(kv._1))
+ .filter(!_._1.startsWith(TableCatalog.OPTION_PREFIX)).toMap
+ }
+
+ def makeQualifiedPathWithWarehouse(path: String): URI = {
+ val p = new Path(catalog.conf.warehousePath, path)
+ val fs = p.getFileSystem(catalog.hadoopConfiguration())
+ fs.makeQualified(p).toUri
+ }
+
+ private def checkMetadata(expected: Map[String, String], actual: Map[String, String]): Unit = {
+ // remove location and comment that are automatically added by HMS unless they are expected
+ val toRemove =
+ NAMESPACE_RESERVED_PROPERTIES.filter(expected.contains)
+ assert(expected -- toRemove === actual)
+ }
+
+ override def beforeEach(): Unit = {
Review Comment:
usually, the pattern should be symmetrical, e.g.
```
def beforeEach(): Unit = {
super.beforeEach()
...
}
def afterEach(): Unit = {
...
super.afterEach()
}
```
or
```
def beforeEach(): Unit = {
...
super.beforeEach()
}
def afterEach(): Unit = {
super.afterEach()
...
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3530: [KYUUBI #3529] [TEST] Add tests to check for consistency with V1 in hive connector
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3530:
URL: https://github.com/apache/incubator-kyuubi/pull/3530#discussion_r984230540
##########
extensions/spark/kyuubi-spark-connector-hive/src/main/scala/org/apache/kyuubi/spark/connector/hive/HiveTable.scala:
##########
@@ -59,7 +59,7 @@ case class HiveTable(
catalogTable.stats.map(_.sizeInBytes.toLong).getOrElse(defaultTableSize))
}
- override def name(): String = catalogTable.identifier.table
+ override def name(): String = catalogTable.identifier.unquotedString
Review Comment:
please separate the change if this is a test only PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org