ashvina commented on code in PR #591:
URL: https://github.com/apache/incubator-xtable/pull/591#discussion_r1889638434
##########
xtable-api/src/main/java/org/apache/xtable/conversion/TargetTable.java:
##########
@@ -44,4 +44,8 @@ public TargetTable(
this.metadataRetention =
metadataRetention == null ? Duration.of(7, ChronoUnit.DAYS) :
metadataRetention;
}
+
+ public String getId() {
Review Comment:
What is the contract of Id? Could you please add javadoc for public methods
of the core classes ?
##########
xtable-api/src/main/java/org/apache/xtable/conversion/ConversionConfig.java:
##########
@@ -34,14 +35,21 @@ public class ConversionConfig {
@NonNull SourceTable sourceTable;
// One or more targets to sync the table metadata to
List<TargetTable> targetTables;
+ // Each target table can be synced to multiple target catalogs, this is map
from
+ // targetTableIdentifier to target catalogs.
+ Map<String, List<TargetCatalogConfig>> targetCatalogs;
Review Comment:
I am curious about the expected behavior if `XTable` fails to update a
subset of the `TargetCatalogs` and if that impacts the way incremental sync
works?
This may have been covered elsewhere in the PR that I might have missed.
##########
xtable-api/src/main/java/org/apache/xtable/model/catalog/CatalogTableIdentifier.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.xtable.model.catalog;
+
+import lombok.Builder;
+import lombok.NonNull;
+import lombok.Value;
+
+/** This class represents the unique identifier for a table in a catalog. */
+@Value
+@Builder
+public class CatalogTableIdentifier {
+ /**
+ * Catalogs have the ability to group tables logically, databaseName is the
identifier for such
+ * logical classification. The alternate names for this field include
namespace, schemaName etc.
Review Comment:
I agree that the nesting vary depending on the database. However, the usage
of a 3-part naming is quite common. The convention includes name of database,
schema, and table or view. I am wondering if it would be feasible to keep the
table Id as a string instead of an object whose structure can vary a lot.
--
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: commits-unsubscr...@xtable.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
