ashvina commented on code in PR #591: URL: https://github.com/apache/incubator-xtable/pull/591#discussion_r1882985381
########## 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. + */ + @NonNull String databaseName; + + /** + * The table name used when syncing the table to the catalog. NOTE: This name can be different + * from the table name in storage. + */ + @NonNull String tableName; + + public String getId() { + return String.format("%s-%s", databaseName, tableName); Review Comment: In Iceberg, a "." character is used as the separator. This id convention could create issues with Iceberg source and target tables? ########## xtable-api/src/main/java/org/apache/xtable/conversion/TargetCatalog.java: ########## @@ -0,0 +1,45 @@ +/* + * 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.conversion; + +import lombok.Builder; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.NonNull; + +import org.apache.xtable.model.catalog.CatalogTableIdentifier; + +@EqualsAndHashCode(callSuper = true) +@Getter +public class TargetCatalog extends ExternalCatalog { + /** + * The tableIdentifiers(databaseName, tableName) that will be used when syncing it to the {@link + * TargetCatalog}. + */ + @NonNull CatalogTableIdentifier catalogTableIdentifier; Review Comment: This proposal makes `TableIdentifier` a member of the `Catalog` instance. This coupling seems a bit off since, in practice, a `Catalog` instance typically contains many tables (a 1-to-n relationship). However, this implementation makes it a 1-to-1 relationship. Wouldn't it be clearer to replace `CatalogConfig` in `ExternalTable` with `ExternalCatalog`? This change would better reflect the actual relationship and improve the clarity of the code. ########## xtable-api/src/main/java/org/apache/xtable/conversion/ExternalTable.java: ########## @@ -38,6 +40,8 @@ class ExternalTable { protected final @NonNull String formatName; /** The path to the root of the table or the metadata directory depending on the format */ protected final @NonNull String basePath; + /** The path to the data files, defaults to the metadataPath */ Review Comment: There is no field for `metadataPath` in `ExternalTable`. I guess it was supposed to be the `basePath` ########## xtable-api/src/main/java/org/apache/xtable/conversion/ExternalTable.java: ########## @@ -38,6 +40,8 @@ class ExternalTable { protected final @NonNull String formatName; /** The path to the root of the table or the metadata directory depending on the format */ protected final @NonNull String basePath; + /** The path to the data files, defaults to the metadataPath */ + protected final @NonNull String dataPath; Review Comment: Could you please clarify why was it necessary to pull the `dataPath` from the `SourceTable` into the base class? ########## 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 { Review Comment: 1) What do you think of calling this class `TableIdentifier`. 2) Also, we should consider making the naming more generic, since it should represent all types of table identifiers. For instance, while `databaseName` is a popular namespace, there's also `schema`. In some scenarios, `databaseName` is synonymous with `catalogName`. The current two-part naming based on `table` and `databaseName` seems a bit restrictive to me. Would you mind sharing the use cases this naming caters to? -- 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
