zjw1111 commented on code in PR #222: URL: https://github.com/apache/paimon-cpp/pull/222#discussion_r3841553382
########## include/paimon/catalog/format_table_catalog.h: ########## @@ -0,0 +1,57 @@ +/* + * 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. + */ + +#pragma once + +#include <memory> + +#include "paimon/result.h" +#include "paimon/visibility.h" + +namespace paimon { + +class FormatTable; +class Identifier; + +/// What a catalog implements when it can load a format table itself. +/// +/// It is a separate base rather than a virtual on `Catalog`, which is exported and derived from +/// outside this library: a virtual added there would have no slot in an already-compiled subclass. +/// A catalog inherits this alongside `Catalog`, and `Catalog::GetFormatTable()` finds it with a +/// `dynamic_cast`. +/// +/// A catalog that does not implement it still serves format tables: `Catalog::GetFormatTable()` +/// falls back to reading the location and the schema through the methods every catalog has. +class PAIMON_EXPORT FormatTableCatalog { + public: + virtual ~FormatTableCatalog() = default; + + /// Loads `identifier` as a format table. + /// + /// The catalog decides both halves the fallback has to guess at: whether the location and the + /// schema can be read in one round trip, and whether it put this table's metadata under the + /// table path. + /// + /// @param identifier Identifier of the table to load. + /// @return A result containing the format table, or an error status if the table does not + /// exist or its `type` option is not `format-table`. + virtual Result<std::shared_ptr<FormatTable>> LoadFormatTable( Review Comment: Thanks for preserving `Catalog` ABI with a separate capability interface. The part that feels odd here is that the catalog returns a fully constructed `FormatTable`, so each catalog owns table construction and validation in addition to loading metadata. Could this return a small descriptor containing `location`, `schema`, and metadata placement instead, with `Catalog::GetFormatTable()` calling `FormatTable::Create()` centrally? This would keep REST's single-request optimization and FileSystemCatalog's metadata-under-location distinction, while giving all catalog implementations one construction/validation path. Using an enum for metadata placement would also be easier to extend than the current boolean. -- 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]
