tengqm commented on code in PR #5864:
URL: https://github.com/apache/gravitino/pull/5864#discussion_r1886700225
##########
catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonSchemaPropertiesMetadata.java:
##########
@@ -34,7 +34,7 @@
*/
public class PaimonSchemaPropertiesMetadata extends BasePropertiesMetadata {
- public static final String COMMENT = "comment";
+ public static final String COMMENT = PaimonConstants.COMMENT;
Review Comment:
Why do we have this constant copied into `PaimonSchemaPropertiesMetadata`?
##########
catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonTablePropertiesMetadata.java:
##########
@@ -35,14 +35,14 @@
*/
public class PaimonTablePropertiesMetadata extends BasePropertiesMetadata {
- public static final String COMMENT = "comment";
- public static final String OWNER = "owner";
- public static final String BUCKET_KEY = "bucket-key";
- public static final String MERGE_ENGINE = "merge-engine";
- public static final String SEQUENCE_FIELD = "sequence.field";
- public static final String ROWKIND_FIELD = "rowkind.field";
- public static final String PRIMARY_KEY = "primary-key";
- public static final String PARTITION = "partition";
+ public static final String COMMENT = PaimonConstants.COMMENT;
+ public static final String OWNER = PaimonConstants.OWNER;
+ public static final String BUCKET_KEY = PaimonConstants.BUCKET_KEY;
+ public static final String MERGE_ENGINE = PaimonConstants.MERGE_ENGINE;
+ public static final String SEQUENCE_FIELD = PaimonConstants.SEQUENCE_FIELD;
+ public static final String ROWKIND_FIELD = PaimonConstants.ROWKIND_FIELD;
+ public static final String PRIMARY_KEY = PaimonConstants.PRIMARY_KEY;
+ public static final String PARTITION = PaimonConstants.PARTITION;
Review Comment:
Same as above. Why do we replicate these constants?
##########
spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/catalog/BaseCatalog.java:
##########
@@ -411,6 +421,32 @@ protected org.apache.gravitino.rel.Table
loadGravitinoTable(Identifier ident)
}
}
+ protected org.apache.spark.sql.connector.catalog.Table loadSparkTable(
+ Identifier ident, String version) {
+ try {
+ return sparkCatalog.loadTable(ident, version);
+ } catch (NoSuchTableException e) {
+ throw new RuntimeException(
+ String.format(
+ "Failed to load the real sparkTable: %s",
Review Comment:
is `sparkeTable` a well known term in the community?
##########
spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/paimon/PaimonPropertiesConstants.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.gravitino.spark.connector.paimon;
+
+import org.apache.gravitino.catalog.lakehouse.paimon.PaimonConstants;
+
+public class PaimonPropertiesConstants {
+
+ public static final String GRAVITINO_PAIMON_CATALOG_BACKEND =
PaimonConstants.CATALOG_BACKEND;
Review Comment:
I'm really confused by this pattern of copying constant definitions.
The referenced constant is defined for Paimon, and we are now inside a
Paimon specific class, but we are copying these constants inside the same
logical package.
We are giving these constants new names, by prepending many terms.
The result? We get a new public class, giving many existing constants new
aliases.
A new Java file, with many long constant names ...
##########
spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/version/CatalogNameAdaptor.java:
##########
@@ -27,15 +27,24 @@
public class CatalogNameAdaptor {
private static final Map<String, String> catalogNames =
ImmutableMap.of(
- "hive-3.3",
"org.apache.gravitino.spark.connector.hive.GravitinoHiveCatalogSpark33",
- "hive-3.4",
"org.apache.gravitino.spark.connector.hive.GravitinoHiveCatalogSpark34",
- "hive-3.5",
"org.apache.gravitino.spark.connector.hive.GravitinoHiveCatalogSpark35",
+ "hive-3.3",
+
"org.apache.gravitino.spark.connector.hive.GravitinoHiveCatalogSpark33",
+ "hive-3.4",
+
"org.apache.gravitino.spark.connector.hive.GravitinoHiveCatalogSpark34",
+ "hive-3.5",
+
"org.apache.gravitino.spark.connector.hive.GravitinoHiveCatalogSpark35",
Review Comment:
These changes really hurts the readability of the source.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
