[#3292] fix(spark-connector): passing Gravitino catalog properties to spark connector #3270

FANNG1 · 2024-05-06T00:34:43Z

What changes were proposed in this pull request?

passing Gravitino catalog properties with spark.bypass. prefix to spark connector
refactor the catalog properties transform logic.

Why are the changes needed?

Fix: #3292

Does this PR introduce any user-facing change?

yes, add document

How was this patch tested?

add UT
test in local envriment

FANNG1 · 2024-05-08T00:29:19Z

@jerryshao @qqqttt123 @yuqi1129 @diqiu50 @caican00 please help to review this PR when you have time, thanks

FANNG1 · 2024-05-08T02:09:33Z

KyuubiHiveConnectorDelegationTokenProvider needs to inject catalog properties to spark conf, but I'm not sure does Gravitino provide a self managed token provider or reuse the kyuubi token provider. @qqqttt123

diqiu50 · 2024-05-08T15:05:59Z

...c/main/java/com/datastrato/gravitino/spark/connector/iceberg/IcebergPropertiesConstants.java

+  static final String ICEBERG_CATALOG_BACKEND_HIVE = CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE;
+
+  static final String GRAVITINO_ICEBERG_CATALOG_BACKEND_JDBC = "jdbc";
+  static final String ICEBERG_CATALOG_BACKEND_JDBC = "jdbc";

  private IcebergPropertiesConstants() {}
 }


How to distinguish which constants here are for the catalog and which are for the table?

they are all catalog properties

You suggest to add CATALOG to all property names?

If that, the class name IcebergCatalogPropertiesConstants is better

I prefer to keep current file name, because it maybe support table properties constant.

That works; find a way to distinguish them.

That works; find a way to distinguish them.

added CATALOG to property names.

qqqttt123 · 2024-05-09T06:14:08Z

KyuubiHiveConnectorDelegationTokenProvider needs to inject catalog properties to spark conf, but I'm not sure does Gravitino provide a self managed token provider or reuse the kyuubi token provider. @qqqttt123

Prefer self-managed tokenProvider. Because the tokenProvider will be used for Iceberg, Hive, Paimon.

FANNG1 · 2024-05-10T00:27:38Z

self-managed tokenProvider

Does self-managed tokenProvider needs catalog properties from spark conf? If not, I will keep current implement and not inject catalog properties to spark conf.

jerryshao · 2024-05-10T03:57:17Z

docs/apache-hive-catalog.md

@@ -42,6 +42,9 @@ The Hive catalog supports creating, updating, and deleting databases and tables

 When you use the Gravitino with Trino. You can pass the Trino Hive connector configuration using prefix `trino.bypass.`. For example, using `trino.bypass.hive.config.resources` to pass the `hive.config.resources` to the Gravitino Hive catalog in Trino runtime.

+When you use the Gravitino with Spark. You can pass the Spark Hive connector configuration using prefix `spark.bypass.`. For example, using `spark.bypass.hive.config.resources` to pass the `hive.config.resources` to the Spark Hive connector in Spark runtime.


Are you sure these two configs spark.bypass.hive.config.resources and hive.config.resources works in Spark?

hive.config.resources is just a config example, change it to a real config like hive.exec.dynamic.partition.mode?

Why do you use a non-existent config as an example, this is really confusing. Please think of as a user who reads your document, does he really understand what you want to describe?

got it, fixed

jerryshao · 2024-05-10T03:58:04Z

docs/lakehouse-iceberg-catalog.md

@@ -43,6 +43,9 @@ Any properties not defined by Gravitino with `gravitino.bypass.` prefix will pas

 When you use the Gravitino with Trino. You can pass the Trino Iceberg connector configuration using prefix `trino.bypass.`. For example, using `trino.bypass.iceberg.table-statistics-enabled` to pass the `iceberg.table-statistics-enabled` to the Gravitino Iceberg catalog in Trino runtime.

+When you use the Gravitino with Spark. You can pass the Spark Iceberg connector configuration using prefix `spark.bypass.`. For example, using `spark.bypass.iceberg.config.resources` to pass the `iceberg.config.resources` to the Spark Iceberg connector in Spark runtime.


jerryshao · 2024-05-10T04:02:27Z

docs/spark-connector/spark-catalog-hive.md

+|---------------------------------|-----------------------------|----------------------------|---------------|
+| `metastore.uris`                | `hive.metastore.uris`       | Hive metastore uri address | 0.5.0         |
+
+Catalog properties with prefix `spark.bypass.` are passed to Spark Hive connector. For example, using `spark.bypass.config.resources` to pass the `config.resources` to the Spark Hive connector.


So should we use "Gravitino catalog property name" or "Spark catalog property name", you bring in new concepts without any explanation, how do users use it?

replace with Gravitino catalog property names

Again, as I mentioned here, you add two tables in here and iceberg for a bunch of properties without explaining what are they and how user uses them, so what actually do you want to deliver to users?

add some description, please help to review again

jerryshao · 2024-05-10T04:06:57Z

...rk-connector/src/main/java/com/datastrato/gravitino/spark/connector/PropertiesConverter.java

+   * @param properties Gravitino catalog properties.
+   * @return properties for the Spark connector.
+   */
+  Map<String, String> transformSparkCatalogProperties(Map<String, String> properties);


What is the meaning of transform, why don't you use toXXX to align with below interfaces if the semantics are the same.

jerryshao · 2024-05-10T04:09:49Z

...nector/src/main/java/com/datastrato/gravitino/spark/connector/hive/GravitinoHiveCatalog.java

-    HashMap<String, String> all = new HashMap<>(options);
-    all.put(GravitinoSparkConfig.SPARK_HIVE_METASTORE_URI, metastoreUri);
+    Map<String, String> all =
+        HivePropertiesConverter.getInstance().toSparkCatalogProperties(options, properties);


Why don't you use getPropertiesConverter().toXXX here?

FANNG1 · 2024-05-11T09:01:55Z

@jerryshao @diqiu50 , all comments are addressed, please help review again

jerryshao · 2024-05-11T10:36:21Z

...rk-connector/src/main/java/com/datastrato/gravitino/spark/connector/PropertiesConverter.java

+  /**
+   * Transform properties from Gravitino catalog properties to Spark connector properties.
+   *
+   * <p>This interface focus on the catalog specific transform logic, the common logic are


"This interface focuses..."

jerryshao · 2024-05-11T10:45:24Z

@FANNG1 just one minor issue, please fix it.

FANNG1 · 2024-05-11T12:28:42Z

@FANNG1 just one minor issue, please fix it.

fixed

FANNG1 · 2024-05-13T02:25:17Z

@jerryshao could you help to review again? thanks

… spark connector (#3270) ### What changes were proposed in this pull request? 1. passing Gravitino catalog properties with `spark.bypass.` prefix to spark connector 2. refactor the catalog properties transform logic. ### Why are the changes needed? Fix: #3292 ### Does this PR introduce _any_ user-facing change? yes, add document ### How was this patch tested? 1. add UT 3. test in local envriment

…ies to spark connector (apache#3270) ### What changes were proposed in this pull request? 1. passing Gravitino catalog properties with `spark.bypass.` prefix to spark connector 2. refactor the catalog properties transform logic. ### Why are the changes needed? Fix: apache#3292 ### Does this PR introduce _any_ user-facing change? yes, add document ### How was this patch tested? 1. add UT 3. test in local envriment

FANNG1 marked this pull request as draft May 6, 2024 00:34

FANNG1 changed the title ~~[#3181] fix(spark-connector): failed to insert data to hive table in spark-sql~~ [#3181] fix(spark-connector): passing Gravitino catalog properties to spark connector May 6, 2024

FANNG1 mentioned this pull request May 6, 2024

[Bug report] In certain environment, failed to insert data to hive table in spark-sql #3181

Closed

FANNG1 force-pushed the insert branch 3 times, most recently from c7de58c to d76c394 Compare May 7, 2024 01:08

FANNG1 changed the title ~~[#3181] fix(spark-connector): passing Gravitino catalog properties to spark connector~~ [#3292] fix(spark-connector): passing Gravitino catalog properties to spark connector May 7, 2024

FANNG1 self-assigned this May 7, 2024

FANNG1 added the branch-0.5 label May 7, 2024

FANNG1 force-pushed the insert branch 2 times, most recently from e8d26af to 0c87206 Compare May 7, 2024 09:29

fix insert data bug

7fd9c88

FANNG1 force-pushed the insert branch from 0c87206 to 7fd9c88 Compare May 7, 2024 09:58

FANNG1 marked this pull request as ready for review May 7, 2024 09:58

FANNG1 requested review from jerryshao, qqqttt123, diqiu50 and yuqi1129 May 7, 2024 10:03

diqiu50 reviewed May 9, 2024

View reviewed changes

jerryshao reviewed May 10, 2024

View reviewed changes

FANNG1 added 2 commits May 10, 2024 14:37

fix comment

a699ca3

fix comment

95a5451

fix comment

44d2af6

jerryshao reviewed May 11, 2024

View reviewed changes

fix comment

0424375

jerryshao approved these changes May 13, 2024

View reviewed changes

jerryshao merged commit 013021d into apache:main May 13, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#3292] fix(spark-connector): passing Gravitino catalog properties to spark connector #3270

[#3292] fix(spark-connector): passing Gravitino catalog properties to spark connector #3270

FANNG1 commented May 6, 2024 •

edited

Loading

FANNG1 commented May 8, 2024

FANNG1 commented May 8, 2024

diqiu50 May 8, 2024

FANNG1 May 9, 2024

FANNG1 May 9, 2024

diqiu50 May 9, 2024

FANNG1 May 10, 2024

diqiu50 May 10, 2024

FANNG1 May 10, 2024

qqqttt123 commented May 9, 2024

FANNG1 commented May 10, 2024

jerryshao May 10, 2024

FANNG1 May 10, 2024

jerryshao May 10, 2024

FANNG1 May 10, 2024

jerryshao May 10, 2024

FANNG1 May 10, 2024

jerryshao May 10, 2024 •

edited

Loading

FANNG1 May 10, 2024

jerryshao May 10, 2024

FANNG1 May 10, 2024

jerryshao May 10, 2024

FANNG1 May 10, 2024

jerryshao May 10, 2024

FANNG1 May 10, 2024

FANNG1 commented May 11, 2024

jerryshao May 11, 2024

FANNG1 May 11, 2024

jerryshao commented May 11, 2024

FANNG1 commented May 11, 2024

FANNG1 commented May 13, 2024

		@@ -42,6 +42,9 @@ The Hive catalog supports creating, updating, and deleting databases and tables

		When you use the Gravitino with Trino. You can pass the Trino Hive connector configuration using prefix `trino.bypass.`. For example, using `trino.bypass.hive.config.resources` to pass the `hive.config.resources` to the Gravitino Hive catalog in Trino runtime.

		When you use the Gravitino with Spark. You can pass the Spark Hive connector configuration using prefix `spark.bypass.`. For example, using `spark.bypass.hive.config.resources` to pass the `hive.config.resources` to the Spark Hive connector in Spark runtime.

		@@ -43,6 +43,9 @@ Any properties not defined by Gravitino with `gravitino.bypass.` prefix will pas

		When you use the Gravitino with Trino. You can pass the Trino Iceberg connector configuration using prefix `trino.bypass.`. For example, using `trino.bypass.iceberg.table-statistics-enabled` to pass the `iceberg.table-statistics-enabled` to the Gravitino Iceberg catalog in Trino runtime.

		When you use the Gravitino with Spark. You can pass the Spark Iceberg connector configuration using prefix `spark.bypass.`. For example, using `spark.bypass.iceberg.config.resources` to pass the `iceberg.config.resources` to the Spark Iceberg connector in Spark runtime.

[#3292] fix(spark-connector): passing Gravitino catalog properties to spark connector #3270

[#3292] fix(spark-connector): passing Gravitino catalog properties to spark connector #3270

Conversation

FANNG1 commented May 6, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

FANNG1 commented May 8, 2024

FANNG1 commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 commented May 9, 2024

FANNG1 commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FANNG1 commented May 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented May 11, 2024

FANNG1 commented May 11, 2024

FANNG1 commented May 13, 2024

FANNG1 commented May 6, 2024 •

edited

Loading

jerryshao May 10, 2024 •

edited

Loading