SteNicholas commented on code in PR #222: URL: https://github.com/apache/paimon-cpp/pull/222#discussion_r3842959715
########## include/paimon/table/format/format_data_split.h: ########## @@ -0,0 +1,103 @@ +/* + * 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 <cstdint> +#include <limits> +#include <map> +#include <string> +#include <vector> + +#include "paimon/table/source/split.h" +#include "paimon/visibility.h" + +namespace paimon { + +/// A split of a format table: the data files of one partition directory, or of the table +/// directory itself when the table is not partitioned. +/// +/// Every file is held whole, since parquet and orc each record where their own row groups and +/// stripes begin and a reader handed a byte range of one would have to find that out for itself. +/// The partition is carried on the split rather than read from the files: a Hive-style layout +/// keeps partition values in the directory names. +class PAIMON_EXPORT FormatDataSplit : public Split { Review Comment: Done, option (b) it is. `e8131fa`: - `ListPartitions()` moved onto `TableScan` as a virtual whose default returns `NotImplemented`; `FormatTableScan` overrides it. A managed table's partitions live in its manifests, so reading them is a scan of its own rather than a listing, and that is left for a follow-up. - `FormatTableScan`, `FormatTableRead`, `FormatTableWrite`, `FormatTableCommit`, `FormatDataSplit` and `FormatCommitMessage` moved to `src/paimon/core/table/format/`, lost their `PAIMON_EXPORT`, and are out of `paimon/api.h` and the installed headers. - `include/paimon/table/format/` now holds `format_table.h` alone. A caller reaches a format table through `Catalog::GetFormatTable()` and then through the generic entry points, seeing only `Plan`, `Split` and `CommitMessage`. - `docs/source/api/format_table.rst` documents `FormatTable` alone now, and the user guide says which types are internal. Thanks for the review. ########## src/paimon/core/catalog/file_system_catalog.h: ########## @@ -38,11 +39,17 @@ class FileSystem; class Identifier; class Logger; -class FileSystemCatalog : public Catalog { +class FileSystemCatalog : public Catalog, public FormatTableCatalog { Review Comment: Agreed, and changed in `e8131fa`. `FormatTableCatalog` is gone, along with the `dynamic_cast`: ```cpp // Catalog, public: Result<std::shared_ptr<FormatTable>> GetFormatTable(const Identifier& identifier) const; // Catalog, protected: virtual Result<std::shared_ptr<FormatTable>> LoadFormatTable(const Identifier& identifier) const; ``` `GetFormatTable()` calls the hook, and the default implementation is the two-request path (read the location and the schema through the virtuals every catalog has, and treat everything below the location as data). `FileSystemCatalog` and `RestCatalog` override it directly, in their `protected` sections, with the implementations they already had: the file system catalog to say its metadata lives under the table location, the REST catalog to take the location and the schema from one `GetTable` response. So the extension point is now visible on `Catalog` itself, a catalog derives from one base, and there is no RTTI cross-cast. `include/paimon/catalog/format_table_catalog.h` is deleted and its doxygen entry removed. Thanks for pushing on this. -- 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]
