diff --git a/docs/source/user_guide/catalog.rst b/docs/source/user_guide/catalog.rst index b695044dd..b843b5086 100644 --- a/docs/source/user_guide/catalog.rst +++ b/docs/source/user_guide/catalog.rst @@ -113,7 +113,118 @@ Databases and tables are then created, listed, loaded, renamed and dropped through the regular ``Catalog`` API, and table snapshots can be listed through ``Catalog::ListSnapshots``. -The C++ REST catalog covers the database, table and snapshot operations of the -``Catalog`` API. The parts of the Java REST catalog that have no C++ counterpart -yet — altering a database or a table, views, functions, partitions, tags, branch -management and consumers — are not supported. +Committing through the catalog +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +A REST catalog reports ``Catalog::SupportsVersionManagement() == true`` and +publishes snapshots through the server. Pass the catalog and table identifier +to ``CommitContextBuilder::WithCatalog`` to commit on local or object storage +without an atomic snapshot-file rename: + +.. code-block:: cpp + + // Writers and committers share ownership of the catalog. + std::shared_ptr shared_catalog(std::move(catalog)); + + // Load the table's identity before preparing changes. + PAIMON_ASSIGN_OR_RAISE(std::shared_ptr table, + shared_catalog->GetTable(paimon::Identifier("db", "tbl"))); + + paimon::CommitContextBuilder builder(table_path, "commit-user"); + builder.WithCatalog(shared_catalog, paimon::Identifier("db", "tbl")); + // CatalogUuid() stays null when the server supplies no id. + if (std::optional table_uuid = table->CatalogUuid()) { + builder.WithTableId(table_uuid.value()); + } + PAIMON_ASSIGN_OR_RAISE(std::unique_ptr context, builder.Finish()); + PAIMON_ASSIGN_OR_RAISE(std::unique_ptr commit, + paimon::FileStoreCommit::Create(std::move(context))); + PAIMON_RETURN_NOT_OK(commit->Commit(commit_messages)); + +Configure native writers with the same catalog and identifier. They load the +current schema and latest snapshot from the catalog, including committed +offsets when a real-time writer is created: + +.. code-block:: cpp + + paimon::WriteContextBuilder write_builder(table_path, "commit-user"); + write_builder.WithCatalog(shared_catalog, paimon::Identifier("db", "tbl")); + PAIMON_ASSIGN_OR_RAISE(std::unique_ptr write_context, + write_builder.Finish()); + PAIMON_ASSIGN_OR_RAISE(std::unique_ptr writer, + paimon::FileStoreWrite::Create(std::move(write_context))); + +For real-time writes, also set ``WithStreamingMode(true)`` and +``WithRealtimeContext(...)`` with ``realtime.enabled=true``. After +``CommitWithProgress`` succeeds, pass its returned snapshot id to +``writer->RefreshCommittedSnapshot`` to release committed in-memory data. +The refresh reads the current snapshot from the catalog. If another commit has +already advanced the latest snapshot, the requested snapshot must be published +under the table path. Recreating the writer restores offsets from the catalog's +current snapshot. + +Both builders use the catalog's file system for manifests and data, preserving +its object-store credentials. ``WithFileSystem`` overrides it; writers also +allow ``WithFileSystemSchemeToIdentifierMap`` to override file-system selection. +For format tables, use ``WriteContextBuilder(FormatTable)`` instead. + +The snapshot being committed carries a uuid generated on the client, and the +commit names the snapshot it is based on by that snapshot's uuid, so the server +can tell a commit that raced another. A commit which lost such a race is +rebased on the new latest snapshot and retried, bounded by ``commit.max-retries`` +and ``commit.timeout`` as a file-system commit is. + +The HTTP client neither retries nor follows redirects for a commit request. +Transport errors and non-success responses, including 429, 503 and redirects, +leave the outcome uncertain: the server may already have accepted the snapshot. +Its metadata files are retained. Use ``FilterAndCommit`` for batch recovery; +after a real-time error, recreate the writer and context and recover from durable +offsets as described by ``CommitWithProgress``. + +Only the main branch is supported by ``WithCatalog``. Both builders reject other +branches in the identifier or ``branch`` option; the write builder also checks +``WithBranch``. Explicit ``tbl$branch_main`` and ``branch=main`` are accepted. + +The current schema and latest snapshot need no files under the table path. +A catalog response of ``{"snapshot": null}`` means the table has no snapshot. +Only a catalog reporting that snapshot loading is unsupported falls back to +file-system lookup; other catalog errors propagate to the caller. + +Read ``Table::CatalogUuid()`` before preparing changes and pass it to +``WithTableId`` so the server can reject commits to a dropped and recreated table. +``Table::Uuid()`` can fall back to the table name and must not supply this id. +An unset table id is sent as null and subject to server validation. Each commit +attempt reloads the catalog's current schema id for the new snapshot. + +.. note:: + + Manifests, data and historical metadata still require file-system access: + + * ``FilterAndCommit``, ``CommitWithProgress`` and streaming writer recovery + may walk older snapshots to find the commit user's last snapshot. Those + snapshots must be published under the table path. A missing retained + snapshot is an error; the ``EARLIEST`` hint distinguishes expired history. + * ``RowIdCheckConflict`` reuses the latest snapshot from the catalog and reads + earlier snapshots from the table path. With data evolution enabled, the + check also requires the schemas recorded by the row-id files it inspects. + Publishing only the current schema is insufficient when files use older ids. + * ``RollbackToAsLatest`` reads its target snapshot from the table path. + ``Expire`` manages only snapshots published there and updates the + ``EARLIEST`` hint; it does not expire catalog-held metadata. + Before deleting files, expiration verifies that the catalog's current + snapshot matches the latest published snapshot and reads the retained + history. It skips expiration while the current snapshot is unpublished + or differs from its file-system copy. Catalog and retained-metadata read + errors stop expiration before deletion. A catalog that does not support + loading its current snapshot cannot perform local expiration. + Retained snapshots with index manifests also stop expiration with + ``NotImplemented`` before any files are deleted. + +Expiration preserves files referenced by retained snapshots, including files +restored by ``RollbackToAsLatest``. Serialize rollback and expiration through +the upstream coordinator: a rollback must finish before expiration starts, +so its restored file references are visible to the expiration operation. + +The C++ REST catalog covers the database, table, snapshot and commit operations +of the ``Catalog`` API. The parts of the Java REST catalog that have no C++ +counterpart yet — altering a database or a table, views, functions, partitions, +tags, branch management and consumers — are not supported. diff --git a/docs/source/user_guide/format_table.rst b/docs/source/user_guide/format_table.rst index b52bd5d36..a7b8e6721 100644 --- a/docs/source/user_guide/format_table.rst +++ b/docs/source/user_guide/format_table.rst @@ -68,7 +68,7 @@ a constructor taking one - which is what Java does through ``FormatTable.newRead The table already carries its schema and the file system it was loaded through, so a context built from one refuses ``SetTableSchema()``, ``WithFileSystem()``, -``WithFileSystemSchemeToIdentifierMap()`` and a branch rather than quietly ignoring them. Options +``WithFileSystemSchemeToIdentifierMap()``, ``WithCatalog()`` and a branch. Options given at the call still win over the ones the schema stored, as they do everywhere else. A batch comes back in the table's column order, or the projection's when the read names one, @@ -126,11 +126,13 @@ A setting the format path cannot act on is refused by name rather than quietly d * ``WriteContextBuilder::WithWriteSchema()``, which names a subset of the columns to write. * ``WriteContextBuilder::WithWriteId()``, which prefixes a postpone-bucket writer's files so one compaction reader can put them back in order; a format table has no buckets. -* ``CommitContextBuilder::IgnoreEmptyCommit(false)``, ``UseRESTCatalogCommit(true)`` and - ``AppendCommitCheckConflict(true)``. Keeping an empty commit means writing a snapshot that adds - no files, a rest-catalog commit sends that snapshot to a catalog, and the conflict check reads - the manifests of concurrent commits - none of which exist here. Each is refused only when set - away from its default, so an ordinary commit is unaffected. +* ``CommitContextBuilder::IgnoreEmptyCommit(false)``, ``UseRESTCatalogCommit(true)``, + ``WithTableId()``, ``WithCatalog()`` and ``AppendCommitCheckConflict(true)``. Keeping an empty + commit means writing a snapshot that adds no files, a rest-catalog commit sends that snapshot to + a catalog, a table id names the table in the request that commit sends, committing through a + catalog hands it a snapshot to take, and the conflict check reads the manifests of concurrent + commits - none of which exist here. Each is refused only when set away from its default, so an + ordinary commit is unaffected. * the ``branch`` option, whatever names it: a format table keeps no metadata to branch, and its data is the files under its one location, which is where a read or a write would go whichever branch was asked for. It is refused wherever it comes from - the schema, a catalog that parsed diff --git a/docs/source/user_guide/snapshot.rst b/docs/source/user_guide/snapshot.rst index 7d0665ae3..278e72af4 100644 --- a/docs/source/user_guide/snapshot.rst +++ b/docs/source/user_guide/snapshot.rst @@ -49,19 +49,20 @@ Snapshot File Snapshot file is JSON and includes: 1. ``version``: Snapshot file version, current is 3. -2. ``id``: Snapshot id, same as the file name. -3. ``schemaId``: The corresponding schema version for this commit. -4. ``baseManifestList``: A manifest list recording all changes from the previous snapshots. -5. ``deltaManifestList``: A manifest list recording all new changes occurred in this snapshot. -6. ``changelogManifestList``: A manifest list recording all changelog produced in this snapshot; ``null`` if no changelog is produced. -7. ``indexManifest``: A manifest recording all index files of this table; ``null`` if no table index file exists. -8. ``commitUser``: Usually generated by UUID; used for recovery of streaming writes—one stream write job with one user. -9. ``commitIdentifier``: Transaction id corresponding to streaming write; each transaction may result in multiple commits for different ``commitKind`` values. -10. ``commitKind``: Type of changes in this snapshot, including ``append``, ``compact``, ``overwrite`` and ``analyze``. -11. ``timeMillis``: Commit time in milliseconds. -12. ``logOffsets``: Commit log offsets. -13. ``totalRecordCount``: Record count of all changes occurred in this snapshot. -14. ``deltaRecordCount``: Record count of all new changes occurred in this snapshot. -15. ``changelogRecordCount``: Record count of all changelog produced in this snapshot. -16. ``watermark``: Watermark for input records, from Flink watermark mechanism; ``Long.MIN_VALUE`` if there is no watermark. -17. ``statistics``: Stats file name for statistics of this table. +2. ``uuid``: Uuid of this snapshot, generated on the client when the snapshot is created and absent only for a snapshot written before uuid was introduced. A commit names the uuid of the snapshot it is based on, so that a catalog can refuse a commit which raced another. +3. ``id``: Snapshot id, same as the file name. +4. ``schemaId``: The corresponding schema version for this commit. +5. ``baseManifestList``: A manifest list recording all changes from the previous snapshots. +6. ``deltaManifestList``: A manifest list recording all new changes occurred in this snapshot. +7. ``changelogManifestList``: A manifest list recording all changelog produced in this snapshot; ``null`` if no changelog is produced. +8. ``indexManifest``: A manifest recording all index files of this table; ``null`` if no table index file exists. +9. ``commitUser``: Usually generated by UUID; used for recovery of streaming writes—one stream write job with one user. +10. ``commitIdentifier``: Transaction id corresponding to streaming write; each transaction may result in multiple commits for different ``commitKind`` values. +11. ``commitKind``: Type of changes in this snapshot, including ``append``, ``compact``, ``overwrite`` and ``analyze``. +12. ``timeMillis``: Commit time in milliseconds. +13. ``logOffsets``: Commit log offsets. +14. ``totalRecordCount``: Record count of all changes occurred in this snapshot. +15. ``deltaRecordCount``: Record count of all new changes occurred in this snapshot. +16. ``changelogRecordCount``: Record count of all changelog produced in this snapshot. +17. ``watermark``: Watermark for input records, from Flink watermark mechanism; ``Long.MIN_VALUE`` if there is no watermark. +18. ``statistics``: Stats file name for statistics of this table. diff --git a/docs/source/user_guide/write.rst b/docs/source/user_guide/write.rst index a9d81906b..8f4e3e90d 100644 --- a/docs/source/user_guide/write.rst +++ b/docs/source/user_guide/write.rst @@ -169,9 +169,17 @@ Operational Flow 5. For a direct file-system commit on a non-object-store path, the engine passes the objects to ``FileStoreCommit`` for either an Append or PK table. - For an object-store path, it enables REST catalog commit mode, calls - ``Commit``, obtains the JSON request from ``GetLastCommitTableRequest``, and - sends that request to the REST catalog. + For a table of a catalog which owns its versions, such as a REST catalog, it + passes that catalog and the table identifier to + ``WriteContextBuilder::WithCatalog`` and ``CommitContextBuilder::WithCatalog``. + The writer loads the current schema and latest snapshot from the catalog. + On ``Commit``, the snapshot and + the statistics of the change go to the catalog, which decides whether this + commit wins, and one that lost the race is rebased and retried the way a + file-system commit is. A caller without a catalog client can instead enable + REST catalog commit mode, call ``Commit``, obtain the JSON request from + ``GetLastCommitTableRequest``, and send that request to the REST catalog + itself. 6. The local committer or REST catalog validates the messages, updates manifests/metadata, and finalizes the snapshot atomically. diff --git a/include/paimon/catalog/catalog.h b/include/paimon/catalog/catalog.h index f003509b6..742352b0b 100644 --- a/include/paimon/catalog/catalog.h +++ b/include/paimon/catalog/catalog.h @@ -44,8 +44,6 @@ class FormatTable; class Table; class View; class Schema; -class Snapshot; -class PartitionStatistics; class Tag; class Identifier; /// This interface is responsible for reading and writing metadata such as database/table from a @@ -211,6 +209,11 @@ class PAIMON_EXPORT Catalog { virtual Result> ListSnapshots( const Identifier& identifier, const std::string& branch = "") const = 0; + /// Returns whether this catalog publishes snapshots through catalog commits. Defaults to false. + /// @note Implementations returning true must also implement the internal + /// `VersionManagedCatalog`. + virtual bool SupportsVersionManagement() const; + /// Gets a format table: a directory of data files laid out like a standard Hive table. /// /// A format table carries no snapshots and no manifests, so it is loaded through its own diff --git a/include/paimon/catalog/table.h b/include/paimon/catalog/table.h index 71fe2a1ab..ef68d1e44 100644 --- a/include/paimon/catalog/table.h +++ b/include/paimon/catalog/table.h @@ -20,6 +20,7 @@ #include #include +#include #include #include @@ -45,9 +46,10 @@ class PAIMON_EXPORT Table { const std::string& table_path, const Identifier& identifier); + /// @param uuid Metastore UUID, or empty if unavailable. Table(const std::shared_ptr& schema, const std::string& database, - const std::string& table_name) - : schema_(schema), database_(database), table_name_(table_name) {} + const std::string& table_name, const std::string& uuid = "") + : schema_(schema), database_(database), table_name_(table_name), uuid_(uuid) {} ~Table() = default; @@ -59,10 +61,16 @@ class PAIMON_EXPORT Table { /// Full name of the table, default is database.tableName. std::string FullName() const; - /// UUID of the table, metastore can provide the true UUID of this table, default is the full - /// name. + /// Returns the metastore UUID, falling back to `FullName()`. + /// @note Use `CatalogUuid()` for commit requests to avoid sending a table name as an ID. std::string Uuid() const { - return FullName(); + return uuid_.empty() ? FullName() : uuid_; + } + + /// Returns the metastore UUID, or null if unavailable, for + /// `CommitContextBuilder::WithTableId()`. + std::optional CatalogUuid() const { + return uuid_.empty() ? std::nullopt : std::optional(uuid_); } /// Loads the latest schema of table. @@ -74,6 +82,7 @@ class PAIMON_EXPORT Table { std::shared_ptr schema_; std::string database_; std::string table_name_; + std::string uuid_; }; } // namespace paimon diff --git a/include/paimon/commit_context.h b/include/paimon/commit_context.h index e0b3e4b1b..aa2eef266 100644 --- a/include/paimon/commit_context.h +++ b/include/paimon/commit_context.h @@ -21,13 +21,16 @@ #include #include +#include #include +#include "paimon/catalog/identifier.h" #include "paimon/result.h" #include "paimon/type_fwd.h" #include "paimon/visibility.h" namespace paimon { +class Catalog; class Executor; class FormatTable; class MemoryPool; @@ -39,6 +42,7 @@ class MemoryPool; /// @see CommitContextBuilder class PAIMON_EXPORT CommitContext { public: + /// Retained for source compatibility. Prefer `CommitContextBuilder` for input validation. CommitContext(const std::string& root_path, const std::string& commit_user, bool ignore_empty_commit, bool use_rest_catalog_commit, bool append_commit_check_conflict, const std::shared_ptr& memory_pool, @@ -46,6 +50,17 @@ class PAIMON_EXPORT CommitContext { const std::shared_ptr& specific_file_system, const std::map& options, const std::shared_ptr& format_table); + + CommitContext(const std::string& root_path, const std::string& commit_user, + bool ignore_empty_commit, bool use_rest_catalog_commit, + const std::shared_ptr& catalog, + const std::optional& identifier, + const std::optional& table_id, bool append_commit_check_conflict, + const std::shared_ptr& memory_pool, + const std::shared_ptr& executor, + const std::shared_ptr& specific_file_system, + const std::map& options, + const std::shared_ptr& format_table); ~CommitContext(); const std::string& GetRootPath() const { @@ -64,6 +79,21 @@ class PAIMON_EXPORT CommitContext { return use_rest_catalog_commit_; } + /// Returns the configured catalog, or null if unset. + const std::shared_ptr& GetCatalog() const { + return catalog_; + } + + /// Returns the table identifier supplied with the catalog. + const std::optional& GetIdentifier() const { + return identifier_; + } + + /// Returns the catalog table UUID sent as `tableId` in commit requests. + const std::optional& GetTableId() const { + return table_id_; + } + bool AppendCommitCheckConflict() const { return append_commit_check_conflict_; } @@ -95,6 +125,9 @@ class PAIMON_EXPORT CommitContext { std::string commit_user_; bool ignore_empty_commit_; bool use_rest_catalog_commit_; + std::shared_ptr catalog_; + std::optional identifier_; + std::optional table_id_; bool append_commit_check_conflict_; std::shared_ptr memory_pool_; std::shared_ptr executor_; @@ -146,12 +179,37 @@ class PAIMON_EXPORT CommitContextBuilder { /// @return Reference to this builder for method chaining. CommitContextBuilder& IgnoreEmptyCommit(bool ignore_empty_commit); - /// Sets whether to use REST catalog commit (default is false). + /// Builds a REST commit request for the caller to send (default is false). + /// Use `WithCatalog()` for catalog-managed commits; the two modes are mutually exclusive. /// @note Temporary interface, will be removed in the future. - /// @param use_rest_catalog_commit True to use REST catalog commit, false otherwise. + /// @param use_rest_catalog_commit True to build the request without sending it. /// @return Reference to this builder for method chaining. CommitContextBuilder& UseRESTCatalogCommit(bool use_rest_catalog_commit); + /// Loads the current schema from `catalog`. Catalogs with version management also load and + /// commit snapshots, rebasing on conflicts. Other catalogs use file-system snapshot commits. + /// Mutually exclusive with `UseRESTCatalogCommit()`. + /// + /// Data and manifests remain on the file system. Recovery, rollback and row-id checks require + /// historical snapshots there; row-id checks also require the inspected data files' schemas. + /// `Expire()` manages file-system snapshots after confirming that the catalog's current + /// snapshot and retained history are published there. Configure writers with the same catalog + /// via `WriteContextBuilder::WithCatalog()`. + /// @param catalog Non-null catalog, kept alive by this context. + /// @param identifier Table to commit to. Only the main branch is supported. + /// @return Reference to this builder for method chaining. + CommitContextBuilder& WithCatalog(const std::shared_ptr& catalog, + const Identifier& identifier); + + /// Sets the catalog table UUID sent as `tableId` to detect table recreation (default is unset). + /// + /// Capture `Table::CatalogUuid()` before preparing changes. `Table::Uuid()` may fall back to + /// the table name. An unset ID is sent as null and validated by the server. + /// Requires `WithCatalog()` or `UseRESTCatalogCommit()`. + /// @param table_id Catalog UUID of the table to commit to. + /// @return Reference to this builder for method chaining. + CommitContextBuilder& WithTableId(const std::string& table_id); + /// Sets whether append commits should perform conflict checking (default is false). /// @param append_commit_check_conflict True to enable append conflict checks. /// @return Reference to this builder for method chaining. diff --git a/include/paimon/file_store_commit.h b/include/paimon/file_store_commit.h index 553112b31..d1553c714 100644 --- a/include/paimon/file_store_commit.h +++ b/include/paimon/file_store_commit.h @@ -46,9 +46,8 @@ class CommitMessage; /// dropping partitions, and retrieving commit metrics. /// /// @note Direct file-system commits support append-only and primary-key tables on non-object-store -/// paths. Object-store paths require REST catalog commit mode: enable it with -/// `CommitContextBuilder::UseRESTCatalogCommit()`, call `Commit()` or `FilterAndCommit()`, and then -/// retrieve the request with `GetLastCommitTableRequest()`. +/// paths. `CommitContextBuilder::WithCatalog()` also supports object stores when the catalog +/// manages versions. `UseRESTCatalogCommit()` only prepares a request for the caller to send. class PAIMON_EXPORT FileStoreCommit { public: /// Create an instance of `FileStoreCommit`. @@ -94,9 +93,10 @@ class PAIMON_EXPORT FileStoreCommit { /// An error is terminal for the writer state which produced `realtime_commits`. The caller must /// discard its `RealtimeContext` and `FileStoreWrite`, load the current latest snapshot's /// durable offsets, recreate both objects, and replay input from those exclusive offsets. - /// External conflicts returned after submitting a REST catalog request are not retried by this - /// method. Concurrent rollback or partition deletion from another process must be fenced by - /// the upstream coordinator. + /// Conflicts from `CommitContextBuilder::WithCatalog()` are retried internally. With + /// `UseRESTCatalogCommit()`, the caller sends the request and handles server conflicts. + /// Concurrent rollback or partition deletion must be fenced by the upstream coordinator. + /// Configure catalog writers with `WriteContextBuilder::WithCatalog()` too. /// /// @param realtime_commits Commit messages and left-closed, right-open offset ranges to /// commit. @@ -164,19 +164,21 @@ class PAIMON_EXPORT FileStoreCommit { const std::vector>& commit_messages, int64_t commit_identifier, std::optional watermark = std::nullopt) = 0; - /// If user want to use REST catalog commit, please set - /// `CommitContextBuilder::UseRESTCatalogCommit()`, then call `Commit()` (or - /// `FilterAndCommit()`) normally, then call this method to get the last commit table request, - /// which is a JSON string that can be used to send to REST catalog server. - /// + /// Returns the request from the latest commit attempt, including failed attempts. + /// Catalog commits send requests automatically; `UseRESTCatalogCommit()` only prepares them. /// @note Temporary interface for internal use, will be removed in the future. - /// - /// @return A Result containing a JSON string which including `snapshot` and `statistics`, but - /// excluding `tableId`. + /// @return JSON with `tableId`, `baseSnapshotUuid`, `snapshot` and `statistics`. + /// Unset `tableId` and an absent or legacy base snapshot UUID are serialized as null. virtual Result GetLastCommitTableRequest() = 0; /// Expire old snapshot in the file store. /// + /// Protects files referenced by retained snapshots, including files restored by rollback. + /// Catalog commits require the current snapshot and retained history to be published to the + /// file system before deletion. An unpublished or mismatched current snapshot skips expiration; + /// catalog or retained-metadata read errors propagate without deleting files. + /// Retained snapshots with index manifests return `NotImplemented` before deletion. + /// @note Coordinate rollback and expiration so they do not run concurrently. /// @return Result indicating the number of expired items or an error status. virtual Result Expire() = 0; @@ -218,7 +220,8 @@ class PAIMON_EXPORT FileStoreCommit { /// there is no latest snapshot or the target snapshot does not exist. /// @note Rollback restores the real-time progress recorded by the target snapshot. Active /// real-time writers and their `RealtimeContext` instances must be recreated before - /// further real-time operations. + /// further real-time operations. Coordinate rollback with expiration so the target's + /// files cannot be deleted while the rollback is being prepared or committed. virtual Result RollbackToAsLatest(int64_t target_snapshot_id) = 0; /// Configure row-id conflict checking from a specific snapshot id. diff --git a/include/paimon/file_store_write.h b/include/paimon/file_store_write.h index 3e3cfb8e4..5bc2fe73e 100644 --- a/include/paimon/file_store_write.h +++ b/include/paimon/file_store_write.h @@ -126,6 +126,10 @@ class PAIMON_EXPORT FileStoreWrite { /// /// The writer loads the snapshot's partition-bucket offsets and releases sealed memory that is /// fully covered by disk. Calling this method on a non-real-time writer returns an error. + /// + /// With `WriteContextBuilder::WithCatalog()`, the latest snapshot can be loaded from the + /// catalog. Older requested snapshots must be available on the file system. + /// /// If the snapshot overwrites table contents or moves committed progress backwards, such as /// after a partition drop, overwrite, or rollback, this method returns an error and the caller /// must recreate the `RealtimeContext` and writer. These operations are not fenced against an diff --git a/include/paimon/write_context.h b/include/paimon/write_context.h index 16514d394..ff7245ee6 100644 --- a/include/paimon/write_context.h +++ b/include/paimon/write_context.h @@ -25,11 +25,13 @@ #include #include +#include "paimon/catalog/identifier.h" #include "paimon/result.h" #include "paimon/type_fwd.h" #include "paimon/visibility.h" namespace paimon { +class Catalog; class Executor; class FormatTable; class MemoryPool; @@ -51,7 +53,9 @@ class PAIMON_EXPORT WriteContext { const std::map& fs_scheme_to_identifier_map, const std::shared_ptr& realtime_context, const std::map& options, - const std::shared_ptr& format_table); + const std::shared_ptr& format_table, + const std::shared_ptr& catalog = nullptr, + const std::optional& identifier = std::nullopt); ~WriteContext(); @@ -126,6 +130,16 @@ class PAIMON_EXPORT WriteContext { return format_table_; } + /// Returns the catalog supplying table metadata, or null if unset. + const std::shared_ptr& GetCatalog() const { + return catalog_; + } + + /// Returns the table identifier supplied with the catalog. + const std::optional& GetIdentifier() const { + return identifier_; + } + private: std::string root_path_; std::string commit_user_; @@ -144,6 +158,8 @@ class PAIMON_EXPORT WriteContext { std::shared_ptr realtime_context_; std::map options_; std::shared_ptr format_table_; + std::shared_ptr catalog_; + std::optional identifier_; }; /// `WriteContextBuilder` used to build a `WriteContext`, has input validation. @@ -157,8 +173,8 @@ class PAIMON_EXPORT WriteContextBuilder { /// Constructs a `WriteContextBuilder` for a format table that is already loaded: the only way /// to write one whose schema lives in a metastore rather than under its location, such as a /// table a REST catalog serves. The table carries what such a location does not say, so - /// `WithFileSystem()`, `WithFileSystemSchemeToIdentifierMap()` and a branch are refused here - /// rather than ignored. + /// `WithFileSystem()`, `WithFileSystemSchemeToIdentifierMap()`, `WithCatalog()` and a branch + /// are refused here rather than ignored. /// /// There is no commit user: a format table keeps no snapshot to record one in. /// @@ -237,6 +253,19 @@ class PAIMON_EXPORT WriteContextBuilder { /// @note If not set, use default file system (configured in `Options::FILE_SYSTEM`) WriteContextBuilder& WithFileSystem(const std::shared_ptr& file_system); + /// Loads a native table's schema and, for catalogs with version management, its latest + /// snapshot. Use the same catalog and identifier as `CommitContextBuilder` to restore and + /// refresh real-time progress. + /// + /// Only the main branch is supported. Data, manifests and historical metadata remain on the + /// file system. The catalog supplies that file system unless overridden + /// by `WithFileSystem()` or `WithFileSystemSchemeToIdentifierMap()`. + /// @param catalog Non-null catalog, kept alive while the writer uses its snapshot loader. + /// @param identifier The native table to write to. + /// @return Reference to this builder for method chaining. + WriteContextBuilder& WithCatalog(const std::shared_ptr& catalog, + const Identifier& identifier); + /// Enables the real-time write path with the provided shared context. /// @param realtime_context Non-null context that owns the real-time stores. /// @return Reference to this builder for method chaining. diff --git a/src/paimon/CMakeLists.txt b/src/paimon/CMakeLists.txt index a25397b98..0a9e1029a 100644 --- a/src/paimon/CMakeLists.txt +++ b/src/paimon/CMakeLists.txt @@ -762,6 +762,7 @@ if(PAIMON_BUILD_TESTS) core/casting/cast_executor_test.cpp core/casting/casted_row_test.cpp core/casting/casting_utils_test.cpp + core/catalog/catalog_snapshot_commit_test.cpp core/catalog/commit_table_request_test.cpp core/catalog/renaming_snapshot_commit_test.cpp core/catalog/file_system_catalog_test.cpp diff --git a/src/paimon/core/catalog/catalog.cpp b/src/paimon/core/catalog/catalog.cpp index 805cc9724..698130416 100644 --- a/src/paimon/core/catalog/catalog.cpp +++ b/src/paimon/core/catalog/catalog.cpp @@ -18,7 +18,10 @@ #include "paimon/catalog/catalog.h" +#include +#include #include +#include #include "fmt/format.h" #include "paimon/catalog_options.h" @@ -64,6 +67,10 @@ Result> Catalog::Create(const std::string& root_path, return std::make_unique(core_options.GetFileSystem(), root_path, options); } +bool Catalog::SupportsVersionManagement() const { + return false; +} + Result> Catalog::GetFormatTable(const Identifier& identifier) const { // Refused here rather than in each override, so every catalog rejects a system table the same // way and in the same order: the database first, so that an identifier of "sys" is rejected diff --git a/src/paimon/core/catalog/catalog_snapshot_commit.h b/src/paimon/core/catalog/catalog_snapshot_commit.h index 27c0c883d..97c3222b2 100644 --- a/src/paimon/core/catalog/catalog_snapshot_commit.h +++ b/src/paimon/core/catalog/catalog_snapshot_commit.h @@ -18,23 +18,67 @@ #pragma once +#include +#include #include +#include #include +#include "paimon/catalog/catalog.h" +#include "paimon/catalog/identifier.h" #include "paimon/core/catalog/commit_table_request.h" #include "paimon/core/catalog/snapshot_commit.h" +#include "paimon/core/catalog/version_managed_catalog.h" namespace paimon { -/// A `SnapshotCommit` using `Catalog` to commit. +/// Commits snapshots through a catalog or prepares requests for an external caller. +/// +/// @note Without a catalog, the caller retrieves the request with `GetLastCommitTableRequest()` +/// and sends it. class CatalogSnapshotCommit : public SnapshotCommit { public: - Result Commit(const Snapshot& snapshot, + /// @param catalog Catalog implementing version management. + /// @param identifier Identifier of the table to commit to. + /// @param table_id Catalog UUID captured before preparing changes; null if unavailable. + CatalogSnapshotCommit(const std::shared_ptr& catalog, Identifier identifier, + const std::optional& table_id) + : catalog_(catalog), + version_managed_catalog_(AsVersionManaged(catalog)), + identifier_(std::move(identifier)), + table_id_(table_id) {} + + /// @param table_id Catalog UUID to include in the request; null if unavailable. + explicit CatalogSnapshotCommit(const std::optional& table_id = std::nullopt) + : identifier_("", ""), table_id_(table_id) {} + + Result Commit(const std::optional& base_snapshot_uuid, + const Snapshot& snapshot, const std::vector& statistics) override { - commit_table_request_ = CommitTableRequest(snapshot, statistics); - return true; + commit_table_request_ = + CommitTableRequest(table_id_, base_snapshot_uuid, snapshot, statistics); + if (catalog_ == nullptr) { + return true; + } + if (version_managed_catalog_ == nullptr) { + return Status::Invalid( + "this catalog does not manage the versions of its tables, so there is nothing to " + "commit the snapshot to"); + } + return version_managed_catalog_->CommitSnapshot(identifier_, table_id_, base_snapshot_uuid, + snapshot, statistics); + } + + std::string DescribeTarget() const override { + return catalog_ == nullptr ? "commit table request built, not sent" + : "through catalog, " + identifier_.ToString(); + } + + bool IsRequestOnly() const override { + return catalog_ == nullptr; } + /// Returns the request from the latest attempt, including refusals and errors. Result GetLastCommitTableRequest() override { if (commit_table_request_) { return commit_table_request_.value().ToJsonString(); @@ -44,6 +88,11 @@ class CatalogSnapshotCommit : public SnapshotCommit { } private: + std::shared_ptr catalog_; + // Owned by catalog_. + VersionManagedCatalog* version_managed_catalog_ = nullptr; + Identifier identifier_; + std::optional table_id_; std::optional commit_table_request_; }; diff --git a/src/paimon/core/catalog/catalog_snapshot_commit_test.cpp b/src/paimon/core/catalog/catalog_snapshot_commit_test.cpp new file mode 100644 index 000000000..bf3b2d639 --- /dev/null +++ b/src/paimon/core/catalog/catalog_snapshot_commit_test.cpp @@ -0,0 +1,146 @@ +/* + * 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. + */ + +#include "paimon/core/catalog/catalog_snapshot_commit.h" + +#include +#include +#include +#include +#include + +#include "gtest/gtest.h" +#include "paimon/catalog/identifier.h" +#include "paimon/core/catalog/commit_table_request.h" +#include "paimon/core/partition/partition_statistics.h" +#include "paimon/result.h" +#include "paimon/status.h" +#include "paimon/testing/mock/mock_catalog.h" +#include "paimon/testing/utils/snapshot_test_helper.h" +#include "paimon/testing/utils/testharness.h" + +namespace paimon::test { + +TEST(CatalogSnapshotCommitTest, TestCommitThroughCatalog) { + auto catalog = std::make_shared(true); + CatalogSnapshotCommit commit(catalog, Identifier("db", "tbl"), "table-uuid"); + ASSERT_FALSE(commit.IsRequestOnly()); + ASSERT_NOK_WITH_MSG(commit.GetLastCommitTableRequest(), + "Should call Commit first before GetLastCommitTableRequest."); + + Snapshot snapshot = BuildTestSnapshot(2, "snapshot-uuid-2"); + std::vector statistics = { + PartitionStatistics({{"f1", "20"}}, 1, 541, 1, 1724090888743, -1), + PartitionStatistics({{"f1", "10"}}, 4, 1118, 2, 1724090888727, 2)}; + ASSERT_OK_AND_ASSIGN(bool success, commit.Commit("base-snapshot-uuid", snapshot, statistics)); + ASSERT_TRUE(success); + + ASSERT_EQ(catalog->CommitCalls().size(), 1u); + const MockVersionManagedCatalog::CommitCall& call = catalog->CommitCalls().front(); + ASSERT_EQ(call.identifier, Identifier("db", "tbl")); + ASSERT_EQ(call.table_uuid, std::optional("table-uuid")); + ASSERT_EQ(call.base_snapshot_uuid, std::optional("base-snapshot-uuid")); + ASSERT_EQ(call.snapshot, snapshot); + ASSERT_EQ(call.statistics, statistics); + ASSERT_NE(commit.DescribeTarget().find("through catalog"), std::string::npos); + + ASSERT_OK_AND_ASSIGN(std::string request_str, commit.GetLastCommitTableRequest()); + ASSERT_OK_AND_ASSIGN(CommitTableRequest request, + CommitTableRequest::FromJsonString(request_str)); + ASSERT_EQ(request, + CommitTableRequest("table-uuid", "base-snapshot-uuid", snapshot, statistics)); + + Snapshot next_snapshot = BuildTestSnapshot(3, "snapshot-uuid-3"); + ASSERT_OK_AND_ASSIGN(success, commit.Commit(snapshot.Uuid(), next_snapshot, {})); + ASSERT_TRUE(success); + ASSERT_EQ(catalog->CommitCalls().size(), 2u); + const MockVersionManagedCatalog::CommitCall& next_call = catalog->CommitCalls().back(); + ASSERT_EQ(next_call.identifier, Identifier("db", "tbl")); + ASSERT_EQ(next_call.table_uuid, std::optional("table-uuid")); + ASSERT_EQ(next_call.base_snapshot_uuid, snapshot.Uuid()); + ASSERT_EQ(next_call.snapshot, next_snapshot); + ASSERT_TRUE(next_call.statistics.empty()); + ASSERT_OK_AND_ASSIGN(std::string next_request_str, commit.GetLastCommitTableRequest()); + ASSERT_OK_AND_ASSIGN(CommitTableRequest next_request, + CommitTableRequest::FromJsonString(next_request_str)); + ASSERT_EQ(next_request, CommitTableRequest("table-uuid", snapshot.Uuid(), next_snapshot, {})); +} + +TEST(CatalogSnapshotCommitTest, TestLostRaceIsNotAnError) { + auto catalog = std::make_shared(false); + CatalogSnapshotCommit commit(catalog, Identifier("db", "tbl"), std::nullopt); + ASSERT_OK_AND_ASSIGN( + bool success, commit.Commit("base-snapshot-uuid", BuildTestSnapshot(2, std::nullopt), {})); + ASSERT_FALSE(success); + ASSERT_EQ(catalog->CommitCalls().size(), 1u); + ASSERT_EQ(catalog->CommitCalls().front().table_uuid, std::nullopt); +} + +TEST(CatalogSnapshotCommitTest, TestCatalogFailurePropagates) { + auto catalog = + std::make_shared(Status::IOError("catalog unreachable")); + CatalogSnapshotCommit commit(catalog, Identifier("db", "tbl"), "table-uuid"); + ASSERT_NOK_WITH_MSG(commit.Commit(std::nullopt, BuildTestSnapshot(1, std::nullopt), {}), + "catalog unreachable"); + + ASSERT_OK_AND_ASSIGN(std::string request_str, commit.GetLastCommitTableRequest()); + ASSERT_OK_AND_ASSIGN(CommitTableRequest request, + CommitTableRequest::FromJsonString(request_str)); + ASSERT_EQ(request.GetTableId(), std::optional("table-uuid")); + ASSERT_EQ(request.GetSnapshot().Id(), 1); +} + +TEST(CatalogSnapshotCommitTest, TestCatalogWhichDoesNotManageVersionsIsRefused) { + auto catalog = std::make_shared(); + catalog->SetSupportsVersionManagement(false); + CatalogSnapshotCommit commit(catalog, Identifier("db", "tbl"), "table-uuid"); + ASSERT_NOK_WITH_MSG(commit.Commit(std::nullopt, BuildTestSnapshot(1, std::nullopt), {}), + "does not manage the versions of its tables"); + ASSERT_TRUE(catalog->CommitCalls().empty()); +} + +TEST(CatalogSnapshotCommitTest, TestBuildRequestWithoutCatalog) { + CatalogSnapshotCommit commit("table-uuid"); + ASSERT_TRUE(commit.IsRequestOnly()); + ASSERT_NOK_WITH_MSG(commit.GetLastCommitTableRequest(), + "Should call Commit first before GetLastCommitTableRequest."); + ASSERT_EQ(commit.DescribeTarget(), "commit table request built, not sent"); + + Snapshot snapshot = BuildTestSnapshot(2, std::nullopt); + ASSERT_OK_AND_ASSIGN(bool success, commit.Commit("base-snapshot-uuid", snapshot, {})); + ASSERT_TRUE(success); + + ASSERT_OK_AND_ASSIGN(std::string request_str, commit.GetLastCommitTableRequest()); + ASSERT_OK_AND_ASSIGN(CommitTableRequest request, + CommitTableRequest::FromJsonString(request_str)); + ASSERT_EQ(request.GetTableId(), std::optional("table-uuid")); + ASSERT_EQ(request.GetBaseSnapshotUuid(), std::optional("base-snapshot-uuid")); + ASSERT_EQ(request.GetSnapshot().Id(), 2); + + CatalogSnapshotCommit bare_commit; + ASSERT_OK_AND_ASSIGN(bool bare_success, + bare_commit.Commit(std::nullopt, BuildTestSnapshot(1, std::nullopt), {})); + ASSERT_TRUE(bare_success); + ASSERT_OK_AND_ASSIGN(std::string bare_request_str, bare_commit.GetLastCommitTableRequest()); + ASSERT_OK_AND_ASSIGN(CommitTableRequest bare_request, + CommitTableRequest::FromJsonString(bare_request_str)); + ASSERT_EQ(bare_request.GetTableId(), std::nullopt); + ASSERT_EQ(bare_request.GetBaseSnapshotUuid(), std::nullopt); +} + +} // namespace paimon::test diff --git a/src/paimon/core/catalog/commit_table_request.h b/src/paimon/core/catalog/commit_table_request.h index fa53789ac..c336a7944 100644 --- a/src/paimon/core/catalog/commit_table_request.h +++ b/src/paimon/core/catalog/commit_table_request.h @@ -19,6 +19,7 @@ #pragma once #include +#include #include #include @@ -32,14 +33,45 @@ namespace paimon { +/// Request to commit a table snapshot and its partition statistics. class CommitTableRequest : public Jsonizable { public: - CommitTableRequest(const Snapshot& snapshot, const std::vector& statistics) - : snapshot_(snapshot), statistics_(statistics) {} + /// @param table_id Catalog table UUID used to detect table recreation; null if unavailable. + /// @param base_snapshot_uuid Base snapshot UUID; null for an absent or legacy snapshot. + /// @param snapshot Snapshot to be committed. + /// @param statistics Partition statistics for this change. + CommitTableRequest(const std::optional& table_id, + const std::optional& base_snapshot_uuid, + const Snapshot& snapshot, const std::vector& statistics) + : table_id_(table_id), + base_snapshot_uuid_(base_snapshot_uuid), + snapshot_(snapshot), + statistics_(statistics) {} + + const std::optional& GetTableId() const { + return table_id_; + } + + const std::optional& GetBaseSnapshotUuid() const { + return base_snapshot_uuid_; + } + + const Snapshot& GetSnapshot() const { + return snapshot_; + } + + const std::vector& GetStatistics() const { + return statistics_; + } rapidjson::Value ToJson(rapidjson::Document::AllocatorType* allocator) const noexcept(false) override { rapidjson::Value obj(rapidjson::kObjectType); + obj.AddMember(rapidjson::StringRef(FIELD_TABLE_ID), + RapidJsonUtil::SerializeValue(table_id_, allocator).Move(), *allocator); + obj.AddMember(rapidjson::StringRef(FIELD_BASE_SNAPSHOT_UUID), + RapidJsonUtil::SerializeValue(base_snapshot_uuid_, allocator).Move(), + *allocator); obj.AddMember(rapidjson::StringRef(FIELD_SNAPSHOT), RapidJsonUtil::SerializeValue(snapshot_, allocator).Move(), *allocator); obj.AddMember(rapidjson::StringRef(FIELD_STATISTICS), @@ -48,6 +80,10 @@ class CommitTableRequest : public Jsonizable { } void FromJson(const rapidjson::Value& obj) noexcept(false) override { + table_id_ = + RapidJsonUtil::DeserializeKeyValue>(obj, FIELD_TABLE_ID); + base_snapshot_uuid_ = RapidJsonUtil::DeserializeKeyValue>( + obj, FIELD_BASE_SNAPSHOT_UUID); snapshot_ = RapidJsonUtil::DeserializeKeyValue(obj, FIELD_SNAPSHOT); statistics_ = RapidJsonUtil::DeserializeKeyValue>( obj, FIELD_STATISTICS); @@ -57,20 +93,26 @@ class CommitTableRequest : public Jsonizable { if (this == &other) { return true; } - return snapshot_.TEST_Equal(other.snapshot_) && statistics_ == other.statistics_; + return table_id_ == other.table_id_ && base_snapshot_uuid_ == other.base_snapshot_uuid_ && + snapshot_.TEST_Equal(other.snapshot_) && statistics_ == other.statistics_; } bool operator==(const CommitTableRequest& other) const { - return snapshot_ == other.snapshot_ && statistics_ == other.statistics_; + return table_id_ == other.table_id_ && base_snapshot_uuid_ == other.base_snapshot_uuid_ && + snapshot_ == other.snapshot_ && statistics_ == other.statistics_; } private: JSONIZABLE_FRIEND_AND_DEFAULT_CTOR(CommitTableRequest); private: + static constexpr const char* FIELD_TABLE_ID = "tableId"; + static constexpr const char* FIELD_BASE_SNAPSHOT_UUID = "baseSnapshotUuid"; static constexpr const char* FIELD_SNAPSHOT = "snapshot"; static constexpr const char* FIELD_STATISTICS = "statistics"; + std::optional table_id_; + std::optional base_snapshot_uuid_; Snapshot snapshot_; std::vector statistics_; }; diff --git a/src/paimon/core/catalog/commit_table_request_test.cpp b/src/paimon/core/catalog/commit_table_request_test.cpp index deafee36b..8f5553044 100644 --- a/src/paimon/core/catalog/commit_table_request_test.cpp +++ b/src/paimon/core/catalog/commit_table_request_test.cpp @@ -18,30 +18,20 @@ #include "paimon/core/catalog/commit_table_request.h" -#include #include #include +#include #include "gtest/gtest.h" #include "paimon/result.h" #include "paimon/status.h" +#include "paimon/testing/utils/snapshot_test_helper.h" #include "paimon/testing/utils/testharness.h" namespace paimon::test { TEST(CommitTableRequestTest, TestSimple) { - Snapshot snapshot( - /*version=*/3, /*id=*/1, /*schema_id=*/0, - /*base_manifest_list=*/"manifest-list-3879e56f-2f27-49ae-a2f3-3dcbb8eb0beb-0", - /*base_manifest_list_size=*/291, - /*delta_manifest_list=*/"manifest-list-3879e56f-2f27-49ae-a2f3-3dcbb8eb0beb-1", - /*delta_manifest_list_size=*/1342, /*changelog_manifest_list=*/std::nullopt, - /*changelog_manifest_list_size=*/std::nullopt, /*index_manifest=*/std::nullopt, - /*commit_user=*/"commit_user_1", /*commit_identifier=*/9223372036854775807, - /*commit_kind=*/Snapshot::CommitKind::Append(), /*time_millis=*/1758097357597, - /*total_record_count=*/5, - /*delta_record_count=*/5, /*changelog_record_count=*/0, /*watermark=*/std::nullopt, - /*statistics=*/std::nullopt, /*properties=*/std::nullopt, /*next_row_id=*/0); + Snapshot snapshot = BuildTestSnapshot(1); std::vector partition_statistics = { PartitionStatistics(/*spec=*/{{"f1", "20"}}, /*record_count=*/1, /*file_size_in_bytes=*/541, /*file_count=*/1, /*last_file_creation_time=*/1724090888743, @@ -50,6 +40,8 @@ TEST(CommitTableRequestTest, TestSimple) { /*file_size_in_bytes=*/1118, /*file_count=*/2, /*last_file_creation_time=*/1724090888727, /*total_buckets=*/-1)}; std::string expected_request_str = R"({ + "tableId": "table-uuid", + "baseSnapshotUuid": "base-snapshot-uuid", "snapshot": { "version": 3, "id": 1, @@ -91,14 +83,58 @@ TEST(CommitTableRequestTest, TestSimple) { ] })"; - CommitTableRequest request1(snapshot, partition_statistics); + CommitTableRequest request1("table-uuid", "base-snapshot-uuid", snapshot, partition_statistics); ASSERT_OK_AND_ASSIGN(std::string request_str1, request1.ToJsonString()); ASSERT_EQ(request_str1, expected_request_str); ASSERT_OK_AND_ASSIGN(CommitTableRequest request2, CommitTableRequest::FromJsonString(request_str1)); ASSERT_EQ(request1, request2); + ASSERT_EQ(request2.GetTableId(), std::optional("table-uuid")); + ASSERT_EQ(request2.GetBaseSnapshotUuid(), std::optional("base-snapshot-uuid")); ASSERT_OK_AND_ASSIGN(std::string request_str2, request2.ToJsonString()); ASSERT_EQ(request_str1, request_str2); } +TEST(CommitTableRequestTest, TestNullTableIdAndBaseSnapshotUuid) { + Snapshot snapshot = BuildTestSnapshot(1); + + std::string expected_request_str = R"({ + "tableId": null, + "baseSnapshotUuid": null, + "snapshot": { + "version": 3, + "id": 1, + "schemaId": 0, + "baseManifestList": "manifest-list-3879e56f-2f27-49ae-a2f3-3dcbb8eb0beb-0", + "baseManifestListSize": 291, + "deltaManifestList": "manifest-list-3879e56f-2f27-49ae-a2f3-3dcbb8eb0beb-1", + "deltaManifestListSize": 1342, + "commitUser": "commit_user_1", + "commitIdentifier": 9223372036854775807, + "commitKind": "APPEND", + "timeMillis": 1758097357597, + "totalRecordCount": 5, + "deltaRecordCount": 5, + "changelogRecordCount": 0, + "nextRowId": 0 + }, + "statistics": [] +})"; + + CommitTableRequest request(std::nullopt, std::nullopt, snapshot, {}); + ASSERT_OK_AND_ASSIGN(std::string request_str, request.ToJsonString()); + ASSERT_EQ(request_str, expected_request_str); + ASSERT_OK_AND_ASSIGN(CommitTableRequest parsed, + CommitTableRequest::FromJsonString(request_str)); + ASSERT_EQ(parsed.GetTableId(), std::nullopt); + ASSERT_EQ(parsed.GetBaseSnapshotUuid(), std::nullopt); + ASSERT_EQ(request, parsed); + + ASSERT_OK_AND_ASSIGN(std::string snapshot_str, snapshot.ToJsonString()); + ASSERT_OK_AND_ASSIGN(CommitTableRequest without_fields, + CommitTableRequest::FromJsonString(R"({"snapshot": )" + snapshot_str + + R"(, "statistics": []})")); + ASSERT_EQ(without_fields, parsed); +} + } // namespace paimon::test diff --git a/src/paimon/core/catalog/file_system_catalog_test.cpp b/src/paimon/core/catalog/file_system_catalog_test.cpp index bdfcf92d0..1b30c9cfe 100644 --- a/src/paimon/core/catalog/file_system_catalog_test.cpp +++ b/src/paimon/core/catalog/file_system_catalog_test.cpp @@ -26,13 +26,16 @@ #include "arrow/c/helpers.h" #include "gtest/gtest.h" #include "paimon/catalog/identifier.h" +#include "paimon/commit_context.h" #include "paimon/common/data/blob_utils.h" #include "paimon/common/utils/path_util.h" +#include "paimon/core/catalog/version_managed_catalog.h" #include "paimon/core/core_options.h" #include "paimon/core/schema/table_schema.h" #include "paimon/core/table/system/global_system_tables.h" #include "paimon/core/table/system/system_table_schema.h" #include "paimon/defs.h" +#include "paimon/file_store_commit.h" #include "paimon/fs/file_system.h" #include "paimon/fs/file_system_factory.h" #include "paimon/snapshot/snapshot_info.h" @@ -1235,6 +1238,58 @@ TEST(FileSystemCatalogTest, TestListSnapshots) { ASSERT_LT(snapshots[0].snapshot_id, snapshots[1].snapshot_id); } +namespace { +class ClaimsVersionManagementCatalog : public FileSystemCatalog { + public: + using FileSystemCatalog::FileSystemCatalog; + + bool SupportsVersionManagement() const override { + return true; + } +}; +} // namespace + +TEST(FileSystemCatalogTest, TestClaimingVersionManagementWithoutImplementingItIsRefused) { + std::map options; + options[Options::FILE_SYSTEM] = "local"; + ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options)); + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + std::shared_ptr claiming = std::make_shared( + core_options.GetFileSystem(), dir->Str(), options); + + ASSERT_TRUE(claiming->SupportsVersionManagement()); + ASSERT_EQ(AsVersionManaged(claiming), nullptr); + ASSERT_NOK_WITH_MSG(CheckVersionManagementImplemented(claiming), + "does not implement VersionManagedCatalog"); + + CommitContextBuilder claiming_builder(dir->Str(), "commit_user_1"); + ASSERT_OK_AND_ASSIGN(std::unique_ptr claiming_context, + claiming_builder.SetOptions(options) + .WithCatalog(claiming, Identifier("db", "tbl")) + .Finish()); + ASSERT_NOK_WITH_MSG(FileStoreCommit::Create(std::move(claiming_context)), + "does not implement VersionManagedCatalog"); + + std::shared_ptr plain = + std::make_shared(core_options.GetFileSystem(), dir->Str(), options); + ASSERT_OK(CheckVersionManagementImplemented(plain)); + ASSERT_OK(CheckVersionManagementImplemented(nullptr)); +} + +TEST(FileSystemCatalogTest, TestDoesNotManageVersions) { + std::map options; + options[Options::FILE_SYSTEM] = "local"; + ASSERT_OK_AND_ASSIGN(auto core_options, CoreOptions::FromMap(options)); + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + std::shared_ptr catalog = + std::make_shared(core_options.GetFileSystem(), dir->Str(), options); + + ASSERT_FALSE(catalog->SupportsVersionManagement()); + ASSERT_EQ(AsVersionManaged(catalog), nullptr); +} + TEST(FileSystemCatalogTest, TestListSnapshotsTableNotExist) { std::map options; options[Options::FILE_SYSTEM] = "local"; diff --git a/src/paimon/core/catalog/renaming_snapshot_commit.h b/src/paimon/core/catalog/renaming_snapshot_commit.h index e493fe372..bda2aad41 100644 --- a/src/paimon/core/catalog/renaming_snapshot_commit.h +++ b/src/paimon/core/catalog/renaming_snapshot_commit.h @@ -19,6 +19,7 @@ #pragma once #include +#include #include #include #include @@ -44,7 +45,9 @@ class RenamingSnapshotCommit : public SnapshotCommit { const std::shared_ptr& snapshot_manager) : fs_(fs), snapshot_manager_(snapshot_manager) {} - Result Commit(const Snapshot& snapshot, + /// @note The atomic rename detects conflicts by snapshot ID, so `base_snapshot_uuid` is unused. + Result Commit(const std::optional& base_snapshot_uuid, + const Snapshot& snapshot, const std::vector& statistics) override { PAIMON_ASSIGN_OR_RAISE(std::string json_str, snapshot.ToJsonString()); std::string snapshot_path = snapshot_manager_->SnapshotPath(snapshot.Id()); diff --git a/src/paimon/core/catalog/renaming_snapshot_commit_test.cpp b/src/paimon/core/catalog/renaming_snapshot_commit_test.cpp index c415390a2..e2768b1c0 100644 --- a/src/paimon/core/catalog/renaming_snapshot_commit_test.cpp +++ b/src/paimon/core/catalog/renaming_snapshot_commit_test.cpp @@ -18,14 +18,13 @@ #include "paimon/core/catalog/renaming_snapshot_commit.h" -#include -#include #include #include "gtest/gtest.h" #include "paimon/common/utils/path_util.h" #include "paimon/core/partition/partition_statistics.h" #include "paimon/fs/local/local_file_system.h" +#include "paimon/testing/utils/snapshot_test_helper.h" #include "paimon/testing/utils/testharness.h" namespace paimon::test { @@ -39,19 +38,8 @@ TEST(RenamingSnapshotCommitTest, TestSimple) { auto commit = std::make_shared(fs, snapshot_manager); ASSERT_NOK_WITH_MSG(commit->GetLastCommitTableRequest(), "renaming snapshot commit do not support get last commit table request"); - Snapshot snapshot( - /*version=*/3, /*id=*/1, /*schema_id=*/0, - /*base_manifest_list=*/"manifest-list-3879e56f-2f27-49ae-a2f3-3dcbb8eb0beb-0", - /*base_manifest_list_size=*/291, - /*delta_manifest_list=*/"manifest-list-3879e56f-2f27-49ae-a2f3-3dcbb8eb0beb-1", - /*delta_manifest_list_size=*/1342, /*changelog_manifest_list=*/std::nullopt, - /*changelog_manifest_list_size=*/std::nullopt, /*index_manifest=*/std::nullopt, - /*commit_user=*/"commit_user_1", /*commit_identifier=*/9223372036854775807, - /*commit_kind=*/Snapshot::CommitKind::Append(), /*time_millis=*/1758097357597, - /*total_record_count=*/5, - /*delta_record_count=*/5, /*changelog_record_count=*/0, /*watermark=*/std::nullopt, - /*statistics=*/std::nullopt, /*properties=*/std::nullopt, /*next_row_id=*/0); - ASSERT_OK_AND_ASSIGN(bool success, commit->Commit(snapshot, /*statistics=*/{})); + Snapshot snapshot = BuildTestSnapshot(1); + ASSERT_OK_AND_ASSIGN(bool success, commit->Commit(std::nullopt, snapshot, {})); ASSERT_TRUE(success); ASSERT_OK_AND_ASSIGN(bool exist, fs->Exists(PathUtil::JoinPath(dir->Str(), "snapshot/snapshot-1"))); @@ -60,7 +48,7 @@ TEST(RenamingSnapshotCommitTest, TestSimple) { fs->Exists(PathUtil::JoinPath(dir->Str(), "snapshot/LATEST"))); ASSERT_TRUE(exist1); // duplicate commit for snapshot-1 - ASSERT_OK_AND_ASSIGN(bool success1, commit->Commit(snapshot, /*statistics=*/{})); + ASSERT_OK_AND_ASSIGN(bool success1, commit->Commit("snapshot-uuid-0", snapshot, {})); ASSERT_FALSE(success1); } diff --git a/src/paimon/core/catalog/snapshot_commit.h b/src/paimon/core/catalog/snapshot_commit.h index 0a5dc8876..08664f67b 100644 --- a/src/paimon/core/catalog/snapshot_commit.h +++ b/src/paimon/core/catalog/snapshot_commit.h @@ -18,6 +18,7 @@ #pragma once +#include #include #include @@ -31,10 +32,24 @@ class SnapshotCommit { public: virtual ~SnapshotCommit() = default; - virtual Result Commit(const Snapshot& snapshot, + /// @param base_snapshot_uuid Base snapshot UUID; null for an absent or legacy snapshot. + /// @param snapshot Snapshot to be committed. + /// @param statistics Partition statistics for this change. + virtual Result Commit(const std::optional& base_snapshot_uuid, + const Snapshot& snapshot, const std::vector& statistics) = 0; virtual Result GetLastCommitTableRequest() = 0; + + /// Returns whether a successful call only prepares a request for the caller to send. + virtual bool IsRequestOnly() const { + return false; + } + + /// Returns a log description of the commit target; empty for file-system commits. + virtual std::string DescribeTarget() const { + return ""; + } }; } // namespace paimon diff --git a/src/paimon/core/catalog/version_managed_catalog.h b/src/paimon/core/catalog/version_managed_catalog.h new file mode 100644 index 000000000..16c4aeb6a --- /dev/null +++ b/src/paimon/core/catalog/version_managed_catalog.h @@ -0,0 +1,84 @@ +/* + * 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 +#include +#include +#include + +#include "paimon/catalog/catalog.h" +#include "paimon/catalog/identifier.h" +#include "paimon/core/partition/partition_statistics.h" +#include "paimon/core/snapshot.h" +#include "paimon/result.h" +#include "paimon/status.h" + +namespace paimon { + +/// Snapshot publication interface for catalogs returning true from `SupportsVersionManagement()`. +/// +/// Kept internal because `Snapshot` and `PartitionStatistics` are not installed API types. +class VersionManagedCatalog { + public: + virtual ~VersionManagedCatalog() = default; + + /// Loads the current snapshot used as the base for commits. + /// + /// @param identifier Identifier (database and table name) of the table to read. + /// @return The current snapshot, or null when the table has none yet. + virtual Result> LoadSnapshot(const Identifier& identifier) const = 0; + + /// Atomically commits a snapshot if its table and base UUIDs match the catalog state. + /// + /// @param identifier Identifier (database and table name) of the table to commit to. + /// @param table_uuid Catalog table UUID used to detect table recreation; null if unavailable. + /// @param base_snapshot_uuid Base snapshot UUID; null for an absent or legacy snapshot. + /// @param snapshot The snapshot to commit. + /// @param statistics Partition statistics for this change. + /// @return True on success, false on a conflict requiring rebase, or an error on failure. + /// Errors leave publication uncertain, so manifests must be preserved for recovery. + virtual Result CommitSnapshot(const Identifier& identifier, + const std::optional& table_uuid, + const std::optional& base_snapshot_uuid, + const Snapshot& snapshot, + const std::vector& statistics) = 0; +}; + +/// Returns version management only when the catalog both advertises and implements it. +inline VersionManagedCatalog* AsVersionManaged(const std::shared_ptr& catalog) { + if (catalog == nullptr || !catalog->SupportsVersionManagement()) { + return nullptr; + } + return dynamic_cast(catalog.get()); +} + +/// Prevents file-system fallback when a catalog advertises version management without implementing +/// it. +inline Status CheckVersionManagementImplemented(const std::shared_ptr& catalog) { + if (catalog != nullptr && catalog->SupportsVersionManagement() && + dynamic_cast(catalog.get()) == nullptr) { + return Status::Invalid( + "this catalog reports that it manages the versions of its tables but does not " + "implement VersionManagedCatalog, so there is nothing to commit the snapshot to"); + } + return Status::OK(); +} + +} // namespace paimon diff --git a/src/paimon/core/operation/append_only_file_store_write.cpp b/src/paimon/core/operation/append_only_file_store_write.cpp index 55853de97..874fdb3e2 100644 --- a/src/paimon/core/operation/append_only_file_store_write.cpp +++ b/src/paimon/core/operation/append_only_file_store_write.cpp @@ -126,7 +126,10 @@ Status AppendOnlyFileStoreWrite::RefreshCommittedSnapshot(int64_t snapshot_id) { if (!realtime_context_) { return Status::Invalid("refresh committed snapshot requires a real-time writer"); } - PAIMON_ASSIGN_OR_RAISE(Snapshot snapshot, snapshot_manager_->LoadSnapshot(snapshot_id)); + PAIMON_ASSIGN_OR_RAISE(std::optional latest, snapshot_manager_->LatestSnapshot()); + PAIMON_ASSIGN_OR_RAISE(Snapshot snapshot, latest && latest->Id() == snapshot_id + ? Result(latest.value()) + : snapshot_manager_->LoadSnapshot(snapshot_id)); PAIMON_ASSIGN_OR_RAISE( RealtimeOffsetMap committed_offsets, RealtimeCommitProperties::ReadOffsets(std::optional(std::move(snapshot)), diff --git a/src/paimon/core/operation/commit/conflict_detection.cpp b/src/paimon/core/operation/commit/conflict_detection.cpp index 45411184d..fbea33285 100644 --- a/src/paimon/core/operation/commit/conflict_detection.cpp +++ b/src/paimon/core/operation/commit/conflict_detection.cpp @@ -636,8 +636,12 @@ Status ConflictDetection::CheckForRowIdFromSnapshot( return Status::OK(); } - PAIMON_ASSIGN_OR_RAISE(Snapshot check_snapshot, - snapshot_manager_->LoadSnapshot(row_id_check_from_snapshot_.value())); + // The latest snapshot may only exist in the catalog. Reuse the one this attempt is based on. + PAIMON_ASSIGN_OR_RAISE( + Snapshot check_snapshot, + row_id_check_from_snapshot_.value() == latest_snapshot.Id() + ? Result(latest_snapshot) + : snapshot_manager_->LoadSnapshot(row_id_check_from_snapshot_.value())); if (!check_snapshot.NextRowId()) { return Status::Invalid(fmt::format("Next row id cannot be null for snapshot {}.", row_id_check_from_snapshot_.value())); @@ -662,7 +666,10 @@ Status ConflictDetection::CheckForRowIdFromSnapshot( for (int64_t snapshot_id = from_snapshot_id; snapshot_id <= latest_snapshot.Id(); ++snapshot_id) { - PAIMON_ASSIGN_OR_RAISE(Snapshot snapshot, snapshot_manager_->LoadSnapshot(snapshot_id)); + PAIMON_ASSIGN_OR_RAISE(Snapshot snapshot, + snapshot_id == latest_snapshot.Id() + ? Result(latest_snapshot) + : snapshot_manager_->LoadSnapshot(snapshot_id)); if (snapshot.GetCommitKind() == Snapshot::CommitKind::Compact()) { continue; } diff --git a/src/paimon/core/operation/commit_context.cpp b/src/paimon/core/operation/commit_context.cpp index 2b62fdc2c..f4e80c404 100644 --- a/src/paimon/core/operation/commit_context.cpp +++ b/src/paimon/core/operation/commit_context.cpp @@ -19,9 +19,15 @@ #include "paimon/commit_context.h" +#include +#include #include +#include "fmt/format.h" +#include "paimon/catalog/catalog.h" #include "paimon/common/utils/path_util.h" +#include "paimon/core/utils/branch_manager.h" +#include "paimon/defs.h" #include "paimon/executor.h" #include "paimon/memory/memory_pool.h" #include "paimon/result.h" @@ -38,10 +44,29 @@ CommitContext::CommitContext(const std::string& root_path, const std::string& co const std::shared_ptr& specific_file_system, const std::map& options, const std::shared_ptr& format_table) + : CommitContext(root_path, commit_user, ignore_empty_commit, use_rest_catalog_commit, + /*catalog=*/nullptr, /*identifier=*/std::nullopt, /*table_id=*/std::nullopt, + append_commit_check_conflict, memory_pool, executor, specific_file_system, + options, format_table) {} + +CommitContext::CommitContext(const std::string& root_path, const std::string& commit_user, + bool ignore_empty_commit, bool use_rest_catalog_commit, + const std::shared_ptr& catalog, + const std::optional& identifier, + const std::optional& table_id, + bool append_commit_check_conflict, + const std::shared_ptr& memory_pool, + const std::shared_ptr& executor, + const std::shared_ptr& specific_file_system, + const std::map& options, + const std::shared_ptr& format_table) : root_path_(root_path), commit_user_(commit_user), ignore_empty_commit_(ignore_empty_commit), use_rest_catalog_commit_(use_rest_catalog_commit), + catalog_(catalog), + identifier_(identifier), + table_id_(table_id), append_commit_check_conflict_(append_commit_check_conflict), memory_pool_(memory_pool), executor_(executor), @@ -58,6 +83,9 @@ class CommitContextBuilder::Impl { void Reset() { ignore_empty_commit_ = true; use_rest_catalog_commit_ = false; + catalog_.reset(); + identifier_.reset(); + table_id_.reset(); append_commit_check_conflict_ = false; memory_pool_ = GetDefaultPool(); executor_ = CreateDefaultExecutor(); @@ -74,6 +102,9 @@ class CommitContextBuilder::Impl { std::string commit_user_; bool ignore_empty_commit_ = true; bool use_rest_catalog_commit_ = false; + std::shared_ptr catalog_; + std::optional identifier_; + std::optional table_id_; bool append_commit_check_conflict_ = false; std::shared_ptr memory_pool_ = GetDefaultPool(); std::shared_ptr executor_ = CreateDefaultExecutor(); @@ -121,6 +152,18 @@ CommitContextBuilder& CommitContextBuilder::UseRESTCatalogCommit(bool use_rest_c return *this; } +CommitContextBuilder& CommitContextBuilder::WithCatalog(const std::shared_ptr& catalog, + const Identifier& identifier) { + impl_->catalog_ = catalog; + impl_->identifier_.emplace(identifier); + return *this; +} + +CommitContextBuilder& CommitContextBuilder::WithTableId(const std::string& table_id) { + impl_->table_id_ = table_id; + return *this; +} + CommitContextBuilder& CommitContextBuilder::AppendCommitCheckConflict( bool append_commit_check_conflict) { impl_->append_commit_check_conflict_ = append_commit_check_conflict; @@ -149,6 +192,33 @@ Result> CommitContextBuilder::Finish() { if (impl_->built_from_format_table_ && impl_->format_table_ == nullptr) { return Status::Invalid("cannot commit with null format table"); } + if (impl_->catalog_ == nullptr && impl_->identifier_) { + return Status::Invalid("cannot commit through a null catalog"); + } + // Validate both branch sources: metadata paths are built for the main branch. + if (impl_->catalog_ != nullptr) { + PAIMON_ASSIGN_OR_RAISE(std::optional identifier_branch, + impl_->identifier_.value().GetBranchName()); + auto branch_option = impl_->options_.find(Options::BRANCH); + std::optional option_branch = + branch_option == impl_->options_.end() + ? std::nullopt + : std::optional(branch_option->second); + for (const std::optional& branch : {identifier_branch, option_branch}) { + if (branch && + !BranchManager::IsMainBranch(BranchManager::NormalizeBranch(branch.value()))) { + return Status::Invalid(fmt::format( + "a commit through a catalog reads the main branch of the table, so it cannot " + "be aimed at branch '{}'", + branch.value())); + } + } + } + if (impl_->catalog_ != nullptr && impl_->use_rest_catalog_commit_) { + return Status::Invalid( + "a commit either goes to the catalog of WithCatalog() or is handed back as a request " + "for the caller to send, which is what UseRESTCatalogCommit() asks for"); + } // The table already carries the file system it was loaded through, and from a source this // cannot see behind, so a second answer is refused rather than silently dropped. if (impl_->format_table_ != nullptr && impl_->specific_file_system_ != nullptr) { @@ -162,8 +232,9 @@ Result> CommitContextBuilder::Finish() { } auto ctx = std::make_unique( impl_->root_path_, impl_->commit_user_, impl_->ignore_empty_commit_, - impl_->use_rest_catalog_commit_, impl_->append_commit_check_conflict_, impl_->memory_pool_, - impl_->executor_, impl_->specific_file_system_, impl_->options_, impl_->format_table_); + impl_->use_rest_catalog_commit_, impl_->catalog_, impl_->identifier_, impl_->table_id_, + impl_->append_commit_check_conflict_, impl_->memory_pool_, impl_->executor_, + impl_->specific_file_system_, impl_->options_, impl_->format_table_); impl_->Reset(); return ctx; } diff --git a/src/paimon/core/operation/commit_context_test.cpp b/src/paimon/core/operation/commit_context_test.cpp index e790958f5..2d3915c67 100644 --- a/src/paimon/core/operation/commit_context_test.cpp +++ b/src/paimon/core/operation/commit_context_test.cpp @@ -19,15 +19,42 @@ #include "paimon/commit_context.h" +#include +#include + #include "gtest/gtest.h" +#include "paimon/catalog/identifier.h" #include "paimon/common/utils/path_util.h" +#include "paimon/defs.h" #include "paimon/executor.h" #include "paimon/memory/memory_pool.h" +#include "paimon/testing/mock/mock_catalog.h" #include "paimon/testing/mock/mock_file_system.h" #include "paimon/testing/utils/testharness.h" namespace paimon::test { +TEST(CommitContextTest, TestLegacyConstructor) { + auto pool = GetDefaultPool(); + std::shared_ptr executor = CreateDefaultExecutor(); + auto fs = std::make_shared(); + const std::map options = {{"key", "value"}}; + CommitContext context("table_root_path", "commit_user_1", false, true, true, pool, executor, fs, + options, nullptr); + ASSERT_EQ(context.GetRootPath(), "table_root_path"); + ASSERT_EQ(context.GetCommitUser(), "commit_user_1"); + ASSERT_FALSE(context.IgnoreEmptyCommit()); + ASSERT_TRUE(context.UseRESTCatalogCommit()); + ASSERT_TRUE(context.AppendCommitCheckConflict()); + ASSERT_EQ(context.GetMemoryPool(), pool); + ASSERT_EQ(context.GetExecutor(), executor); + ASSERT_EQ(context.GetSpecificFileSystem(), fs); + ASSERT_EQ(context.GetOptions(), options); + ASSERT_EQ(context.GetCatalog(), nullptr); + ASSERT_FALSE(context.GetIdentifier()); + ASSERT_FALSE(context.GetTableId()); +} + TEST(CommitContextTest, TestDefaultValue) { CommitContextBuilder builder("table_root_path", "commit_user_1"); @@ -38,6 +65,9 @@ TEST(CommitContextTest, TestDefaultValue) { ASSERT_EQ(ctx->GetCommitUser(), "commit_user_1"); ASSERT_TRUE(ctx->IgnoreEmptyCommit()); ASSERT_FALSE(ctx->UseRESTCatalogCommit()); + ASSERT_EQ(ctx->GetCatalog(), nullptr); + ASSERT_FALSE(ctx->GetIdentifier().has_value()); + ASSERT_EQ(ctx->GetTableId(), std::nullopt); ASSERT_FALSE(ctx->AppendCommitCheckConflict()); ASSERT_TRUE(ctx->GetMemoryPool()); ASSERT_TRUE(ctx->GetExecutor()); @@ -54,6 +84,7 @@ TEST(CommitContextTest, TestSetContent) { ASSERT_OK_AND_ASSIGN(auto ctx, builder.IgnoreEmptyCommit(false) .UseRESTCatalogCommit(true) + .WithTableId("table-uuid") .AppendCommitCheckConflict(true) .WithMemoryPool(memory_pool) .WithExecutor(executor) @@ -66,6 +97,7 @@ TEST(CommitContextTest, TestSetContent) { ASSERT_EQ(ctx->GetCommitUser(), "commit_user_1"); ASSERT_FALSE(ctx->IgnoreEmptyCommit()); ASSERT_TRUE(ctx->UseRESTCatalogCommit()); + ASSERT_EQ(ctx->GetTableId(), std::optional("table-uuid")); ASSERT_TRUE(ctx->AppendCommitCheckConflict()); ASSERT_EQ(ctx->GetMemoryPool(), memory_pool); ASSERT_EQ(ctx->GetExecutor(), executor); @@ -75,6 +107,59 @@ TEST(CommitContextTest, TestSetContent) { ASSERT_EQ(ctx->GetOptions(), expected_options); } +TEST(CommitContextTest, TestWithCatalog) { + auto catalog = std::make_shared(); + CommitContextBuilder builder("table_root_path", "commit_user_1"); + ASSERT_OK_AND_ASSIGN( + auto ctx, + builder.WithCatalog(catalog, Identifier("db1", "t1")).WithTableId("table-uuid").Finish()); + ASSERT_EQ(ctx->GetCatalog(), catalog); + ASSERT_TRUE(ctx->GetIdentifier().has_value()); + ASSERT_EQ(ctx->GetIdentifier().value(), Identifier("db1", "t1")); + ASSERT_EQ(ctx->GetTableId(), std::optional("table-uuid")); + + CommitContextBuilder null_catalog_builder("table_root_path", "commit_user_1"); + ASSERT_NOK_WITH_MSG(null_catalog_builder.WithCatalog(nullptr, Identifier("db1", "t1")).Finish(), + "cannot commit through a null catalog"); + + CommitContextBuilder both_builder("table_root_path", "commit_user_1"); + ASSERT_NOK_WITH_MSG(both_builder.WithCatalog(catalog, Identifier("db1", "t1")) + .UseRESTCatalogCommit(true) + .Finish(), + "either goes to the catalog"); + + CommitContextBuilder branch_builder("table_root_path", "commit_user_1"); + ASSERT_NOK_WITH_MSG( + branch_builder.WithCatalog(catalog, Identifier("db1", "t1$branch_dev")).Finish(), + "cannot be aimed at branch 'dev'"); + CommitContextBuilder branch_option_builder("table_root_path", "commit_user_1"); + ASSERT_NOK_WITH_MSG(branch_option_builder.WithCatalog(catalog, Identifier("db1", "t1")) + .AddOption(Options::BRANCH, "dev") + .Finish(), + "cannot be aimed at branch 'dev'"); + CommitContextBuilder mixed_builder("table_root_path", "commit_user_1"); + ASSERT_NOK_WITH_MSG(mixed_builder.WithCatalog(catalog, Identifier("db1", "t1$branch_dev")) + .AddOption(Options::BRANCH, "main") + .Finish(), + "cannot be aimed at branch 'dev'"); + CommitContextBuilder mixed_option_builder("table_root_path", "commit_user_1"); + ASSERT_NOK_WITH_MSG( + mixed_option_builder.WithCatalog(catalog, Identifier("db1", "t1$branch_main")) + .AddOption(Options::BRANCH, "dev") + .Finish(), + "cannot be aimed at branch 'dev'"); + + CommitContextBuilder main_builder("table_root_path", "commit_user_1"); + ASSERT_OK(main_builder.WithCatalog(catalog, Identifier("db1", "t1$branch_main")) + .AddOption(Options::BRANCH, "main") + .Finish()); + + ASSERT_OK_AND_ASSIGN(auto next_ctx, builder.Finish()); + ASSERT_EQ(next_ctx->GetCatalog(), nullptr); + ASSERT_FALSE(next_ctx->GetIdentifier().has_value()); + ASSERT_EQ(next_ctx->GetTableId(), std::nullopt); +} + TEST(CommitContextTest, TestSetOptionsOverridesAddedOptions) { CommitContextBuilder builder("table_root_path", "commit_user_1"); builder.AddOption("old", "value"); diff --git a/src/paimon/core/operation/expire_snapshots.cpp b/src/paimon/core/operation/expire_snapshots.cpp index 9dcf1b5d8..a7103e65a 100644 --- a/src/paimon/core/operation/expire_snapshots.cpp +++ b/src/paimon/core/operation/expire_snapshots.cpp @@ -19,7 +19,6 @@ #include "paimon/core/operation/expire_snapshots.h" #include -#include #include #include #include @@ -31,9 +30,11 @@ #include "paimon/common/data/binary_row.h" #include "paimon/common/executor/future.h" #include "paimon/common/utils/date_time_utils.h" +#include "paimon/common/utils/linked_hash_map.h" #include "paimon/common/utils/path_util.h" #include "paimon/common/utils/scope_guard.h" #include "paimon/core/io/data_file_path_factory.h" +#include "paimon/core/manifest/file_entry.h" #include "paimon/core/manifest/file_kind.h" #include "paimon/core/manifest/manifest_entry.h" #include "paimon/core/manifest/manifest_file.h" @@ -86,7 +87,7 @@ Result ExpireSnapshots::Expire() { return Status::Invalid("Expire failed: snapshot manager is nullptr"); } PAIMON_ASSIGN_OR_RAISE(std::optional latest_snapshot_id, - snapshot_manager_->LatestSnapshotId()); + snapshot_manager_->LatestSnapshotIdFromFileSystem()); if (latest_snapshot_id == std::nullopt) { // no snapshot, nothing to expire return 0; @@ -111,19 +112,72 @@ Result ExpireSnapshots::Expire() { if (exist) { PAIMON_ASSIGN_OR_RAISE(Snapshot snapshot, snapshot_manager_->LoadSnapshot(snapshot_id)); if (older_than_ms <= snapshot.TimeMillis()) { - return ExpireUntil(earliest_snapshot_id.value(), snapshot_id); + return ExpireUntil(earliest_snapshot_id.value(), snapshot_id, + latest_snapshot_id.value()); } } } - return ExpireUntil(earliest_snapshot_id.value(), max); + return ExpireUntil(earliest_snapshot_id.value(), max, latest_snapshot_id.value()); } -Result ExpireSnapshots::ExpireUntil(int64_t earliest_snapshot_id, - int64_t end_exclusive_id) { +Result ExpireSnapshots::ExpireUntil(int64_t earliest_snapshot_id, int64_t end_exclusive_id, + int64_t latest_snapshot_id) { if (end_exclusive_id <= earliest_snapshot_id) { // TODO(jinli.zjw): write earliest hint return 0; } + // Read the retained boundary before deleting files referenced by expired snapshots. + PAIMON_ASSIGN_OR_RAISE(bool exist, snapshot_manager_->SnapshotExists(end_exclusive_id)); + if (!exist) { + PAIMON_LOG_DEBUG(logger_, "Skip expiration of table %s: retained snapshot #%ld is missing.", + snapshot_manager_->RootPath().c_str(), end_exclusive_id); + return 0; + } + + // An unpublished catalog rollback can reference files deleted by older snapshots. + // Require the current snapshot and its retained history to be visible before deleting files. + PAIMON_ASSIGN_OR_RAISE(SnapshotManager::LatestSnapshotResult latest, + snapshot_manager_->LatestSnapshotWithSource()); + if (snapshot_manager_->HasSnapshotLoader() && !latest.from_catalog) { + return Status::NotImplemented( + "snapshot expiration through a catalog requires loading its current snapshot"); + } + if (!latest.snapshot || latest.snapshot->Id() != latest_snapshot_id) { + PAIMON_LOG_DEBUG(logger_, + "Skip expiration of table %s: the current snapshot differs from the " + "latest published snapshot #%ld.", + snapshot_manager_->RootPath().c_str(), latest_snapshot_id); + return 0; + } + std::vector retained_snapshots; + for (int64_t id = end_exclusive_id; id <= latest_snapshot_id; ++id) { + PAIMON_ASSIGN_OR_RAISE(Snapshot snapshot, snapshot_manager_->LoadSnapshot(id)); + retained_snapshots.push_back(std::move(snapshot)); + } + if (latest.from_catalog && !(retained_snapshots.back() == latest.snapshot.value())) { + PAIMON_LOG_DEBUG(logger_, + "Skip expiration of table %s: snapshot #%ld differs between the catalog " + "and file system.", + snapshot_manager_->RootPath().c_str(), latest_snapshot_id); + return 0; + } + std::set retained_offset_files; + if (realtime_enabled_) { + for (const Snapshot& retained_snapshot : retained_snapshots) { + std::optional offsets_path = + RealtimeCommitProperties::GetOffsetsPath(retained_snapshot); + if (offsets_path) { + retained_offset_files.insert(offsets_path.value()); + } + } + } + std::set skipping_sets; + PAIMON_RETURN_NOT_OK(GetManifestSkippingSet(retained_snapshots, &skipping_sets)); + DataFilePathFactoryCache data_file_path_factory_cache; + std::set skipping_data_files; + PAIMON_RETURN_NOT_OK(GetDataFileSkippingSet(retained_snapshots, &data_file_path_factory_cache, + &skipping_data_files)); + int64_t begin_inclusive_id = earliest_snapshot_id; for (int64_t id = end_exclusive_id - 1; id >= earliest_snapshot_id; id--) { PAIMON_ASSIGN_OR_RAISE(bool exist, snapshot_manager_->SnapshotExists(id)); @@ -138,7 +192,6 @@ Result ExpireSnapshots::ExpireUntil(int64_t earliest_snapshot_id, // Since the data file deletion information for each snapshot is recorded in the delta part of // the next snapshot, it is necessary to check the next snapshot. Otherwise, its data files will // not be deleted in this round. - DataFilePathFactoryCache data_file_path_factory_cache; for (int64_t id = begin_inclusive_id + 1; id <= end_exclusive_id; id++) { PAIMON_ASSIGN_OR_RAISE(bool exist, snapshot_manager_->SnapshotExists(id)); if (!exist) { @@ -146,8 +199,8 @@ Result ExpireSnapshots::ExpireUntil(int64_t earliest_snapshot_id, continue; } PAIMON_ASSIGN_OR_RAISE(Snapshot snapshot, snapshot_manager_->LoadSnapshot(id)); - PAIMON_RETURN_NOT_OK( - CleanUnusedDataFiles(snapshot.DeltaManifestList(), &data_file_path_factory_cache)); + PAIMON_RETURN_NOT_OK(CleanUnusedDataFiles(snapshot.DeltaManifestList(), skipping_data_files, + &data_file_path_factory_cache)); } // TODO(jinli.zjw): support delete changelog files @@ -155,30 +208,6 @@ Result ExpireSnapshots::ExpireUntil(int64_t earliest_snapshot_id, // then delete changed bucket directories if they are empty PAIMON_RETURN_NOT_OK(CleanEmptyDirectories()); - PAIMON_ASSIGN_OR_RAISE(bool exist, snapshot_manager_->SnapshotExists(end_exclusive_id)); - if (!exist) { - return 0; - } - std::vector retained_snapshots; - PAIMON_ASSIGN_OR_RAISE(Snapshot snapshot, snapshot_manager_->LoadSnapshot(end_exclusive_id)); - retained_snapshots.push_back(snapshot); - std::set retained_offset_files; - if (realtime_enabled_) { - PAIMON_ASSIGN_OR_RAISE(std::vector all_snapshots, - snapshot_manager_->GetAllSnapshots()); - for (const Snapshot& retained_snapshot : all_snapshots) { - if (retained_snapshot.Id() < end_exclusive_id) { - continue; - } - std::optional offsets_path = - RealtimeCommitProperties::GetOffsetsPath(retained_snapshot); - if (offsets_path) { - retained_offset_files.insert(offsets_path.value()); - } - } - } - std::set skipping_sets; - PAIMON_RETURN_NOT_OK(GetManifestSkippingSet(retained_snapshots, &skipping_sets)); std::set expired_offset_files; for (int64_t id = begin_inclusive_id; id < end_exclusive_id; id++) { PAIMON_LOG_DEBUG(logger_, "Ready to delete manifests in snapshot #%ld", id); @@ -294,7 +323,8 @@ Status ExpireSnapshots::CleanUnusedManifests(const std::string& manifest_list_na } Status ExpireSnapshots::CleanUnusedDataFiles( - const std::string& manifest_list_name, DataFilePathFactoryCache* data_file_path_factory_cache) { + const std::string& manifest_list_name, const std::set& skipping_data_files, + DataFilePathFactoryCache* data_file_path_factory_cache) { std::vector manifest_file_metas; auto status = manifest_list_->Read(manifest_list_name, nullptr, &manifest_file_metas); if (status.ok()) { @@ -316,21 +346,13 @@ Status ExpireSnapshots::CleanUnusedDataFiles( std::vector> futures; ScopeGuard guard([&futures]() { Wait(futures); }); for (const auto& [_, entry] : data_files_to_delete) { - std::unordered_map>& bucket_factories = - (*data_file_path_factory_cache)[entry.Partition()]; - auto factory_iter = bucket_factories.find(entry.Bucket()); - if (factory_iter == bucket_factories.end()) { - PAIMON_ASSIGN_OR_RAISE( - std::shared_ptr data_file_path_factory, - path_factory_->CreateDataFilePathFactory(entry.Partition(), entry.Bucket())); - factory_iter = - bucket_factories.emplace(entry.Bucket(), std::move(data_file_path_factory)) - .first; - } - const std::shared_ptr& data_file_path_factory = - factory_iter->second; + PAIMON_ASSIGN_OR_RAISE(std::shared_ptr data_file_path_factory, + GetDataFilePathFactory(entry, data_file_path_factory_cache)); for (const std::string& delete_file_path : data_file_path_factory->CollectFiles(entry.File())) { + if (skipping_data_files.count(delete_file_path) != 0) { + continue; + } futures.push_back(Via(executor_.get(), [this, delete_file_path]() { auto status = fs_->Delete(delete_file_path); // delete quietly will ignore any status error @@ -343,6 +365,65 @@ Status ExpireSnapshots::CleanUnusedDataFiles( return Status::OK(); } +Result> ExpireSnapshots::GetDataFilePathFactory( + const ManifestEntry& entry, DataFilePathFactoryCache* data_file_path_factory_cache) const { + std::unordered_map>& bucket_factories = + (*data_file_path_factory_cache)[entry.Partition()]; + auto factory_iter = bucket_factories.find(entry.Bucket()); + if (factory_iter == bucket_factories.end()) { + PAIMON_ASSIGN_OR_RAISE( + std::shared_ptr factory, + path_factory_->CreateDataFilePathFactory(entry.Partition(), entry.Bucket())); + factory_iter = bucket_factories.emplace(entry.Bucket(), std::move(factory)).first; + } + return factory_iter->second; +} + +Status ExpireSnapshots::GetDataFileSkippingSet( + const std::vector& retained_snapshots, + DataFilePathFactoryCache* data_file_path_factory_cache, + std::set* skipping_data_files) const { + auto retain_files = [&](const ManifestEntry& entry) -> Status { + if (entry.Kind() == FileKind::Add()) { + PAIMON_ASSIGN_OR_RAISE(std::shared_ptr factory, + GetDataFilePathFactory(entry, data_file_path_factory_cache)); + for (const std::string& path : factory->CollectFiles(entry.File())) { + skipping_data_files->insert(path); + } + } + return Status::OK(); + }; + + // Merge the boundary snapshot once, then protect additions from each later snapshot. + // A rollback can add an old file again, including its extra files and external location. + for (size_t i = 0; i < retained_snapshots.size(); ++i) { + std::vector manifests; + if (i == 0) { + PAIMON_RETURN_NOT_OK( + manifest_list_->ReadDataManifests(retained_snapshots[i], &manifests)); + } else { + PAIMON_RETURN_NOT_OK( + manifest_list_->ReadDeltaManifests(retained_snapshots[i], &manifests)); + } + LinkedHashMap boundary_entries; + for (const ManifestFileMeta& manifest : manifests) { + std::vector entries; + PAIMON_RETURN_NOT_OK(manifest_file_->Read(manifest.FileName(), nullptr, &entries)); + if (i == 0) { + PAIMON_RETURN_NOT_OK(FileEntry::MergeEntries(entries, &boundary_entries)); + } else { + for (const ManifestEntry& entry : entries) { + PAIMON_RETURN_NOT_OK(retain_files(entry)); + } + } + } + for (const auto& [_, entry] : boundary_entries) { + PAIMON_RETURN_NOT_OK(retain_files(entry)); + } + } + return Status::OK(); +} + Status ExpireSnapshots::GetDataFilesToDelete( const std::vector& data_file_entries, std::map* data_files_to_delete) const { @@ -374,7 +455,6 @@ Status ExpireSnapshots::GetManifestSkippingSet(const std::vector& reta } // TODO(jinli.zjw): skip index manifests if (snapshot.IndexManifest() && snapshot.IndexManifest().value() != "") { - assert(false); return Status::NotImplemented("do not support expire snapshot with index manifest"); } if (snapshot.Statistics()) { diff --git a/src/paimon/core/operation/expire_snapshots.h b/src/paimon/core/operation/expire_snapshots.h index 64af99a87..526b20323 100644 --- a/src/paimon/core/operation/expire_snapshots.h +++ b/src/paimon/core/operation/expire_snapshots.h @@ -61,9 +61,11 @@ class ExpireSnapshots { std::unordered_map>>; - Result ExpireUntil(int64_t earliest_snapshot_id, int64_t end_exclusive_id); + Result ExpireUntil(int64_t earliest_snapshot_id, int64_t end_exclusive_id, + int64_t latest_snapshot_id); Status CleanUnusedDataFiles(const std::string& manifest_list_name, + const std::set& skipping_data_files, DataFilePathFactoryCache* data_file_path_factory_cache); Status CleanUnusedManifests(const std::string& manifest_list_name, const std::set& skipping_sets); @@ -72,6 +74,11 @@ class ExpireSnapshots { std::map* data_files_to_delete) const; Status GetManifestSkippingSet(const std::vector& retained_snapshots, std::set* skipping_manifest_set) const; + Status GetDataFileSkippingSet(const std::vector& retained_snapshots, + DataFilePathFactoryCache* data_file_path_factory_cache, + std::set* skipping_data_files) const; + Result> GetDataFilePathFactory( + const ManifestEntry& entry, DataFilePathFactoryCache* data_file_path_factory_cache) const; bool TryDeleteEmptyDirectory(const std::string& path) const; std::shared_ptr snapshot_manager_; diff --git a/src/paimon/core/operation/expire_snapshots_test.cpp b/src/paimon/core/operation/expire_snapshots_test.cpp index f256476df..778ee469f 100644 --- a/src/paimon/core/operation/expire_snapshots_test.cpp +++ b/src/paimon/core/operation/expire_snapshots_test.cpp @@ -264,7 +264,7 @@ TEST_F(ExpireSnapshotsTest, TestGetDataFileToDelete) { } } -TEST_F(ExpireSnapshotsTest, TestCleanUnusedDataFileDeletesExtraFiles) { +TEST_F(ExpireSnapshotsTest, TestCleanUnusedDataFilePreservesRetainedExtraFiles) { auto mgr = std::make_shared(fs_, test_data_path_); ASSERT_OK_AND_ASSIGN(CoreOptions options, CoreOptions::FromMap({})); ExpireSnapshots expire(mgr, path_factory_, manifest_list_, manifest_file_, fs_, @@ -288,7 +288,13 @@ TEST_F(ExpireSnapshotsTest, TestCleanUnusedDataFileDeletesExtraFiles) { std::pair manifest_list; ASSERT_OK_AND_ASSIGN(manifest_list, manifest_list_->Write(manifest_files)); ExpireSnapshots::DataFilePathFactoryCache data_file_path_factory_cache; - ASSERT_OK(expire.CleanUnusedDataFiles(manifest_list.first, &data_file_path_factory_cache)); + ASSERT_OK(expire.CleanUnusedDataFiles(manifest_list.first, {files[1]}, + &data_file_path_factory_cache)); + for (const std::string& file : files) { + ASSERT_OK_AND_ASSIGN(bool exists, fs_->Exists(file)); + ASSERT_EQ(exists, file == files[1]) << file; + } + ASSERT_OK(expire.CleanUnusedDataFiles(manifest_list.first, {}, &data_file_path_factory_cache)); for (const std::string& file : files) { ASSERT_OK_AND_ASSIGN(bool exists, fs_->Exists(file)); diff --git a/src/paimon/core/operation/file_store_commit.cpp b/src/paimon/core/operation/file_store_commit.cpp index c2f949ec5..15ada3fe7 100644 --- a/src/paimon/core/operation/file_store_commit.cpp +++ b/src/paimon/core/operation/file_store_commit.cpp @@ -20,12 +20,19 @@ #include "paimon/file_store_commit.h" #include +#include #include +#include #include +#include "paimon/catalog/catalog.h" #include "paimon/commit_context.h" #include "paimon/common/types/data_field.h" #include "paimon/common/utils/binary_row_partition_computer.h" +#include "paimon/core/catalog/catalog_snapshot_commit.h" +#include "paimon/core/catalog/renaming_snapshot_commit.h" +#include "paimon/core/catalog/snapshot_commit.h" +#include "paimon/core/catalog/version_managed_catalog.h" #include "paimon/core/core_options.h" #include "paimon/core/manifest/index_manifest_entry.h" #include "paimon/core/manifest/index_manifest_file.h" @@ -47,6 +54,7 @@ #include "paimon/format/file_format.h" #include "paimon/fs/file_system.h" #include "paimon/result.h" +#include "paimon/schema/schema.h" #include "paimon/table/format/format_table.h" namespace arrow { @@ -97,9 +105,37 @@ CommitScanner::ScanSupplier CreatePkScanSupplier( }; } -/// Maps a `CommitContext` onto `FormatTableFileStoreCommit`, refusing by name what a format table -/// cannot honour rather than silently dropping it. Each of the three is refused only when set away -/// from its default, so an ordinary commit is unaffected. +bool IsCatalogCommit(const CommitContext& ctx) { + return AsVersionManaged(ctx.GetCatalog()) != nullptr; +} + +Result> NewSnapshotCommit( + const CommitContext& ctx, const std::shared_ptr& fs, + const std::shared_ptr& snapshot_manager) { + if (IsCatalogCommit(ctx)) { + if (!ctx.GetIdentifier()) { + return Status::Invalid("a catalog commit requires a table identifier"); + } + // Reuse the caller's UUID so table recreation cannot redirect this commit. + return std::shared_ptr(std::make_shared( + ctx.GetCatalog(), ctx.GetIdentifier().value(), ctx.GetTableId())); + } + if (ctx.UseRESTCatalogCommit()) { + return std::shared_ptr( + std::make_shared(ctx.GetTableId())); + } + // Catalogs without version management may still supply a table ID. + if (ctx.GetTableId() && ctx.GetCatalog() == nullptr) { + return Status::Invalid( + "a commit which publishes the snapshot by writing it to the table directory sends no " + "commit table request, so there is nowhere to put a table id; commit through a " + "catalog with WithCatalog() or ask for the request with UseRESTCatalogCommit()"); + } + return std::shared_ptr( + std::make_shared(fs, snapshot_manager)); +} + +/// Creates a format-table commit, rejecting non-default options that require snapshots. Result> NewFormatTableCommit( const std::shared_ptr& table, const CommitContext& ctx) { if (!ctx.IgnoreEmptyCommit()) { @@ -112,6 +148,15 @@ Result> NewFormatTableCommit( "a format table commits by renaming files into place, not by sending a snapshot " "to a rest catalog"); } + if (ctx.GetTableId()) { + return Status::NotImplemented( + "a format table sends no commit table request, so there is nowhere to put a table id"); + } + if (ctx.GetCatalog() != nullptr) { + return Status::NotImplemented( + "a format table keeps no snapshot for a catalog to take, so it commits by renaming " + "files into place rather than through the catalog it was loaded from"); + } if (ctx.AppendCommitCheckConflict()) { return Status::NotImplemented( "a format table has no manifests to check a concurrent commit against"); @@ -142,9 +187,40 @@ Result> FileStoreCommit::Create( return NewFormatTableCommit(given_table, *ctx); } + PAIMON_RETURN_NOT_OK(CheckVersionManagementImplemented(ctx->GetCatalog())); + // Use catalog credentials unless the caller supplied a file system. + std::shared_ptr specific_fs = ctx->GetSpecificFileSystem(); + if (specific_fs == nullptr && ctx->GetCatalog() != nullptr) { + specific_fs = ctx->GetCatalog()->GetFileSystem(); + } PAIMON_ASSIGN_OR_RAISE(CoreOptions tmp_options, - CoreOptions::FromMap(ctx->GetOptions(), ctx->GetSpecificFileSystem())); + CoreOptions::FromMap(ctx->GetOptions(), specific_fs)); const std::string& root_path = ctx->GetRootPath(); + // Catalog-managed schemas may be absent from the table directory. + std::optional catalog_table_schema; + FileStoreCommitImpl::SchemaIdLoader schema_id_loader; + if (ctx->GetCatalog() != nullptr) { + if (!ctx->GetIdentifier()) { + return Status::Invalid("a catalog commit requires a table identifier"); + } + PAIMON_ASSIGN_OR_RAISE(std::shared_ptr loaded_schema, + ctx->GetCatalog()->LoadTableSchema(ctx->GetIdentifier().value())); + PAIMON_ASSIGN_OR_RAISE(std::string schema_json, loaded_schema->GetJsonSchema()); + catalog_table_schema = std::move(schema_json); + std::shared_ptr catalog = ctx->GetCatalog(); + Identifier identifier = ctx->GetIdentifier().value(); + schema_id_loader = [catalog, identifier]() -> Result { + PAIMON_ASSIGN_OR_RAISE(std::shared_ptr schema, + catalog->LoadTableSchema(identifier)); + std::shared_ptr data_schema = std::dynamic_pointer_cast(schema); + if (data_schema == nullptr) { + return Status::Invalid( + "the catalog did not hand back a data table schema, which has the id a " + "snapshot records"); + } + return data_schema->Id(); + }; + } // A format table commits by renaming files into place, so it never reaches the snapshot path // below. The managed path here reads the main branch, so this reads the same one: the two // must not dispatch on different schemas. @@ -154,13 +230,10 @@ Result> FileStoreCommit::Create( std::shared_ptr format_table, FormatTableLoader::TryLoad(tmp_options.GetFileSystem(), root_path, BranchManager::DEFAULT_MAIN_BRANCH, ctx->GetOptions(), - /*specific_table_schema=*/std::nullopt, schema_manager.get(), - &latest_schema)); + catalog_table_schema, schema_manager.get(), &latest_schema)); if (format_table != nullptr) { return NewFormatTableCommit(format_table, *ctx); } - // The schema the dispatch above already read through `schema_manager`, rather than a second - // read of the same file. if (latest_schema == nullptr) { return Status::Invalid("not found latest schema"); } @@ -171,13 +244,13 @@ Result> FileStoreCommit::Create( } std::shared_ptr arrow_schema = DataField::ConvertDataFieldsToArrowSchema(schema->Fields()); - PAIMON_ASSIGN_OR_RAISE(CoreOptions options, - CoreOptions::FromMap(opts, ctx->GetSpecificFileSystem())); + PAIMON_ASSIGN_OR_RAISE(CoreOptions options, CoreOptions::FromMap(opts, specific_fs)); assert(options.GetFileSystem()); assert(options.GetFileFormat()); PAIMON_RETURN_NOT_OK(FileStoreCommitImpl::ValidateCommitOptions(options)); PAIMON_ASSIGN_OR_RAISE(bool is_object_store, FileSystem::IsObjectStore(root_path)); - if (is_object_store && !ctx->UseRESTCatalogCommit() && + // Catalog commits do not require atomic rename on the object store. + if (is_object_store && !ctx->UseRESTCatalogCommit() && !IsCatalogCommit(*ctx) && opts.find("enable-object-store-commit-in-inte-test") == opts.end()) { return Status::NotImplemented( "commit operation does not support object store file system for now"); @@ -200,6 +273,16 @@ Result> FileStoreCommit::Create( options.IndexFileInDataFileDir(), ctx->GetMemoryPool())); auto snapshot_manager = std::make_shared(options.GetFileSystem(), root_path); + if (IsCatalogCommit(*ctx)) { + std::shared_ptr catalog = ctx->GetCatalog(); + VersionManagedCatalog* versioned = AsVersionManaged(catalog); + Identifier identifier = ctx->GetIdentifier().value(); + // Capture the catalog to keep the versioned interface alive. + snapshot_manager->SetSnapshotLoader( + [catalog, versioned, identifier]() -> Result> { + return versioned->LoadSnapshot(identifier); + }); + } PAIMON_ASSIGN_OR_RAISE( std::shared_ptr manifest_list, ManifestList::Create(options.GetFileSystem(), options.GetManifestFormat(), @@ -235,12 +318,15 @@ Result> FileStoreCommit::Create( ctx->GetExecutor(), ctx->GetMemoryPool()); } + PAIMON_ASSIGN_OR_RAISE(std::shared_ptr snapshot_commit, + NewSnapshotCommit(*ctx, options.GetFileSystem(), snapshot_manager)); + return std::make_unique( ctx->GetMemoryPool(), ctx->GetExecutor(), arrow_schema, root_path, ctx->GetCommitUser(), - options, path_factory, std::move(partition_computer), snapshot_manager, - ctx->IgnoreEmptyCommit(), ctx->UseRESTCatalogCommit(), ctx->AppendCommitCheckConflict(), - schema, manifest_file, manifest_list, index_manifest_file, expire_snapshots, schema_manager, - std::move(scan_supplier)); + options, path_factory, std::move(partition_computer), snapshot_manager, snapshot_commit, + ctx->IgnoreEmptyCommit(), ctx->AppendCommitCheckConflict(), schema, + std::move(schema_id_loader), manifest_file, manifest_list, index_manifest_file, + expire_snapshots, schema_manager, std::move(scan_supplier)); } } // namespace paimon diff --git a/src/paimon/core/operation/file_store_commit_impl.cpp b/src/paimon/core/operation/file_store_commit_impl.cpp index adac3e152..3fa60e984 100644 --- a/src/paimon/core/operation/file_store_commit_impl.cpp +++ b/src/paimon/core/operation/file_store_commit_impl.cpp @@ -44,8 +44,6 @@ #include "paimon/common/utils/fields_comparator.h" #include "paimon/common/utils/path_util.h" #include "paimon/common/utils/scope_guard.h" -#include "paimon/core/catalog/catalog_snapshot_commit.h" -#include "paimon/core/catalog/renaming_snapshot_commit.h" #include "paimon/core/catalog/snapshot_commit.h" #include "paimon/core/deletionvectors/deletion_vectors_index_file.h" #include "paimon/core/index/index_file_meta.h" @@ -129,9 +127,10 @@ FileStoreCommitImpl::FileStoreCommitImpl( const std::string& commit_user, const CoreOptions& options, const std::shared_ptr& path_factory, std::unique_ptr partition_computer, - const std::shared_ptr& snapshot_manager, bool ignore_empty_commit, - bool use_rest_catalog_commit, bool append_commit_check_conflict, - const std::shared_ptr& table_schema, + const std::shared_ptr& snapshot_manager, + const std::shared_ptr& snapshot_commit, bool ignore_empty_commit, + bool append_commit_check_conflict, const std::shared_ptr& table_schema, + FileStoreCommitImpl::SchemaIdLoader schema_id_loader, const std::shared_ptr& manifest_file, const std::shared_ptr& manifest_list, const std::shared_ptr& index_manifest_file, @@ -148,12 +147,14 @@ FileStoreCommitImpl::FileStoreCommitImpl( fs_(options.GetFileSystem()), partition_computer_(std::move(partition_computer)), snapshot_manager_(snapshot_manager), + snapshot_commit_(snapshot_commit), ignore_empty_commit_(ignore_empty_commit), append_commit_check_conflict_(append_commit_check_conflict), retry_waiter_(options.GetCommitMinRetryWait(), options.GetCommitMaxRetryWait()), num_bucket_(options.GetBucket()), bucket_mode_(ResolveBucketMode(options.GetBucket(), table_schema)), table_schema_(table_schema), + schema_id_loader_(std::move(schema_id_loader)), commit_scanner_(std::make_shared( snapshot_manager, schema_manager, manifest_list, manifest_file, index_manifest_file, table_schema, schema, options, executor, pool, partition_computer_.get(), @@ -166,13 +167,7 @@ FileStoreCommitImpl::FileStoreCommitImpl( expire_snapshots_(expire_snapshots), schema_manager_(schema_manager), metrics_(std::make_shared()), - logger_(Logger::GetLogger("FileStoreCommitImpl")) { - if (use_rest_catalog_commit) { - snapshot_commit_ = std::make_shared(); - } else { - snapshot_commit_ = std::make_shared(fs_, snapshot_manager_); - } -} + logger_(Logger::GetLogger("FileStoreCommitImpl")) {} FileStoreCommitImpl::~FileStoreCommitImpl() = default; @@ -330,6 +325,7 @@ Result FileStoreCommitImpl::RollbackToAsLatest(int64_t target_snapshot_id) std::optional next_row_id = std::max(latest.NextRowId(), target_snapshot.NextRowId()); int64_t delta_record_count = ManifestEntry::RecordCountAdd(delta_files) - ManifestEntry::RecordCountDelete(delta_files); + PAIMON_ASSIGN_OR_RAISE(std::string new_snapshot_uuid, Snapshot::GenerateUuid()); Snapshot new_snapshot( latest.Id() + 1, target_snapshot.SchemaId(), base_manifest_list.first, base_manifest_list.second, delta_manifest_list.first, delta_manifest_list.second, @@ -339,7 +335,7 @@ Result FileStoreCommitImpl::RollbackToAsLatest(int64_t target_snapshot_id) Snapshot::CommitKind::Overwrite(), DateTimeUtils::GetCurrentUTCTimeUs() / 1000, target_snapshot.TotalRecordCount(), delta_record_count, /*changelog_record_count=*/std::nullopt, target_snapshot.Watermark(), - target_snapshot.Statistics(), target_snapshot.Properties(), next_row_id); + target_snapshot.Statistics(), target_snapshot.Properties(), next_row_id, new_snapshot_uuid); std::unordered_map partition_entry_map; PAIMON_RETURN_NOT_OK(PartitionEntry::Merge(delta_files, &partition_entry_map)); @@ -349,13 +345,16 @@ Result FileStoreCommitImpl::RollbackToAsLatest(int64_t target_snapshot_id) delta_statistics.push_back(partition_entry); } - PAIMON_ASSIGN_OR_RAISE(bool success, CommitSnapshotImpl(new_snapshot, delta_statistics)); + PAIMON_ASSIGN_OR_RAISE(bool success, + CommitSnapshotImpl(latest_opt, new_snapshot, delta_statistics)); if (success) { PAIMON_LOG_INFO(logger_, - "Successfully rolled back table %s to snapshot %ld as new snapshot %ld by " - "user %s.", - root_path_.c_str(), target_snapshot_id, new_snapshot.Id(), - commit_user_.c_str()); + "%s table %s%s to snapshot %ld as new snapshot %ld " + "by user %s.", + snapshot_commit_->IsRequestOnly() ? "Prepared rollback request for" + : "Successfully rolled back", + root_path_.c_str(), CommitTargetSuffix().c_str(), target_snapshot_id, + new_snapshot.Id(), commit_user_.c_str()); last_committed_snapshot_id_ = new_snapshot.Id(); } return success; @@ -524,30 +523,18 @@ Result>> FileStoreCommitImpl::F } Result> FileStoreCommitImpl::LatestSnapshotOfCommitUserAtOrBefore( - const std::optional& latest_snapshot) const { - if (!latest_snapshot) { - return std::optional(); - } - PAIMON_ASSIGN_OR_RAISE(std::optional earliest_snapshot_id, - snapshot_manager_->EarliestSnapshotId()); - if (!earliest_snapshot_id || earliest_snapshot_id.value() > latest_snapshot->Id()) { - return std::optional(); - } - for (int64_t snapshot_id = latest_snapshot->Id(); snapshot_id >= earliest_snapshot_id.value(); - --snapshot_id) { - PAIMON_ASSIGN_OR_RAISE(Snapshot snapshot, snapshot_manager_->LoadSnapshot(snapshot_id)); - if (snapshot.CommitUser() == commit_user_) { - return std::optional(std::move(snapshot)); - } - } - return std::optional(); + const std::optional& latest_snapshot, bool latest_from_catalog) const { + return snapshot_manager_->LatestSnapshotOfUserAtOrBefore(commit_user_, latest_snapshot, + latest_from_catalog); } Result> FileStoreCommitImpl::ResolveRealtimeCommit( - const std::optional& latest_snapshot, int64_t identifier, + const SnapshotManager::LatestSnapshotResult& latest, int64_t identifier, const std::map& realtime_ranges) const { - PAIMON_ASSIGN_OR_RAISE(std::optional latest_snapshot_of_user, - LatestSnapshotOfCommitUserAtOrBefore(latest_snapshot)); + const std::optional& latest_snapshot = latest.snapshot; + PAIMON_ASSIGN_OR_RAISE( + std::optional latest_snapshot_of_user, + LatestSnapshotOfCommitUserAtOrBefore(latest_snapshot, latest.from_catalog)); const bool identifier_committed = latest_snapshot_of_user && identifier <= latest_snapshot_of_user->CommitIdentifier(); const std::optional& offsets_snapshot = @@ -991,10 +978,10 @@ Result FileStoreCommitImpl::CommitWithProgress( std::shared_ptr committable = CreateManifestCommittable(identifier, commit_messages, watermark, /*properties=*/{}); - PAIMON_ASSIGN_OR_RAISE(std::optional latest_snapshot, - snapshot_manager_->LatestSnapshot()); + PAIMON_ASSIGN_OR_RAISE(SnapshotManager::LatestSnapshotResult latest, + snapshot_manager_->LatestSnapshotWithSource()); PAIMON_ASSIGN_OR_RAISE(std::optional committed_snapshot_id, - ResolveRealtimeCommit(latest_snapshot, identifier, realtime_ranges)); + ResolveRealtimeCommit(latest, identifier, realtime_ranges)); if (committed_snapshot_id) { return committed_snapshot_id.value(); } @@ -1035,12 +1022,12 @@ Result FileStoreCommitImpl::TryCommit( int32_t retry_count = 0; int64_t start_millis = DateTimeUtils::GetCurrentUTCTimeUs() / 1000; while (true) { - PAIMON_ASSIGN_OR_RAISE(std::optional latest_snapshot, - snapshot_manager_->LatestSnapshot()); + PAIMON_ASSIGN_OR_RAISE(SnapshotManager::LatestSnapshotResult latest, + snapshot_manager_->LatestSnapshotWithSource()); + const std::optional& latest_snapshot = latest.snapshot; if (!realtime_ranges.empty()) { - PAIMON_ASSIGN_OR_RAISE( - std::optional committed_snapshot_id, - ResolveRealtimeCommit(latest_snapshot, identifier, realtime_ranges)); + PAIMON_ASSIGN_OR_RAISE(std::optional committed_snapshot_id, + ResolveRealtimeCommit(latest, identifier, realtime_ranges)); if (committed_snapshot_id) { last_committed_snapshot_id_ = committed_snapshot_id.value(); return TryCommitResult{retry_count, /*already_committed=*/true}; @@ -1078,11 +1065,17 @@ Result FileStoreCommitImpl::TryCommit( int64_t current_millis = DateTimeUtils::GetCurrentUTCTimeUs() / 1000; if (current_millis - start_millis > options_.GetCommitTimeout() || retry_count >= options_.GetCommitMaxRetries()) { - return Status::Invalid( - fmt::format("Commit failed after {} millis with {} retries, there maybe exist " - "commit conflicts between multiple jobs.", - options_.GetCommitTimeout(), retry_count)); + return Status::Invalid(fmt::format( + "Commit to table {}{} failed after {} millis (commit.timeout is {} millis) with " + "{} retries, there maybe exist commit conflicts between multiple jobs.", + root_path_, CommitTargetSuffix(), current_millis - start_millis, + options_.GetCommitTimeout(), retry_count)); } + PAIMON_LOG_INFO(logger_, + "Commit to table %s%s lost a race with a concurrent commit, rebasing and " + "retrying (retry %d of %d).", + root_path_.c_str(), CommitTargetSuffix().c_str(), retry_count + 1, + options_.GetCommitMaxRetries()); retry_waiter_.RetryWait(retry_count); retry_count++; } @@ -1228,6 +1221,9 @@ Result FileStoreCommitImpl::TryCommitOnce( for (const auto& entry : delta_files) { delta_data_files.push_back(entry.File()); } + if (schema_id_loader_) { + PAIMON_RETURN_NOT_OK(CheckRowIdSchemasArePublished(delta_data_files)); + } PAIMON_ASSIGN_OR_RAISE( std::shared_ptr checker, RowIdColumnConflictChecker::FromDataFiles(schema_manager_, delta_data_files)); @@ -1244,18 +1240,19 @@ Result FileStoreCommitImpl::TryCommitOnce( std::pair base_manifest_list; std::pair delta_manifest_list; std::vector delta_statistics; - std::string new_snapshot_path; std::optional old_index_manifest; std::optional index_manifest_name; ScopeGuard guard([&]() { int64_t commit_time = ((DateTimeUtils::GetCurrentUTCTimeUs() / 1000) - start_millis) / 1000; PAIMON_LOG_WARN(logger_, - "Atomic commit failed for snapshot #%ld (path %s) by user %s with " - "identifier %ld and kind %s after %ld seconds. Clean up and try again.", - new_snapshot_id, new_snapshot_path.c_str(), commit_user_.c_str(), - identifier, Snapshot::CommitKind::ToString(commit_kind).c_str(), - commit_time); + "Atomic commit failed for snapshot #%ld of table %s%s by user %s with " + "identifier %ld and kind %s, based on snapshot uuid %s, after %ld " + "seconds. Clean up and try again.", + new_snapshot_id, root_path_.c_str(), CommitTargetSuffix().c_str(), + commit_user_.c_str(), identifier, + Snapshot::CommitKind::ToString(commit_kind).c_str(), + FormatBaseSnapshotUuid(latest_snapshot).c_str(), commit_time); CleanUpTmpManifests(base_manifest_list.first, delta_manifest_list.first, merge_before_manifests, merge_after_manifests, old_index_manifest, @@ -1347,11 +1344,16 @@ Result FileStoreCommitImpl::TryCommitOnce( latest_snapshot ? latest_snapshot.value().Statistics() : std::nullopt; std::optional changelog_record_count = ManifestEntry::NullableRecordCount(changelog_entries); - int64_t schema_id = 0; - PAIMON_ASSIGN_OR_RAISE(std::optional> table_schema, - schema_manager_->Latest()); - if (table_schema) { - schema_id = table_schema.value()->Id(); + // Reload on each attempt to account for concurrent schema changes. + int64_t schema_id = table_schema_->Id(); + if (schema_id_loader_) { + PAIMON_ASSIGN_OR_RAISE(schema_id, schema_id_loader_()); + } else { + PAIMON_ASSIGN_OR_RAISE(std::optional> latest_schema, + schema_manager_->Latest()); + if (latest_schema) { + schema_id = latest_schema.value()->Id(); + } } // Keep Java semantics: inherit previous stats only when schema matches. @@ -1372,7 +1374,7 @@ Result FileStoreCommitImpl::TryCommitOnce( snapshot_properties, latest_max_sequence_number, delta_files); } - // prepare snapshot file + PAIMON_ASSIGN_OR_RAISE(std::string new_snapshot_uuid, Snapshot::GenerateUuid()); Snapshot new_snapshot( new_snapshot_id, schema_id, base_manifest_list.first, base_manifest_list.second, delta_manifest_list.first, delta_manifest_list.second, @@ -1386,32 +1388,31 @@ Result FileStoreCommitImpl::TryCommitOnce( snapshot_properties.empty() ? std::nullopt : std::optional>(snapshot_properties), - next_row_id_start); + next_row_id_start, new_snapshot_uuid); - Result commit_result = CommitSnapshotImpl(new_snapshot, delta_statistics); + Result commit_result = + CommitSnapshotImpl(latest_snapshot, new_snapshot, delta_statistics); if (!commit_result.ok()) { - // commit exception, not sure about the situation and should not clean up the files. + // The snapshot may have been published before the response was lost. Keep its metadata + // and let FilterAndCommit resolve the outcome before retrying. PAIMON_LOG_WARN(logger_, "You need to call FilterAndCommit to retry commit for exception. %s", commit_result.status().ToString().c_str()); - // To prevent the case where an atomic write times out but actually succeeds, - // retrying the commit could lead to the snapshot file being committed multiple times. - // Therefore, retries should be handled by the upper layer, - // which should call FilterAndCommit to avoid duplicate commits. - // Therefore, we should not trigger cleanup here, - // as it may delete meta files from a snapshot that was just written by ourselves, - // leading to an incomplete or corrupted snapshot. guard.Release(); return Status::Invalid("You need to call FilterAndCommit to retry commit for exception. ", commit_result.status().ToString()); } bool commit_success = commit_result.value(); if (commit_success) { + const std::string new_snapshot_uuid_str = new_snapshot.Uuid().value_or("none"); PAIMON_LOG_INFO(logger_, - "Successfully commit snapshot %ld to table %s by user %s with identifier " - "%ld and kind %s.", - new_snapshot.Id(), root_path_.c_str(), commit_user_.c_str(), + "%s snapshot %ld (uuid %s) to table %s%s by user %s with " + "identifier %ld and kind %s.", + snapshot_commit_->IsRequestOnly() ? "Prepared commit request for" + : "Successfully committed", + new_snapshot.Id(), new_snapshot_uuid_str.c_str(), root_path_.c_str(), + CommitTargetSuffix().c_str(), commit_user_.c_str(), new_snapshot.CommitIdentifier(), Snapshot::CommitKind::ToString(new_snapshot.GetCommitKind()).c_str()); last_committed_snapshot_id_ = new_snapshot.Id(); @@ -1423,8 +1424,42 @@ Result FileStoreCommitImpl::TryCommitOnce( } } +Status FileStoreCommitImpl::CheckRowIdSchemasArePublished( + const std::vector>& delta_files) const { + std::set needed_schema_ids; + for (const std::shared_ptr& file : delta_files) { + if (file != nullptr && file->first_row_id.has_value()) { + needed_schema_ids.insert(file->schema_id); + } + } + for (int64_t schema_id : needed_schema_ids) { + PAIMON_ASSIGN_OR_RAISE(bool published, schema_manager_->SchemaExists(schema_id)); + if (!published) { + return Status::NotImplemented(fmt::format( + "row id conflict checking requires data file schemas in the table directory, " + "but schema {} is not there; the catalog supplies only the current schema. " + "Publish the required schema before checking conflicts", + schema_id)); + } + } + return Status::OK(); +} + +std::string FileStoreCommitImpl::CommitTargetSuffix() const { + const std::string target = snapshot_commit_->DescribeTarget(); + return target.empty() ? std::string() : " (" + target + ")"; +} + +std::string FileStoreCommitImpl::FormatBaseSnapshotUuid(const std::optional& snapshot) { + if (!snapshot || !snapshot.value().Uuid()) { + return "none"; + } + return snapshot.value().Uuid().value(); +} + Result FileStoreCommitImpl::CommitSnapshotImpl( - const Snapshot& new_snapshot, const std::vector& delta_statistics) { + const std::optional& base_snapshot, const Snapshot& new_snapshot, + const std::vector& delta_statistics) { std::vector statistics; statistics.reserve(delta_statistics.size()); for (const auto& entry : delta_statistics) { @@ -1432,16 +1467,20 @@ Result FileStoreCommitImpl::CommitSnapshotImpl( entry.ToPartitionStatistics(partition_computer_.get())); statistics.emplace_back(std::move(partition_statistics)); } - Result commit_result = snapshot_commit_->Commit(new_snapshot, statistics); + std::optional base_snapshot_uuid = + base_snapshot ? base_snapshot.value().Uuid() : std::nullopt; + Result commit_result = + snapshot_commit_->Commit(base_snapshot_uuid, new_snapshot, statistics); if (!commit_result.ok()) { - // exception when performing the atomic rename, - // we cannot clean up because we can't determine the success + // An error leaves publication uncertain; preserve metadata and report the attempted UUID. return Status::Invalid(fmt::format( - "Exception occurs when committing snapshot #{} by user {} with identifier {} and kind " - "{}. Cannot clean up because we can't determine the success. {}", - new_snapshot.Id(), commit_user_, new_snapshot.CommitIdentifier(), + "Exception occurs when committing snapshot #{} (uuid {}) of table {}{} by user {} " + "with identifier {} and kind {}, based on snapshot uuid {}. Cannot clean up because " + "we can't determine the success. {}", + new_snapshot.Id(), new_snapshot.Uuid().value_or("none"), root_path_, + CommitTargetSuffix(), commit_user_, new_snapshot.CommitIdentifier(), Snapshot::CommitKind::ToString(new_snapshot.GetCommitKind()), - commit_result.status().ToString())); + base_snapshot_uuid.value_or("none"), commit_result.status().ToString())); } return commit_result; } diff --git a/src/paimon/core/operation/file_store_commit_impl.h b/src/paimon/core/operation/file_store_commit_impl.h index 31006ebbc..94f00bef0 100644 --- a/src/paimon/core/operation/file_store_commit_impl.h +++ b/src/paimon/core/operation/file_store_commit_impl.h @@ -38,6 +38,7 @@ #include "paimon/core/operation/commit/row_id_column_conflict_checker.h" #include "paimon/core/snapshot.h" #include "paimon/core/table/bucket_mode.h" +#include "paimon/core/utils/snapshot_manager.h" #include "paimon/file_store_commit.h" #include "paimon/logging.h" #include "paimon/memory/memory_pool.h" @@ -65,7 +66,6 @@ class IndexManifestFile; struct IndexManifestEntry; class ManifestList; class ManifestFileMeta; -class SnapshotManager; class SchemaManager; class TableSchema; class BinaryRowPartitionComputer; @@ -82,6 +82,9 @@ class SnapshotCommit; /// Commit operation which provides commit and overwrite. class FileStoreCommitImpl : public FileStoreCommit { public: + /// Loads the current catalog schema ID; unset to use `schema_manager_`. + using SchemaIdLoader = std::function()>; + static Status ValidateCommitOptions(const CoreOptions& options); FileStoreCommitImpl(const std::shared_ptr& pool, @@ -91,9 +94,10 @@ class FileStoreCommitImpl : public FileStoreCommit { const std::shared_ptr& path_factory, std::unique_ptr partition_computer, const std::shared_ptr& snapshot_manager, - bool ignore_empty_commit, bool use_rest_catalog_commit, - bool append_commit_check_conflict, + const std::shared_ptr& snapshot_commit, + bool ignore_empty_commit, bool append_commit_check_conflict, const std::shared_ptr& table_schema, + SchemaIdLoader schema_id_loader, const std::shared_ptr& manifest_file, const std::shared_ptr& manifest_list, const std::shared_ptr& index_manifest_file, @@ -180,13 +184,14 @@ class FileStoreCommitImpl : public FileStoreCommit { Result>> FilterCommitted( const std::vector>& committables); + /// Pass the source flag returned with `latest_snapshot` to apply the correct history boundary. Result> LatestSnapshotOfCommitUserAtOrBefore( - const std::optional& latest_snapshot) const; + const std::optional& latest_snapshot, bool latest_from_catalog) const; /// Returns the containing snapshot id when the commit is already complete, or nullopt when /// neither its identifier nor offset ranges have been committed. Result> ResolveRealtimeCommit( - const std::optional& latest_snapshot, int64_t identifier, + const SnapshotManager::LatestSnapshotResult& latest, int64_t identifier, const std::map& realtime_ranges) const; std::shared_ptr CreateManifestCommittable( @@ -226,7 +231,16 @@ class FileStoreCommitImpl : public FileStoreCommit { const std::optional& latest_snapshot, bool detect_conflicts); - Result CommitSnapshotImpl(const Snapshot& new_snapshot, + /// Requires schemas for files carrying row IDs to be available in the table directory. + Status CheckRowIdSchemasArePublished( + const std::vector>& delta_files) const; + + std::string CommitTargetSuffix() const; + + static std::string FormatBaseSnapshotUuid(const std::optional& snapshot); + + Result CommitSnapshotImpl(const std::optional& base_snapshot, + const Snapshot& new_snapshot, const std::vector& delta_statistics); Result> ReadAddManifestEntries(const Snapshot& snapshot) const; @@ -273,6 +287,7 @@ class FileStoreCommitImpl : public FileStoreCommit { int32_t num_bucket_ = 0; BucketMode bucket_mode_ = BucketMode::BUCKET_UNAWARE; std::shared_ptr table_schema_; + SchemaIdLoader schema_id_loader_; std::shared_ptr commit_scanner_; ConflictDetection conflict_detection_; diff --git a/src/paimon/core/operation/file_store_commit_impl_test.cpp b/src/paimon/core/operation/file_store_commit_impl_test.cpp index 7c756c3d9..dfada9443 100644 --- a/src/paimon/core/operation/file_store_commit_impl_test.cpp +++ b/src/paimon/core/operation/file_store_commit_impl_test.cpp @@ -24,12 +24,14 @@ #include #include #include +#include #include #include "arrow/c/abi.h" #include "arrow/c/bridge.h" #include "arrow/status.h" #include "arrow/type.h" +#include "fmt/format.h" #include "gmock/gmock.h" #include "gtest/gtest.h" #include "paimon/catalog/catalog.h" @@ -68,6 +70,7 @@ #include "paimon/core/schema/table_schema.h" #include "paimon/core/stats/simple_stats.h" #include "paimon/core/table/sink/commit_message_impl.h" +#include "paimon/core/table/system/system_table_schema.h" #include "paimon/core/utils/file_store_path_factory.h" #include "paimon/core/utils/file_utils.h" #include "paimon/core/utils/snapshot_manager.h" @@ -80,8 +83,10 @@ #include "paimon/memory/bytes.h" #include "paimon/memory/memory_pool.h" #include "paimon/metrics.h" +#include "paimon/testing/mock/mock_catalog.h" #include "paimon/testing/utils/binary_row_generator.h" #include "paimon/testing/utils/io_exception_helper.h" +#include "paimon/testing/utils/snapshot_test_helper.h" #include "paimon/testing/utils/testharness.h" #include "paimon/testing/utils/timezone_guard.h" @@ -157,6 +162,7 @@ class FileStoreCommitImplTest : public testing::Test { /*primary_keys=*/{}, {}, /*ignore_if_exists=*/false)); table_path_ = PathUtil::JoinPath(test_root_, "foo.db/bar"); + ASSERT_OK_AND_ASSIGN(table_schema_, catalog->LoadTableSchema(Identifier("foo", "bar"))); } void TearDown() override { auto factory_creator = paimon::FactoryCreator::GetInstance(); @@ -236,6 +242,167 @@ class FileStoreCommitImplTest : public testing::Test { return result; } + std::set NewFileNames(const std::vector>& msgs) { + std::set names; + for (const std::shared_ptr& msg : msgs) { + std::shared_ptr impl = + std::dynamic_pointer_cast(msg); + if (impl == nullptr) { + ADD_FAILURE() << "commit message is not a CommitMessageImpl"; + continue; + } + for (const std::shared_ptr& file : + impl->GetNewFilesIncrement().NewFiles()) { + names.insert(file->file_name); + } + } + EXPECT_FALSE(names.empty()) << "these commit messages add no data file"; + return names; + } + + std::shared_ptr CreateCatalog( + Result answer = true, const std::string& table_uuid = "mock-table-uuid") const { + auto catalog = std::make_shared(std::move(answer), table_uuid); + catalog->SetTableSchema(table_schema_); + return catalog; + } + + std::shared_ptr TableSchemaWithId(int64_t schema_id) const { + EXPECT_OK_AND_ASSIGN( + std::unique_ptr evolved, + TableSchema::Create(schema_id, arrow::schema(fields_), {"f1"}, {}, {})); + return std::shared_ptr(std::move(evolved)); + } + + std::shared_ptr CreateCatalogServingSchemaId( + int64_t schema_id) const { + std::shared_ptr catalog = CreateCatalog(); + catalog->SetTableSchema(TableSchemaWithId(schema_id)); + return catalog; + } + + Status PrepareFirstCommitMessageFiles() { + return PrepareFakeFiles( + {"/f1=10/bucket-0/data-51a45441-6037-4af3-b67b-5cefd75dc6f2-0.orc", + "/f1=10/bucket-1/data-6828284c-e707-49b5-af6b-69be79af120c-0.orc", + "/f1=20/bucket-0/data-8dc7f04c-3c98-48b2-9d56-834d746c4a40-0.orc", + "/f1=10/bucket-1/data-fd1d2255-43f2-4534-b4cc-08b29e662940-0.orc", + "/f1=20/bucket-0/data-7b3f4cc7-116b-4d2f-9c62-5dadc1f11bcb-0.orc"}); + } + + void PublishAcceptedSnapshots(const std::shared_ptr& catalog) { + MockVersionManagedCatalog* raw = catalog.get(); + std::shared_ptr fs = file_system_; + std::string table_path = table_path_; + catalog->SetOnCommit([raw, fs, table_path]() { + if (raw->AcceptedSnapshots().empty()) { + return; + } + const Snapshot& taken = raw->AcceptedSnapshots().back(); + SnapshotManager manager(fs, table_path); + ASSERT_OK(fs->Mkdirs(manager.SnapshotDirectory())); + ASSERT_OK_AND_ASSIGN(std::string json, taken.ToJsonString()); + ASSERT_OK(fs->WriteFile(manager.SnapshotPath(taken.Id()), json, true)); + }); + } + + std::vector> CommitMessagesOfRound(int32_t round) const { + std::vector> msgs = GetCommitMessages( + paimon::test::GetDataDir() + "/orc/append_09.db/append_09/commit_messages/" + + "commit_messages-0" + std::to_string(round), + 3); + EXPECT_GT(msgs.size(), 0u) << "no commit messages in fixture commit_messages-0" << round; + return msgs; + } + + void CommitAsCompetitor(int32_t round, int64_t commit_identifier) { + CommitContextBuilder builder(table_path_, "competitor"); + ASSERT_OK_AND_ASSIGN(std::unique_ptr context, + builder.AddOption(Options::FILE_SYSTEM, "local").Finish()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr competitor, + FileStoreCommit::Create(std::move(context))); + ASSERT_OK(competitor->Commit(CommitMessagesOfRound(round), commit_identifier)); + } + + struct CatalogCommitSpec { + std::shared_ptr catalog; + std::string commit_user = "commit_user_1"; + std::string root_path; + Identifier identifier{"db", "tbl"}; + std::optional table_id; + bool append_commit_check_conflict = false; + std::map options; + }; + + Result> CreateCatalogCommit( + const CatalogCommitSpec& spec) const { + CommitContextBuilder builder(spec.root_path.empty() ? table_path_ : spec.root_path, + spec.commit_user); + builder.AddOption(Options::FILE_SYSTEM, "local") + .WithCatalog(spec.catalog, spec.identifier) + .AppendCommitCheckConflict(spec.append_commit_check_conflict); + if (spec.table_id) { + builder.WithTableId(spec.table_id.value()); + } + for (const auto& [key, value] : spec.options) { + builder.AddOption(key, value); + } + PAIMON_ASSIGN_OR_RAISE(std::unique_ptr context, builder.Finish()); + return FileStoreCommit::Create(std::move(context)); + } + + Result> CreateCatalogCommit( + const std::shared_ptr& catalog) const { + CatalogCommitSpec spec; + spec.catalog = catalog; + return CreateCatalogCommit(spec); + } + + void BuildRealtimeProgress(FileStoreCommitImpl* commit_impl, + const std::vector>& msgs, + std::vector* progress) { + for (const std::shared_ptr& message : msgs) { + std::shared_ptr message_impl = + std::dynamic_pointer_cast(message); + ASSERT_NE(nullptr, message_impl); + std::map partition; + ASSERT_OK_AND_ASSIGN(partition, commit_impl->PartitionToMap(message_impl->Partition())); + progress->push_back(RealtimeCommitProgress{ + message, RealtimePartitionBucket(std::move(partition), message_impl->Bucket()), + OffsetRange(0, 1)}); + } + } + + struct TwoWriterRecovery { + std::shared_ptr catalog; + std::unique_ptr first_commit; + std::map>> inputs; + }; + + void SetUpTwoWriterRecovery(bool publish_snapshots, TwoWriterRecovery* recovery) { + recovery->catalog = CreateCatalog(); + recovery->catalog->CheckBaseSnapshotUuid(); + if (publish_snapshots) { + PublishAcceptedSnapshots(recovery->catalog); + } + + ASSERT_OK_AND_ASSIGN(recovery->first_commit, CreateCatalogCommit(recovery->catalog)); + ASSERT_OK(PrepareFirstCommitMessageFiles()); + recovery->inputs[7] = CommitMessagesOfRound(1); + ASSERT_OK_AND_ASSIGN(int32_t committed, + recovery->first_commit->FilterAndCommit(recovery->inputs, 5)); + ASSERT_EQ(1, committed); + + CatalogCommitSpec other_spec; + other_spec.catalog = recovery->catalog; + other_spec.commit_user = "commit_user_2"; + ASSERT_OK_AND_ASSIGN(std::unique_ptr other_commit, + CreateCatalogCommit(other_spec)); + ASSERT_OK(other_commit->Commit(CommitMessagesOfRound(2), 1)); + ASSERT_EQ(recovery->catalog->AcceptedSnapshots().size(), 2u); + ASSERT_EQ(recovery->catalog->AcceptedSnapshots().back().CommitUser(), "commit_user_2"); + } + size_t CountFiles(const std::string& dir) const { size_t count = 0; std::error_code ec; @@ -298,6 +465,18 @@ class FileStoreCommitImplTest : public testing::Test { /*write_cols=*/std::nullopt, /*column_max_sequence_numbers=*/std::nullopt); } + std::shared_ptr CreateRowIdCommitMessage(const std::string& file_name, + std::optional first_row_id, + int64_t schema_id = 0) { + auto file = CreateAppendDataFileMeta(file_name, 10); + file->schema_id = schema_id; + if (first_row_id) { + file->AssignFirstRowId(first_row_id.value()); + } + return std::make_shared( + CreateIntRow(10), 0, 2, DataIncrement({file}, {}, {}), CompactIncrement({}, {}, {})); + } + bool IsStringInSet(const std::set& strSet, const std::string& target) { return strSet.find(target) != strSet.end(); } @@ -373,6 +552,7 @@ class FileStoreCommitImplTest : public testing::Test { std::unique_ptr dir_; std::string test_root_; std::string table_path_; + std::shared_ptr table_schema_; std::shared_ptr file_system_; arrow::FieldVector fields_; }; @@ -411,15 +591,12 @@ TEST_F(FileStoreCommitImplTest, TestRESTCatalogCommit) { context_builder.AddOption(Options::MANIFEST_TARGET_FILE_SIZE, "8mb") .AddOption(Options::FILE_SYSTEM, "local") .UseRESTCatalogCommit(true) + .WithTableId("table-uuid") .Finish()); ASSERT_OK_AND_ASSIGN(auto commit, FileStoreCommit::Create(std::move(commit_context))); - std::vector> msgs = - GetCommitMessages(paimon::test::GetDataDir() + - "/orc/append_09.db/append_09/commit_messages/commit_messages-01", - /*version=*/3); - ASSERT_GT(msgs.size(), 0); + std::vector> msgs = CommitMessagesOfRound(1); ASSERT_NOK_WITH_MSG(commit->GetLastCommitTableRequest(), "Should call Commit first before GetLastCommitTableRequest."); ASSERT_OK(commit->Commit(msgs)); @@ -448,9 +625,11 @@ TEST_F(FileStoreCommitImplTest, TestRESTCatalogCommit) { /*file_size_in_bytes=*/1118, /*file_count=*/2, /*last_file_creation_time=*/1724090888727l - 28800000l, /*total_buckets=*/-1)}; - CommitTableRequest expected_commit_table_request(expected_snapshot, + CommitTableRequest expected_commit_table_request("table-uuid", std::nullopt, expected_snapshot, expected_partition_statistics); ASSERT_TRUE(commit_table_request.TEST_Equal(expected_commit_table_request)); + ASSERT_TRUE(commit_table_request.GetSnapshot().Uuid().has_value()); + ASSERT_EQ(commit_table_request.GetSnapshot().Uuid().value().size(), 36u); std::shared_ptr metrics = commit->GetCommitMetrics(); ASSERT_TRUE(metrics); @@ -462,6 +641,760 @@ TEST_F(FileStoreCommitImplTest, TestRESTCatalogCommit) { ASSERT_FALSE(exist); } +TEST_F(FileStoreCommitImplTest, TestCommitCarriesBaseSnapshotUuid) { + std::vector> msgs = CommitMessagesOfRound(1); + + CommitContextBuilder base_builder(table_path_, "commit_user_1"); + ASSERT_OK_AND_ASSIGN(std::unique_ptr base_context, + base_builder.AddOption(Options::FILE_SYSTEM, "local").Finish()); + ASSERT_OK_AND_ASSIGN(auto base_commit, FileStoreCommit::Create(std::move(base_context))); + ASSERT_OK(base_commit->Commit(msgs, 1)); + + std::string snapshot_path = PathUtil::JoinPath(table_path_, "snapshot/snapshot-1"); + ASSERT_OK_AND_ASSIGN(Snapshot base_snapshot, Snapshot::FromPath(file_system_, snapshot_path)); + ASSERT_TRUE(base_snapshot.Uuid().has_value()); + const std::string base_uuid = base_snapshot.Uuid().value(); + + CommitContextBuilder rest_builder(table_path_, "commit_user_1"); + ASSERT_OK_AND_ASSIGN(std::unique_ptr rest_context, + rest_builder.AddOption(Options::FILE_SYSTEM, "local") + .UseRESTCatalogCommit(true) + .WithTableId("table-uuid") + .Finish()); + ASSERT_OK_AND_ASSIGN(auto rest_commit, FileStoreCommit::Create(std::move(rest_context))); + ASSERT_OK(rest_commit->Commit(msgs, 2)); + + ASSERT_OK_AND_ASSIGN(std::string request_str, rest_commit->GetLastCommitTableRequest()); + ASSERT_OK_AND_ASSIGN(CommitTableRequest request, + CommitTableRequest::FromJsonString(request_str)); + ASSERT_EQ(request.GetTableId(), std::optional("table-uuid")); + ASSERT_EQ(request.GetBaseSnapshotUuid(), std::optional(base_uuid)); + ASSERT_EQ(request.GetSnapshot().Id(), 2); + ASSERT_TRUE(request.GetSnapshot().Uuid().has_value()); + ASSERT_EQ(request.GetSnapshot().Uuid().value().size(), 36u); + ASSERT_NE(request.GetSnapshot().Uuid(), request.GetBaseSnapshotUuid()); + + auto catalog = CreateCatalog(); + ASSERT_OK_AND_ASSIGN(std::unique_ptr catalog_commit, + CreateCatalogCommit(catalog)); + ASSERT_OK(catalog_commit->Commit(msgs, 3)); + + ASSERT_EQ(catalog->CommitCalls().size(), 1u); + ASSERT_EQ(catalog->CommitCalls().front().base_snapshot_uuid, + std::optional(base_uuid)); + ASSERT_EQ(catalog->CommitCalls().front().snapshot.Id(), 2); +} + +TEST_F(FileStoreCommitImplTest, TestCatalogCommitTakesTheSnapshot) { + auto catalog = CreateCatalog(); + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.table_id = "table-uuid-1"; + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(spec)); + + std::vector> msgs = CommitMessagesOfRound(1); + ASSERT_OK(commit->Commit(msgs)); + + ASSERT_EQ(catalog->CommitCalls().size(), 1u); + const MockVersionManagedCatalog::CommitCall& call = catalog->CommitCalls().front(); + ASSERT_EQ(call.identifier, Identifier("db", "tbl")); + ASSERT_EQ(call.table_uuid, std::optional("table-uuid-1")); + ASSERT_EQ(call.base_snapshot_uuid, std::nullopt); + ASSERT_EQ(call.snapshot.Id(), 1); + ASSERT_EQ(catalog->GetTableCalls(), 0u); + ASSERT_GT(catalog->LoadTableSchemaCalls(), 0u); + + ASSERT_OK_AND_ASSIGN(std::string request_str, commit->GetLastCommitTableRequest()); + ASSERT_OK_AND_ASSIGN(CommitTableRequest request, + CommitTableRequest::FromJsonString(request_str)); + ASSERT_EQ(request.GetTableId(), std::optional("table-uuid-1")); + ASSERT_EQ(request.GetBaseSnapshotUuid(), std::nullopt); + ASSERT_EQ(request.GetSnapshot().Id(), 1); + + ASSERT_OK_AND_ASSIGN( + bool exist, file_system_->Exists(PathUtil::JoinPath(table_path_, "snapshot/snapshot-1"))); + ASSERT_FALSE(exist); + ASSERT_OK_AND_ASSIGN(bool hint_exist, + file_system_->Exists(PathUtil::JoinPath(table_path_, "snapshot/LATEST"))); + ASSERT_FALSE(hint_exist); + ASSERT_GT(CountFiles(PathUtil::JoinPath(table_path_, "manifest")), 0u); +} + +TEST_F(FileStoreCommitImplTest, TestCommitIsRefusedForATableRecreatedUnderTheSameName) { + auto catalog = CreateCatalog(true, "table-uuid-of-the-new-table"); + catalog->CheckTableUuid(); + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.table_id = "table-uuid-of-the-dropped-table"; + spec.options = {{Options::COMMIT_MAX_RETRIES, "2"}, {Options::COMMIT_MIN_RETRY_WAIT, "1ms"}}; + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(spec)); + + std::vector> msgs = CommitMessagesOfRound(1); + ASSERT_NOK_WITH_MSG(commit->Commit(msgs), "no table with id table-uuid-of-the-dropped-table"); + ASSERT_EQ(catalog->CommitCalls().size(), 1u); + ASSERT_EQ(catalog->CommitCalls().front().table_uuid, + std::optional("table-uuid-of-the-dropped-table")); + + ASSERT_OK_AND_ASSIGN(std::unique_ptr unchecked_commit, + CreateCatalogCommit(catalog)); + ASSERT_OK(unchecked_commit->Commit(msgs, 2)); + ASSERT_EQ(catalog->CommitCalls().size(), 2u); + ASSERT_EQ(catalog->CommitCalls().back().table_uuid, std::nullopt); + ASSERT_EQ(catalog->GetTableCalls(), 0u); + ASSERT_OK_AND_ASSIGN(std::string request_str, unchecked_commit->GetLastCommitTableRequest()); + ASSERT_OK_AND_ASSIGN(CommitTableRequest request, + CommitTableRequest::FromJsonString(request_str)); + ASSERT_EQ(request.GetTableId(), std::nullopt); +} + +TEST_F(FileStoreCommitImplTest, TestCatalogServesTheSchemaOfTheTableItNames) { + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + std::shared_ptr catalog = CreateCatalogServingSchemaId(7); + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.root_path = dir->Str(); + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(spec)); + ASSERT_GT(catalog->LoadTableSchemaCalls(), 0u); + + std::vector> msgs = CommitMessagesOfRound(1); + ASSERT_OK(commit->Commit(msgs)); + ASSERT_EQ(catalog->CommitCalls().size(), 1u); + ASSERT_EQ(catalog->CommitCalls().front().snapshot.Id(), 1); + ASSERT_EQ(catalog->CommitCalls().front().snapshot.SchemaId(), 7); +} + +TEST_F(FileStoreCommitImplTest, TestCatalogWhichDoesNotKnowTheTableFailsCreate) { + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + auto empty_catalog = std::make_shared(); + CatalogCommitSpec spec; + spec.catalog = empty_catalog; + spec.root_path = dir->Str(); + ASSERT_NOK_WITH_MSG(CreateCatalogCommit(spec), "not exist"); +} + +TEST_F(FileStoreCommitImplTest, TestCatalogSchemaWinsOverTheOneUnderTheTablePath) { + std::shared_ptr catalog = CreateCatalogServingSchemaId(7); + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(catalog)); + + std::vector> msgs = CommitMessagesOfRound(1); + ASSERT_OK(commit->Commit(msgs)); + ASSERT_EQ(catalog->CommitCalls().size(), 1u); + ASSERT_EQ(catalog->CommitCalls().front().snapshot.SchemaId(), 7); + + CommitContextBuilder path_builder(table_path_, "commit_user_2"); + ASSERT_OK_AND_ASSIGN(std::unique_ptr path_context, + path_builder.AddOption(Options::FILE_SYSTEM, "local").Finish()); + ASSERT_OK_AND_ASSIGN(auto path_commit, FileStoreCommit::Create(std::move(path_context))); + std::vector> other_msgs = CommitMessagesOfRound(2); + ASSERT_OK(path_commit->Commit(other_msgs, 2)); + ASSERT_OK_AND_ASSIGN( + Snapshot written, + Snapshot::FromPath(file_system_, PathUtil::JoinPath(table_path_, "snapshot/snapshot-1"))); + ASSERT_EQ(written.SchemaId(), 0); +} + +TEST_F(FileStoreCommitImplTest, TestCatalogSchemaIsRereadOnEveryCommit) { + std::shared_ptr catalog = CreateCatalogServingSchemaId(7); + catalog->CheckBaseSnapshotUuid(); + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(catalog)); + for (int32_t round : {1, 2}) { + SCOPED_TRACE(round); + catalog->SetTableSchema(TableSchemaWithId(7 + round)); + ASSERT_OK(commit->Commit(CommitMessagesOfRound(round), round)); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), static_cast(round)); + ASSERT_EQ(catalog->AcceptedSnapshots().back().Id(), round); + ASSERT_EQ(catalog->AcceptedSnapshots().back().SchemaId(), 7 + round); + } +} + +TEST_F(FileStoreCommitImplTest, TestCatalogSchemaWhichIsNotADataTableSchemaIsRefused) { + std::shared_ptr catalog = CreateCatalogServingSchemaId(7); + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(catalog)); + + catalog->SetTableSchema(std::make_shared(arrow::schema(fields_))); + ASSERT_NOK_WITH_MSG(commit->Commit(CommitMessagesOfRound(1)), + "did not hand back a data table schema"); + ASSERT_TRUE(catalog->CommitCalls().empty()); +} + +TEST_F(FileStoreCommitImplTest, TestTableIdWithNothingToSendItInIsRefused) { + CommitContextBuilder context_builder(table_path_, "commit_user_1"); + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit_context, + context_builder.AddOption(Options::FILE_SYSTEM, "local") + .WithTableId("table-uuid") + .Finish()); + ASSERT_NOK_WITH_MSG(FileStoreCommit::Create(std::move(commit_context)), + "publishes the snapshot by writing it to the table directory"); + + auto catalog = CreateCatalog(); + catalog->SetSupportsVersionManagement(false); + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.table_id = "table-uuid"; + ASSERT_OK(CreateCatalogCommit(spec)); +} + +TEST_F(FileStoreCommitImplTest, TestCatalogWithoutVersionManagementWritesTheSnapshot) { + auto catalog = CreateCatalog(); + catalog->SetSupportsVersionManagement(false); + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(catalog)); + + std::vector> msgs = CommitMessagesOfRound(1); + ASSERT_OK(commit->Commit(msgs)); + + ASSERT_TRUE(catalog->CommitCalls().empty()); + ASSERT_EQ(catalog->LoadSnapshotCalls(), 0u); + ASSERT_OK_AND_ASSIGN( + bool exist, file_system_->Exists(PathUtil::JoinPath(table_path_, "snapshot/snapshot-1"))); + ASSERT_TRUE(exist); +} + +TEST_F(FileStoreCommitImplTest, TestCatalogCommitRetriesWhenRefused) { + for (int32_t max_retries : {0, 2}) { + SCOPED_TRACE(max_retries); + auto catalog = CreateCatalog(false); + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.table_id = "table-uuid-2"; + spec.options = {{Options::COMMIT_MAX_RETRIES, fmt::format("{}", max_retries)}, + {Options::COMMIT_MIN_RETRY_WAIT, "1ms"}}; + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(spec)); + + Status failed = commit->Commit(CommitMessagesOfRound(1)); + ASSERT_NOK_WITH_MSG(failed, "commit conflicts"); + ASSERT_NE(failed.ToString().find(table_path_), std::string::npos) << failed.ToString(); + ASSERT_NE(failed.ToString().find("through catalog"), std::string::npos) + << failed.ToString(); + ASSERT_NE(failed.ToString().find("table='tbl'"), std::string::npos) << failed.ToString(); + ASSERT_NOK_WITH_MSG(failed, fmt::format("{} retries", max_retries)); + + ASSERT_EQ(catalog->CommitCalls().size(), static_cast(max_retries + 1)); + std::set attempted_uuids; + for (const auto& call : catalog->CommitCalls()) { + ASSERT_EQ(call.table_uuid, std::optional("table-uuid-2")); + ASSERT_EQ(call.base_snapshot_uuid, std::nullopt); + ASSERT_EQ(call.snapshot.Id(), 1); + ASSERT_TRUE(call.snapshot.Uuid().has_value()); + ASSERT_TRUE(attempted_uuids.insert(call.snapshot.Uuid().value()).second); + } + std::vector manifests; + ASSERT_OK(file_system_->ListDir(PathUtil::JoinPath(table_path_, "manifest"), &manifests)); + ASSERT_TRUE(manifests.empty()); + ASSERT_OK_AND_ASSIGN(bool published, file_system_->Exists(PathUtil::JoinPath( + table_path_, "snapshot/snapshot-1"))); + ASSERT_FALSE(published); + } +} + +TEST_F(FileStoreCommitImplTest, TestUnknownCatalogOutcomeKeepsTheManifests) { + auto catalog = CreateCatalog(Status::IOError("catalog unreachable")); + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.table_id = "table-uuid"; + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(spec)); + + std::vector> msgs = CommitMessagesOfRound(1); + Status failed = commit->Commit(msgs); + ASSERT_NOK_WITH_MSG(failed, "catalog unreachable"); + ASSERT_NE(failed.ToString().find(table_path_), std::string::npos) << failed.ToString(); + ASSERT_NE(failed.ToString().find("through catalog"), std::string::npos) << failed.ToString(); + ASSERT_NE(failed.ToString().find("table='tbl'"), std::string::npos) << failed.ToString(); + ASSERT_NE(failed.ToString().find("snapshot uuid none"), std::string::npos) << failed.ToString(); + + ASSERT_EQ(catalog->CommitCalls().size(), 1u); + + ASSERT_OK_AND_ASSIGN(std::string request_str, commit->GetLastCommitTableRequest()); + ASSERT_OK_AND_ASSIGN(CommitTableRequest request, + CommitTableRequest::FromJsonString(request_str)); + const Snapshot& attempted = request.GetSnapshot(); + for (const std::string& manifest_list : + {attempted.BaseManifestList(), attempted.DeltaManifestList()}) { + ASSERT_OK_AND_ASSIGN(bool exist, file_system_->Exists(PathUtil::JoinPath( + table_path_, "manifest/" + manifest_list))); + ASSERT_TRUE(exist) << manifest_list; + } + auto commit_impl = dynamic_cast(commit.get()); + ASSERT_TRUE(commit_impl); + ASSERT_OK_AND_ASSIGN(std::vector entries, + commit_impl->ReadAddManifestEntries(attempted)); + ASSERT_GT(entries.size(), 0u); +} + +TEST_F(FileStoreCommitImplTest, TestCatalogCommitRebasesOnTheWinnerOfEachRace) { + std::vector> msgs = CommitMessagesOfRound(1); + + auto catalog = CreateCatalog(false); + int32_t competitor_round = 0; + catalog->SetOnCommit([&]() { + ++competitor_round; + CommitAsCompetitor(competitor_round + 1, competitor_round); + }); + + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.table_id = "table-uuid-3"; + spec.options = {{Options::COMMIT_MAX_RETRIES, "1"}, {Options::COMMIT_MIN_RETRY_WAIT, "1ms"}}; + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(spec)); + ASSERT_NOK_WITH_MSG(commit->Commit(msgs), "commit conflicts"); + + ASSERT_EQ(catalog->CommitCalls().size(), 2u); + const std::vector& calls = catalog->CommitCalls(); + ASSERT_EQ(calls[0].base_snapshot_uuid, std::nullopt); + ASSERT_EQ(calls[0].snapshot.Id(), 1); + for (size_t i = 1; i < calls.size(); ++i) { + ASSERT_EQ(calls[i].snapshot.Id(), calls[i - 1].snapshot.Id() + 1); + ASSERT_OK_AND_ASSIGN( + Snapshot base, + Snapshot::FromPath( + file_system_, + PathUtil::JoinPath(table_path_, "snapshot/snapshot-" + + std::to_string(calls[i].snapshot.Id() - 1)))); + ASSERT_EQ(calls[i].base_snapshot_uuid, base.Uuid()); + ASSERT_NE(calls[i].snapshot.BaseManifestList(), calls[i - 1].snapshot.BaseManifestList()); + } +} + +TEST_F(FileStoreCommitImplTest, TestCatalogCommitSucceedsAfterLosingOneRace) { + std::vector> msgs = CommitMessagesOfRound(1); + std::set own_files = NewFileNames(msgs); + + auto catalog = CreateCatalog(false); + const std::set competitor_files = NewFileNames(CommitMessagesOfRound(2)); + bool competitor_ran = false; + catalog->SetOnCommit([&]() { + if (competitor_ran) { + return; + } + competitor_ran = true; + CommitAsCompetitor(2, 1); + catalog->SetTableSchema(TableSchemaWithId(7)); + catalog->SetAnswer(true); + }); + + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.options = {{Options::COMMIT_MAX_RETRIES, "3"}, {Options::COMMIT_MIN_RETRY_WAIT, "1ms"}}; + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(spec)); + ASSERT_OK(commit->Commit(msgs)); + + ASSERT_EQ(catalog->CommitCalls().size(), 2u); + ASSERT_EQ(catalog->CommitCalls()[0].snapshot.Id(), 1); + ASSERT_EQ(catalog->CommitCalls()[0].base_snapshot_uuid, std::nullopt); + ASSERT_EQ(catalog->CommitCalls()[0].snapshot.SchemaId(), 0); + ASSERT_OK_AND_ASSIGN( + Snapshot competitor_snapshot, + Snapshot::FromPath(file_system_, PathUtil::JoinPath(table_path_, "snapshot/snapshot-1"))); + ASSERT_EQ(catalog->CommitCalls()[1].snapshot.Id(), 2); + ASSERT_EQ(catalog->CommitCalls()[1].base_snapshot_uuid, competitor_snapshot.Uuid()); + ASSERT_EQ(catalog->CommitCalls()[1].snapshot.SchemaId(), 7); + ASSERT_NE(catalog->CommitCalls()[1].snapshot.Uuid(), catalog->CommitCalls()[0].snapshot.Uuid()); + + for (const std::string& manifest_list : + {catalog->CommitCalls()[0].snapshot.BaseManifestList(), + catalog->CommitCalls()[0].snapshot.DeltaManifestList()}) { + ASSERT_OK_AND_ASSIGN(bool exist, file_system_->Exists(PathUtil::JoinPath( + table_path_, "manifest/" + manifest_list))); + ASSERT_FALSE(exist) << manifest_list; + } + + auto commit_impl = dynamic_cast(commit.get()); + ASSERT_TRUE(commit_impl); + ASSERT_OK_AND_ASSIGN(std::vector candidate_entries, + commit_impl->ReadAddManifestEntries(catalog->CommitCalls()[1].snapshot)); + std::set candidate_files = CollectFileNames(candidate_entries); + ASSERT_FALSE(own_files.empty()); + ASSERT_FALSE(competitor_files.empty()); + std::set expected_files = own_files; + expected_files.insert(competitor_files.begin(), competitor_files.end()); + ASSERT_EQ(candidate_files, expected_files); + ASSERT_EQ(candidate_entries.size(), expected_files.size()); + + ASSERT_OK_AND_ASSIGN( + bool exist, file_system_->Exists(PathUtil::JoinPath(table_path_, "snapshot/snapshot-2"))); + ASSERT_FALSE(exist); +} + +TEST_F(FileStoreCommitImplTest, TestCatalogCommitReadsItsBaseFromTheCatalog) { + auto catalog = CreateCatalog(); + catalog->CheckBaseSnapshotUuid(); + + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(catalog)); + + std::set first_round_files; + for (int32_t round = 1; round <= 2; ++round) { + std::vector> msgs = CommitMessagesOfRound(round); + if (round == 1) { + first_round_files = NewFileNames(msgs); + } + ASSERT_OK(commit->Commit(msgs, round)); + } + + ASSERT_EQ(catalog->CommitCalls().size(), 2u); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 2u); + ASSERT_GT(catalog->LoadSnapshotCalls(), 0u); + for (const Identifier& asked : catalog->LoadSnapshotIdentifiers()) { + ASSERT_EQ(asked, Identifier("db", "tbl")); + } + ASSERT_EQ(catalog->CommitCalls()[0].base_snapshot_uuid, std::nullopt); + ASSERT_EQ(catalog->CommitCalls()[0].snapshot.Id(), 1); + ASSERT_EQ(catalog->CommitCalls()[1].base_snapshot_uuid, catalog->AcceptedSnapshots()[0].Uuid()); + ASSERT_EQ(catalog->CommitCalls()[1].snapshot.Id(), 2); + ASSERT_OK_AND_ASSIGN( + bool exist, file_system_->Exists(PathUtil::JoinPath(table_path_, "snapshot/snapshot-1"))); + ASSERT_FALSE(exist); + + auto commit_impl = dynamic_cast(commit.get()); + ASSERT_TRUE(commit_impl); + ASSERT_OK_AND_ASSIGN(std::vector entries, + commit_impl->ReadAddManifestEntries(catalog->CommitCalls()[1].snapshot)); + std::set files = CollectFileNames(entries); + ASSERT_FALSE(first_round_files.empty()); + for (const std::string& first_file : first_round_files) { + ASSERT_GT(files.count(first_file), 0u) << first_file; + } +} + +TEST_F(FileStoreCommitImplTest, TestFilterAndCommitDedupesAgainstTheCatalogsSnapshot) { + auto catalog = CreateCatalog(); + catalog->CheckBaseSnapshotUuid(); + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(catalog)); + + ASSERT_OK(PrepareFirstCommitMessageFiles()); + std::map>> inputs; + inputs[7] = CommitMessagesOfRound(1); + + ASSERT_OK_AND_ASSIGN(int32_t committed, commit->FilterAndCommit(inputs, 5)); + ASSERT_EQ(1, committed); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 1u); + ASSERT_OK_AND_ASSIGN( + bool exist, file_system_->Exists(PathUtil::JoinPath(table_path_, "snapshot/snapshot-1"))); + ASSERT_FALSE(exist); + + ASSERT_OK_AND_ASSIGN(int32_t again, commit->FilterAndCommit(inputs, 10)); + ASSERT_EQ(0, again); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 1u); +} + +TEST_F(FileStoreCommitImplTest, TestRecoveryAfterAnAnswerLostOnTheWayBack) { + auto catalog = CreateCatalog(); + catalog->CheckBaseSnapshotUuid(); + catalog->SetLostAnswer(Status::IOError("service unavailable")); + ASSERT_OK(PrepareFirstCommitMessageFiles()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(catalog)); + + std::map>> inputs; + inputs[7] = CommitMessagesOfRound(1); + ASSERT_NOK_WITH_MSG(commit->FilterAndCommit(inputs, 5), "service unavailable"); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 1u); + + const Snapshot& taken = catalog->AcceptedSnapshots().front(); + auto commit_impl = dynamic_cast(commit.get()); + ASSERT_TRUE(commit_impl); + ASSERT_OK_AND_ASSIGN(std::vector entries, + commit_impl->ReadAddManifestEntries(taken)); + ASSERT_GT(entries.size(), 0u); + + catalog->SetLostAnswer(Status::OK()); + ASSERT_OK_AND_ASSIGN(int32_t again, commit->FilterAndCommit(inputs, 10)); + ASSERT_EQ(0, again); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 1u); +} + +TEST_F(FileStoreCommitImplTest, TestFilterAndCommitDedupesBehindAnotherWriter) { + TwoWriterRecovery recovery; + ASSERT_NO_FATAL_FAILURE(SetUpTwoWriterRecovery(true, &recovery)); + + ASSERT_OK_AND_ASSIGN(int32_t again, + recovery.first_commit->FilterAndCommit(recovery.inputs, 10)); + ASSERT_EQ(0, again); + ASSERT_EQ(recovery.catalog->AcceptedSnapshots().size(), 2u); +} + +TEST_F(FileStoreCommitImplTest, TestFilterAndCommitRefusesWhenTheHistoryIsNotPublished) { + TwoWriterRecovery recovery; + ASSERT_NO_FATAL_FAILURE(SetUpTwoWriterRecovery(false, &recovery)); + + ASSERT_NOK_WITH_MSG(recovery.first_commit->FilterAndCommit(recovery.inputs, 10), + "cannot tell which snapshot of table"); + ASSERT_EQ(recovery.catalog->AcceptedSnapshots().size(), 2u); +} + +TEST_F(FileStoreCommitImplTest, TestFilterAndCommitRefusesWhenOnlyPartOfTheHistoryIsPublished) { + TwoWriterRecovery recovery; + ASSERT_NO_FATAL_FAILURE(SetUpTwoWriterRecovery(true, &recovery)); + + ASSERT_OK(file_system_->Delete(PathUtil::JoinPath(table_path_, "snapshot/snapshot-1"), false)); + ASSERT_OK_AND_ASSIGN(bool second_exist, file_system_->Exists(PathUtil::JoinPath( + table_path_, "snapshot/snapshot-2"))); + ASSERT_TRUE(second_exist); + + ASSERT_NOK_WITH_MSG(recovery.first_commit->FilterAndCommit(recovery.inputs, 10), + "cannot tell which snapshot of table"); + ASSERT_EQ(recovery.catalog->AcceptedSnapshots().size(), 2u); +} + +TEST_F(FileStoreCommitImplTest, TestFilterAndCommitStopsAtTheEarliestRetainedSnapshot) { + auto catalog = CreateCatalog(); + catalog->CheckBaseSnapshotUuid(); + PublishAcceptedSnapshots(catalog); + ASSERT_OK(PrepareFirstCommitMessageFiles()); + + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(catalog)); + std::map>> first_inputs; + first_inputs[7] = CommitMessagesOfRound(1); + ASSERT_OK_AND_ASSIGN(int32_t first_committed, commit->FilterAndCommit(first_inputs, 5)); + ASSERT_EQ(1, first_committed); + ASSERT_OK(commit->Commit(CommitMessagesOfRound(2), 8)); + + CatalogCommitSpec other_spec; + other_spec.catalog = catalog; + other_spec.commit_user = "commit_user_2"; + ASSERT_OK_AND_ASSIGN(std::unique_ptr other_commit, + CreateCatalogCommit(other_spec)); + ASSERT_OK(other_commit->Commit(CommitMessagesOfRound(3), 1)); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 3u); + + SnapshotManager manager(file_system_, table_path_); + ASSERT_OK(file_system_->Delete(manager.SnapshotPath(1), false)); + ASSERT_OK(manager.CommitEarliestHint(2)); + + std::map>> again_inputs; + again_inputs[8] = CommitMessagesOfRound(2); + ASSERT_OK_AND_ASSIGN(int32_t again, commit->FilterAndCommit(again_inputs, 10)); + ASSERT_EQ(0, again); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 3u); +} + +TEST_F(FileStoreCommitImplTest, TestCommitReadsThroughTheCatalogsFileSystem) { + ASSERT_OK_AND_ASSIGN(std::shared_ptr fs, + FileSystemFactory::Get("gmock_fs", table_path_, {})); + auto* mock_fs = dynamic_cast(fs.get()); + ASSERT_NE(nullptr, mock_fs); + int32_t exists_calls = 0; + EXPECT_CALL(*mock_fs, Exists(testing::_)) + .WillRepeatedly(testing::Invoke([&](const std::string& path) { + ++exists_calls; + return mock_fs->LocalFileSystem::Exists(path); + })); + + auto catalog = CreateCatalog(); + catalog->SetFileSystem(fs); + CommitContextBuilder context_builder(table_path_, "commit_user_1"); + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit_context, + context_builder.WithCatalog(catalog, Identifier("db", "tbl")).Finish()); + ASSERT_OK_AND_ASSIGN(auto commit, FileStoreCommit::Create(std::move(commit_context))); + + ASSERT_OK(commit->Commit(CommitMessagesOfRound(1))); + ASSERT_EQ(catalog->CommitCalls().size(), 1u); + ASSERT_GT(CountFiles(PathUtil::JoinPath(table_path_, "manifest")), 0u); + ASSERT_GT(exists_calls, 0); +} + +TEST_F(FileStoreCommitImplTest, TestAGivenFileSystemWinsOverTheCatalogs) { + ASSERT_OK_AND_ASSIGN(std::shared_ptr catalog_fs, + FileSystemFactory::Get("gmock_fs", table_path_, {})); + auto* mock_catalog_fs = dynamic_cast(catalog_fs.get()); + ASSERT_NE(nullptr, mock_catalog_fs); + int32_t catalog_fs_calls = 0; + EXPECT_CALL(*mock_catalog_fs, Exists(testing::_)) + .WillRepeatedly(testing::Invoke([&](const std::string& path) { + ++catalog_fs_calls; + return mock_catalog_fs->LocalFileSystem::Exists(path); + })); + + ASSERT_OK_AND_ASSIGN(std::shared_ptr given_fs, + FileSystemFactory::Get("gmock_fs", table_path_, {})); + auto* mock_given_fs = dynamic_cast(given_fs.get()); + ASSERT_NE(nullptr, mock_given_fs); + int32_t given_fs_calls = 0; + EXPECT_CALL(*mock_given_fs, Exists(testing::_)) + .WillRepeatedly(testing::Invoke([&](const std::string& path) { + ++given_fs_calls; + return mock_given_fs->LocalFileSystem::Exists(path); + })); + + auto catalog = CreateCatalog(); + catalog->SetFileSystem(catalog_fs); + CommitContextBuilder context_builder(table_path_, "commit_user_1"); + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit_context, + context_builder.WithCatalog(catalog, Identifier("db", "tbl")) + .WithFileSystem(given_fs) + .Finish()); + ASSERT_OK_AND_ASSIGN(auto commit, FileStoreCommit::Create(std::move(commit_context))); + + ASSERT_OK(commit->Commit(CommitMessagesOfRound(1))); + ASSERT_EQ(catalog->CommitCalls().size(), 1u); + ASSERT_GT(given_fs_calls, 0); + ASSERT_EQ(0, catalog_fs_calls); +} + +TEST_F(FileStoreCommitImplTest, TestRealtimeCommitIsIdempotentThroughTheCatalog) { + ASSERT_OK(PrepareFirstCommitMessageFiles()); + auto catalog = CreateCatalog(); + catalog->CheckBaseSnapshotUuid(); + + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.commit_user = "realtime_commit_user"; + spec.options = {{Options::REALTIME_ENABLED, "true"}}; + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(spec)); + auto commit_impl = std::dynamic_pointer_cast( + std::shared_ptr(std::move(commit))); + ASSERT_NE(nullptr, commit_impl); + + std::vector progress; + ASSERT_NO_FATAL_FAILURE( + BuildRealtimeProgress(commit_impl.get(), CommitMessagesOfRound(1), &progress)); + + ASSERT_OK_AND_ASSIGN(int64_t snapshot_id, + commit_impl->CommitWithProgress(progress, 1, std::nullopt)); + ASSERT_EQ(1, snapshot_id); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 1u); + + ASSERT_OK_AND_ASSIGN(int64_t again, commit_impl->CommitWithProgress(progress, 1, std::nullopt)); + ASSERT_EQ(1, again); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 1u); +} + +TEST_F(FileStoreCommitImplTest, TestRealtimeCommitReportsAnUnpublishedHistory) { + ASSERT_OK(PrepareFirstCommitMessageFiles()); + auto catalog = CreateCatalog(); + catalog->SetHeldSnapshot(BuildTestSnapshot(7, "snapshot-uuid-7")); + + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.commit_user = "realtime_commit_user"; + spec.options = {{Options::REALTIME_ENABLED, "true"}}; + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(spec)); + auto commit_impl = std::dynamic_pointer_cast( + std::shared_ptr(std::move(commit))); + ASSERT_NE(nullptr, commit_impl); + + std::vector progress; + ASSERT_NO_FATAL_FAILURE( + BuildRealtimeProgress(commit_impl.get(), CommitMessagesOfRound(1), &progress)); + + ASSERT_NOK_WITH_MSG(commit_impl->CommitWithProgress(progress, 1, std::nullopt), + "is not under the table directory"); + ASSERT_TRUE(catalog->CommitCalls().empty()); +} + +TEST_F(FileStoreCommitImplTest, TestRowIdSchemaPreCheckOnlyNeedsWhatTheCheckReads) { + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + std::shared_ptr catalog = CreateCatalogServingSchemaId(7); + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.root_path = dir->Str(); + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(spec)); + auto commit_impl = dynamic_cast(commit.get()); + ASSERT_TRUE(commit_impl); + + std::shared_ptr without_row_id = CreateAppendDataFileMeta("no-row-id", 10); + ASSERT_FALSE(without_row_id->first_row_id.has_value()); + ASSERT_OK(commit_impl->CheckRowIdSchemasArePublished({without_row_id, nullptr})); + + std::shared_ptr with_row_id = CreateAppendDataFileMeta("row-id", 10); + with_row_id->AssignFirstRowId(0); + ASSERT_NOK_WITH_MSG(commit_impl->CheckRowIdSchemasArePublished({with_row_id}), + "schema 1 is not there"); +} + +TEST_F(FileStoreCommitImplTest, TestRowIdSchemaPreCheckPassesOnAPublishedSchema) { + std::shared_ptr catalog = CreateCatalogServingSchemaId(7); + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(catalog)); + auto commit_impl = dynamic_cast(commit.get()); + ASSERT_TRUE(commit_impl); + + std::shared_ptr with_row_id = CreateAppendDataFileMeta("row-id", 10); + with_row_id->AssignFirstRowId(0); + with_row_id->schema_id = 0; + ASSERT_OK(commit_impl->CheckRowIdSchemasArePublished({with_row_id})); + + std::shared_ptr other_schema = + CreateAppendDataFileMeta("row-id-other-schema", 10); + other_schema->AssignFirstRowId(10); + ASSERT_NOK_WITH_MSG(commit_impl->CheckRowIdSchemasArePublished({with_row_id, other_schema}), + "schema 1 is not there"); +} + +TEST_F(FileStoreCommitImplTest, TestCommitRefusesARowIdCheckMissingItsSchema) { + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + std::shared_ptr catalog = CreateCatalogServingSchemaId(7); + catalog->CheckBaseSnapshotUuid(); + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.root_path = dir->Str(); + spec.options = {{Options::DATA_EVOLUTION_ENABLED, "true"}, + {Options::ROW_TRACKING_ENABLED, "true"}}; + ASSERT_OK_AND_ASSIGN(std::unique_ptr first_commit, CreateCatalogCommit(spec)); + ASSERT_OK(first_commit->Commit({CreateRowIdCommitMessage("base-data", std::nullopt)}, 1)); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 1u); + + spec.append_commit_check_conflict = true; + ASSERT_OK_AND_ASSIGN(std::unique_ptr second_commit, CreateCatalogCommit(spec)); + second_commit->RowIdCheckConflict(1); + ASSERT_NOK_WITH_MSG(second_commit->Commit({CreateRowIdCommitMessage("row-id-data", 0, 1)}, 2), + "schema 1 is not there"); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 1u); +} + +TEST_F(FileStoreCommitImplTest, TestRowIdChecksUseCatalogLatestAndPublishedHistory) { + std::shared_ptr catalog = CreateCatalogServingSchemaId(7); + catalog->CheckBaseSnapshotUuid(); + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.options = {{Options::DATA_EVOLUTION_ENABLED, "true"}, + {Options::ROW_TRACKING_ENABLED, "true"}}; + ASSERT_OK_AND_ASSIGN(std::unique_ptr first_commit, CreateCatalogCommit(spec)); + ASSERT_OK(first_commit->Commit({CreateRowIdCommitMessage("base-data", std::nullopt)}, 1)); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 1u); + ASSERT_EQ(catalog->AcceptedSnapshots().front().NextRowId(), std::optional(10)); + SnapshotManager manager(file_system_, table_path_); + ASSERT_OK_AND_ASSIGN(bool published, manager.SnapshotExists(1)); + ASSERT_FALSE(published); + + spec.append_commit_check_conflict = true; + ASSERT_OK_AND_ASSIGN(std::unique_ptr second_commit, CreateCatalogCommit(spec)); + second_commit->RowIdCheckConflict(1); + ASSERT_OK(second_commit->Commit({CreateRowIdCommitMessage("first-update", 0)}, 2)); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 2u); + ASSERT_EQ(catalog->AcceptedSnapshots().back().SchemaId(), 7); + + spec.commit_user = "concurrent-writer"; + ASSERT_OK_AND_ASSIGN(std::unique_ptr concurrent_commit, + CreateCatalogCommit(spec)); + concurrent_commit->RowIdCheckConflict(1); + auto concurrent_message = CreateRowIdCommitMessage("concurrent-update", 0); + ASSERT_NOK_WITH_MSG(concurrent_commit->Commit({concurrent_message}, 3), "snapshot-1"); + ASSERT_OK_AND_ASSIGN(std::string first_snapshot_json, + catalog->AcceptedSnapshots().front().ToJsonString()); + ASSERT_OK(file_system_->AtomicStore(manager.SnapshotPath(1), first_snapshot_json)); + ASSERT_OK_AND_ASSIGN(published, manager.SnapshotExists(2)); + ASSERT_FALSE(published); + + ASSERT_NOK_WITH_MSG(concurrent_commit->Commit({concurrent_message}, 3), + "updating the same file"); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 2u); +} + +TEST_F(FileStoreCommitImplTest, TestCatalogSnapshotLoaderFailurePropagates) { + auto catalog = CreateCatalog(); + catalog->SetLoadSnapshotStatus( + Status::IOError("catalog unreachable while loading the snapshot")); + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.table_id = "table-uuid"; + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(spec)); + + std::vector> msgs = CommitMessagesOfRound(1); + ASSERT_NOK_WITH_MSG(commit->Commit(msgs), "catalog unreachable while loading the snapshot"); + ASSERT_TRUE(catalog->CommitCalls().empty()); +} + TEST_F(FileStoreCommitImplTest, TestSnapshotSequenceMaxPropertyMergedOnCommit) { CommitContextBuilder context_builder(table_path_, "commit_user_1"); ASSERT_OK_AND_ASSIGN(std::unique_ptr commit_context, @@ -938,6 +1871,50 @@ TEST_F(FileStoreCommitImplTest, TestRollbackToAsLatest) { ASSERT_EQ(CollectFileNames(snapshot5_entries), CollectFileNames(snapshot3_entries)); } +TEST_F(FileStoreCommitImplTest, TestRollbackToAsLatestThroughCatalog) { + CommitContextBuilder base_builder(table_path_, "commit_user_1"); + ASSERT_OK_AND_ASSIGN(std::unique_ptr base_context, + base_builder.AddOption(Options::FILE_SYSTEM, "local").Finish()); + ASSERT_OK_AND_ASSIGN(auto base_commit, FileStoreCommit::Create(std::move(base_context))); + for (int32_t round = 1; round <= 2; ++round) { + ASSERT_OK(base_commit->Commit(CommitMessagesOfRound(round), round)); + } + + const std::string latest_path = PathUtil::JoinPath(table_path_, "snapshot/snapshot-2"); + ASSERT_OK_AND_ASSIGN(Snapshot latest, Snapshot::FromPath(file_system_, latest_path)); + ASSERT_TRUE(latest.Uuid().has_value()); + + auto catalog = CreateCatalog(); + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(catalog)); + + ASSERT_OK_AND_ASSIGN(bool rolled_back, commit->RollbackToAsLatest(1)); + ASSERT_TRUE(rolled_back); + + ASSERT_EQ(catalog->CommitCalls().size(), 1u); + ASSERT_EQ(catalog->CommitCalls().front().base_snapshot_uuid, latest.Uuid()); + ASSERT_EQ(catalog->CommitCalls().front().snapshot.Id(), 3); + ASSERT_OK_AND_ASSIGN( + bool exist, file_system_->Exists(PathUtil::JoinPath(table_path_, "snapshot/snapshot-3"))); + ASSERT_FALSE(exist); +} + +TEST_F(FileStoreCommitImplTest, TestRollbackToAsLatestNeedsTheTargetUnderTheTablePath) { + CommitContextBuilder base_builder(table_path_, "commit_user_1"); + ASSERT_OK_AND_ASSIGN(std::unique_ptr base_context, + base_builder.AddOption(Options::FILE_SYSTEM, "local").Finish()); + ASSERT_OK_AND_ASSIGN(auto base_commit, FileStoreCommit::Create(std::move(base_context))); + for (int32_t round = 1; round <= 2; ++round) { + ASSERT_OK(base_commit->Commit(CommitMessagesOfRound(round), round)); + } + + ASSERT_OK(file_system_->Delete(PathUtil::JoinPath(table_path_, "snapshot/snapshot-1"), false)); + + auto catalog = CreateCatalog(); + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, CreateCatalogCommit(catalog)); + ASSERT_NOK(commit->RollbackToAsLatest(1)); + ASSERT_TRUE(catalog->CommitCalls().empty()); +} + TEST_F(FileStoreCommitImplTest, TestRollbackToAsLatestNoLatestSnapshotReturnsError) { CommitContextBuilder context_builder(table_path_, "commit_user_1"); ASSERT_OK_AND_ASSIGN(std::unique_ptr commit_context, @@ -988,6 +1965,8 @@ TEST_F(FileStoreCommitImplTest, TestRollbackToAsLatestDeletionVectorOnlyChange) ASSERT_OK_AND_ASSIGN(std::unique_ptr commit_context, context_builder.AddOption(Options::MANIFEST_TARGET_FILE_SIZE, "8mb") .AddOption(Options::FILE_SYSTEM, "local") + .AddOption(Options::SNAPSHOT_NUM_RETAINED_MIN, "1") + .AddOption(Options::SNAPSHOT_NUM_RETAINED_MAX, "1") .Finish()); ASSERT_OK_AND_ASSIGN(auto commit, FileStoreCommit::Create(std::move(commit_context))); auto commit_impl = dynamic_cast(commit.get()); @@ -1021,6 +2000,13 @@ TEST_F(FileStoreCommitImplTest, TestRollbackToAsLatestDeletionVectorOnlyChange) ASSERT_OK_AND_ASSIGN(Snapshot latest_snapshot, commit_impl->snapshot_manager_->LoadSnapshot(2)); ASSERT_TRUE(latest_snapshot.IndexManifest().has_value()); + // Unsupported expiration must leave the rollback target and its manifests intact. + Result expired = commit->Expire(); + ASSERT_FALSE(expired.ok()); + ASSERT_TRUE(expired.status().IsNotImplemented()) << expired.status().ToString(); + ASSERT_OK_AND_ASSIGN(bool exists, commit_impl->snapshot_manager_->SnapshotExists(1)); + ASSERT_TRUE(exists); + // Roll back to snapshot 1: the data files are identical, so the delta carries no data change // and the new snapshot inherits the target's (empty) index manifest, dropping the deletion // vector. @@ -1473,6 +2459,287 @@ TEST_F(FileStoreCommitImplTest, TestTryOverwriteThenCommit) { ASSERT_EQ(FileKind::Add(), entries2[0].Kind()); } +class FileStoreRollbackExpireTest + : public FileStoreCommitImplTest, + public ::testing::WithParamInterface> {}; + +TEST_P(FileStoreRollbackExpireTest, TestRetainedRollbackKeepsRestoredFiles) { + const auto [use_catalog, commits_after_rollback] = GetParam(); + auto catalog = CreateCatalog(); + if (use_catalog) { + catalog->CheckBaseSnapshotUuid(); + PublishAcceptedSnapshots(catalog); + } else { + catalog->SetSupportsVersionManagement(false); + } + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.options = {{Options::SNAPSHOT_NUM_RETAINED_MIN, "2"}, + {Options::SNAPSHOT_NUM_RETAINED_MAX, "2"}}; + ASSERT_OK_AND_ASSIGN(auto commit, CreateCatalogCommit(spec)); + ASSERT_OK(PrepareFirstCommitMessageFiles()); + ASSERT_OK(commit->Commit(CommitMessagesOfRound(1), 0)); + ASSERT_OK(commit->DropPartition({{{"f1", "10"}}}, 1)); + ASSERT_OK(commit->DropPartition({{{"f1", "20"}}}, 2)); + ASSERT_OK_AND_ASSIGN(bool rolled_back, commit->RollbackToAsLatest(1)); + ASSERT_TRUE(rolled_back); + for (int32_t i = 0; i < commits_after_rollback; ++i) { + ASSERT_OK(commit->Commit(CommitMessagesOfRound(i + 2), i + 3)); + } + + SnapshotManager manager(file_system_, table_path_); + ASSERT_OK(manager.CommitEarliestHint(1)); + ASSERT_OK(manager.CommitLatestHint(4 + commits_after_rollback)); + ASSERT_OK_AND_ASSIGN(int32_t expired_count, commit->Expire()); + ASSERT_EQ(expired_count, 2 + commits_after_rollback); + const std::string restored_data_path = + table_path_ + "/f1=10/bucket-0/data-51a45441-6037-4af3-b67b-5cefd75dc6f2-0.orc"; + ASSERT_OK_AND_ASSIGN(bool exists, file_system_->Exists(restored_data_path)); + ASSERT_TRUE(exists); + + // Once no retained snapshot references the restored file, it can be deleted normally. + ASSERT_OK(commit->DropPartition({{{"f1", "10"}}}, 10)); + ASSERT_OK(commit->DropPartition({{{"f1", "20"}}}, 11)); + ASSERT_OK(manager.CommitLatestHint(6 + commits_after_rollback)); + ASSERT_OK_AND_ASSIGN(expired_count, commit->Expire()); + ASSERT_EQ(expired_count, 2); + ASSERT_OK_AND_ASSIGN(exists, file_system_->Exists(restored_data_path)); + ASSERT_FALSE(exists); +} + +INSTANTIATE_TEST_SUITE_P(RollbackPosition, FileStoreRollbackExpireTest, + ::testing::Combine(::testing::Bool(), ::testing::Values(0, 1, 2)), + [](const ::testing::TestParamInfo>& info) { + return fmt::format("{}_{}", + std::get<0>(info.param) ? "Catalog" : "FileSystem", + std::get<1>(info.param)); + }); + +TEST_F(FileStoreCommitImplTest, TestCatalogExpireWaitsForRollbackPublication) { + auto catalog = CreateCatalog(); + catalog->CheckBaseSnapshotUuid(); + PublishAcceptedSnapshots(catalog); + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.options = {{Options::SNAPSHOT_NUM_RETAINED_MIN, "2"}, + {Options::SNAPSHOT_NUM_RETAINED_MAX, "2"}}; + ASSERT_OK_AND_ASSIGN(auto commit, CreateCatalogCommit(spec)); + ASSERT_OK(PrepareFirstCommitMessageFiles()); + ASSERT_OK(commit->Commit(CommitMessagesOfRound(1), 0)); + ASSERT_OK(commit->DropPartition({{{"f1", "10"}}}, 1)); + ASSERT_OK(commit->DropPartition({{{"f1", "20"}}}, 2)); + + SnapshotManager manager(file_system_, table_path_); + ASSERT_OK(manager.CommitEarliestHint(1)); + ASSERT_OK(manager.CommitLatestHint(3)); + catalog->SetOnCommit({}); + ASSERT_OK_AND_ASSIGN(bool rolled_back, commit->RollbackToAsLatest(1)); + ASSERT_TRUE(rolled_back); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 4u); + ASSERT_OK_AND_ASSIGN(bool exists, manager.SnapshotExists(4)); + ASSERT_FALSE(exists); + const std::string restored_data_path = + table_path_ + "/f1=10/bucket-0/data-51a45441-6037-4af3-b67b-5cefd75dc6f2-0.orc"; + ASSERT_OK_AND_ASSIGN(int32_t expired_count, commit->Expire()); + ASSERT_EQ(expired_count, 0); + ASSERT_OK_AND_ASSIGN(exists, manager.SnapshotExists(1)); + ASSERT_TRUE(exists); + ASSERT_OK_AND_ASSIGN(exists, file_system_->Exists(restored_data_path)); + ASSERT_TRUE(exists); + + for (const Status& status : {Status::IOError("catalog unavailable"), + Status::NotImplemented("snapshot loading unsupported")}) { + SCOPED_TRACE(status.ToString()); + catalog->SetLoadSnapshotStatus(status); + ASSERT_NOK(commit->Expire()); + ASSERT_OK_AND_ASSIGN(exists, file_system_->Exists(restored_data_path)); + ASSERT_TRUE(exists); + ASSERT_OK_AND_ASSIGN(exists, manager.SnapshotExists(1)); + ASSERT_TRUE(exists); + } + catalog->SetLoadSnapshotStatus(Status::OK()); + + // Matching IDs alone do not prove that the catalog snapshot has been published. + ASSERT_OK_AND_ASSIGN(std::string different_snapshot_json, + BuildTestSnapshot(4, "other-uuid").ToJsonString()); + ASSERT_OK(file_system_->AtomicStore(manager.SnapshotPath(4), different_snapshot_json)); + ASSERT_OK(manager.CommitLatestHint(4)); + ASSERT_OK_AND_ASSIGN(expired_count, commit->Expire()); + ASSERT_EQ(expired_count, 0); + ASSERT_OK_AND_ASSIGN(exists, file_system_->Exists(restored_data_path)); + ASSERT_TRUE(exists); + std::string earliest_hint; + ASSERT_OK(file_system_->ReadFile(table_path_ + "/snapshot/EARLIEST", &earliest_hint)); + ASSERT_EQ(earliest_hint, "1"); + + ASSERT_OK_AND_ASSIGN(std::string snapshot_json, + catalog->AcceptedSnapshots().back().ToJsonString()); + ASSERT_OK(file_system_->Delete(manager.SnapshotPath(4), false)); + ASSERT_OK(file_system_->AtomicStore(manager.SnapshotPath(4), snapshot_json)); + ASSERT_OK_AND_ASSIGN(expired_count, commit->Expire()); + ASSERT_EQ(expired_count, 2); + ASSERT_OK_AND_ASSIGN(exists, file_system_->Exists(restored_data_path)); + ASSERT_TRUE(exists); + ASSERT_OK(file_system_->ReadFile(table_path_ + "/snapshot/EARLIEST", &earliest_hint)); + ASSERT_EQ(earliest_hint, "3"); +} + +TEST_F(FileStoreCommitImplTest, TestCatalogExpireUsesPublishedSnapshots) { + auto catalog = CreateCatalog(); + catalog->CheckBaseSnapshotUuid(); + PublishAcceptedSnapshots(catalog); + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.options = {{Options::SNAPSHOT_NUM_RETAINED_MIN, "2"}, + {Options::SNAPSHOT_NUM_RETAINED_MAX, "2"}}; + ASSERT_OK_AND_ASSIGN(auto commit, CreateCatalogCommit(spec)); + ASSERT_OK(PrepareFirstCommitMessageFiles()); + ASSERT_OK(commit->Commit(CommitMessagesOfRound(1), 0)); + ASSERT_OK(commit->DropPartition({{{"f1", "10"}}}, 1)); + catalog->SetOnCommit({}); + ASSERT_OK(commit->DropPartition({{{"f1", "20"}}}, 2)); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 3u); + + SnapshotManager manager(file_system_, table_path_); + ASSERT_OK(manager.CommitEarliestHint(1)); + ASSERT_OK(manager.CommitLatestHint(2)); + const std::string expired_data_path = + table_path_ + "/f1=10/bucket-0/data-51a45441-6037-4af3-b67b-5cefd75dc6f2-0.orc"; + const std::string retained_data_path = + table_path_ + "/f1=20/bucket-0/data-8dc7f04c-3c98-48b2-9d56-834d746c4a40-0.orc"; + catalog->SetLoadSnapshotStatus(Status::IOError("catalog unavailable")); + ASSERT_OK_AND_ASSIGN(int32_t expired_count, commit->Expire()); + ASSERT_EQ(expired_count, 0); + for (int64_t snapshot_id : {1, 2}) { + ASSERT_OK_AND_ASSIGN(bool exists, manager.SnapshotExists(snapshot_id)); + ASSERT_TRUE(exists); + } + ASSERT_OK_AND_ASSIGN(bool exists, manager.SnapshotExists(3)); + ASSERT_FALSE(exists); + ASSERT_OK_AND_ASSIGN(exists, file_system_->Exists(expired_data_path)); + ASSERT_TRUE(exists); + ASSERT_OK_AND_ASSIGN(exists, file_system_->Exists(retained_data_path)); + ASSERT_TRUE(exists); + std::string earliest_hint; + ASSERT_OK(file_system_->ReadFile(table_path_ + "/snapshot/EARLIEST", &earliest_hint)); + ASSERT_EQ(earliest_hint, "1"); + ASSERT_OK_AND_ASSIGN(std::optional latest_id, manager.LatestSnapshotId()); + ASSERT_EQ(latest_id, std::optional(2)); + + ASSERT_OK_AND_ASSIGN(std::string snapshot_json, + catalog->AcceptedSnapshots().back().ToJsonString()); + ASSERT_OK(file_system_->AtomicStore(manager.SnapshotPath(3), snapshot_json)); + ASSERT_OK(manager.CommitLatestHint(3)); + ASSERT_NOK_WITH_MSG(commit->Expire(), "catalog unavailable"); + ASSERT_OK_AND_ASSIGN(exists, manager.SnapshotExists(1)); + ASSERT_TRUE(exists); + ASSERT_OK_AND_ASSIGN(exists, file_system_->Exists(expired_data_path)); + ASSERT_TRUE(exists); + ASSERT_OK(file_system_->ReadFile(table_path_ + "/snapshot/EARLIEST", &earliest_hint)); + ASSERT_EQ(earliest_hint, "1"); + + catalog->SetLoadSnapshotStatus(Status::OK()); + ASSERT_OK_AND_ASSIGN(expired_count, commit->Expire()); + ASSERT_EQ(expired_count, 1); + ASSERT_OK_AND_ASSIGN(exists, manager.SnapshotExists(1)); + ASSERT_FALSE(exists); + for (int64_t snapshot_id : {2, 3}) { + ASSERT_OK_AND_ASSIGN(exists, manager.SnapshotExists(snapshot_id)); + ASSERT_TRUE(exists); + } + ASSERT_OK_AND_ASSIGN(exists, file_system_->Exists(expired_data_path)); + ASSERT_FALSE(exists); + ASSERT_OK_AND_ASSIGN(exists, file_system_->Exists(retained_data_path)); + ASSERT_TRUE(exists); + ASSERT_OK(file_system_->ReadFile(table_path_ + "/snapshot/EARLIEST", &earliest_hint)); + ASSERT_EQ(earliest_hint, "2"); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 3u); + ASSERT_OK_AND_ASSIGN(expired_count, commit->Expire()); + ASSERT_EQ(expired_count, 0); +} + +TEST_F(FileStoreCommitImplTest, TestCatalogExpireValidatesRetainedMetadataBeforeDeletingFiles) { + auto catalog = CreateCatalog(); + catalog->CheckBaseSnapshotUuid(); + PublishAcceptedSnapshots(catalog); + CatalogCommitSpec spec; + spec.catalog = catalog; + spec.options = {{Options::SNAPSHOT_NUM_RETAINED_MIN, "2"}, + {Options::SNAPSHOT_NUM_RETAINED_MAX, "2"}}; + ASSERT_OK_AND_ASSIGN(auto commit, CreateCatalogCommit(spec)); + ASSERT_OK(PrepareFirstCommitMessageFiles()); + ASSERT_OK(commit->Commit(CommitMessagesOfRound(1), 0)); + ASSERT_OK(commit->DropPartition({{{"f1", "10"}}}, 1)); + ASSERT_OK(commit->DropPartition({{{"f1", "20"}}}, 2)); + ASSERT_OK(commit->Commit(CommitMessagesOfRound(2), 3)); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 4u); + + SnapshotManager manager(file_system_, table_path_); + ASSERT_OK(manager.CommitEarliestHint(1)); + ASSERT_OK(manager.CommitLatestHint(4)); + ASSERT_OK_AND_ASSIGN(Snapshot retained_snapshot, manager.LoadSnapshot(3)); + const std::string expired_data_path = + table_path_ + "/f1=10/bucket-0/data-51a45441-6037-4af3-b67b-5cefd75dc6f2-0.orc"; + auto* commit_impl = dynamic_cast(commit.get()); + ASSERT_NE(commit_impl, nullptr); + std::vector retained_manifests; + ASSERT_OK( + commit_impl->manifest_list_->ReadDeltaManifests(retained_snapshot, &retained_manifests)); + ASSERT_FALSE(retained_manifests.empty()); + const std::vector retained_metadata_paths = { + manager.SnapshotPath(3), + PathUtil::JoinPath(table_path_, "manifest/" + retained_snapshot.DeltaManifestList()), + PathUtil::JoinPath(table_path_, "manifest/" + retained_manifests.front().FileName())}; + for (const std::string& path : retained_metadata_paths) { + SCOPED_TRACE(path); + std::string content; + ASSERT_OK(file_system_->ReadFile(path, &content)); + ASSERT_OK(file_system_->Delete(path)); + if (path == manager.SnapshotPath(3)) { + ASSERT_OK_AND_ASSIGN(int32_t expired_count, commit->Expire()); + ASSERT_EQ(expired_count, 0); + } else { + ASSERT_NOK(commit->Expire()); + } + ASSERT_OK_AND_ASSIGN(bool exists, file_system_->Exists(expired_data_path)); + ASSERT_TRUE(exists); + for (int64_t snapshot_id : {1, 2}) { + ASSERT_OK_AND_ASSIGN(exists, manager.SnapshotExists(snapshot_id)); + ASSERT_TRUE(exists); + } + std::string earliest_hint; + ASSERT_OK(file_system_->ReadFile(table_path_ + "/snapshot/EARLIEST", &earliest_hint)); + ASSERT_EQ(earliest_hint, "1"); + ASSERT_OK(file_system_->AtomicStore(path, content)); + } + ASSERT_OK_AND_ASSIGN(int32_t expired_count, commit->Expire()); + ASSERT_EQ(expired_count, 2); + ASSERT_OK_AND_ASSIGN(bool exists, file_system_->Exists(expired_data_path)); + ASSERT_FALSE(exists); + ASSERT_OK_AND_ASSIGN(std::optional earliest_id, manager.EarliestSnapshotId()); + ASSERT_EQ(earliest_id, std::optional(3)); +} + +TEST_F(FileStoreCommitImplTest, TestCatalogExpireWithoutPublishedSnapshots) { + auto catalog = CreateCatalog(); + catalog->CheckBaseSnapshotUuid(); + ASSERT_OK_AND_ASSIGN(auto commit, CreateCatalogCommit(catalog)); + ASSERT_OK(PrepareFirstCommitMessageFiles()); + ASSERT_OK(commit->Commit(CommitMessagesOfRound(1), 0)); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 1u); + catalog->SetLoadSnapshotStatus(Status::IOError("catalog unavailable")); + ASSERT_OK_AND_ASSIGN(int32_t expired_count, commit->Expire()); + ASSERT_EQ(expired_count, 0); + ASSERT_OK_AND_ASSIGN(bool exists, file_system_->Exists(table_path_ + "/snapshot")); + ASSERT_FALSE(exists); + ASSERT_OK_AND_ASSIGN( + exists, + file_system_->Exists(table_path_ + + "/f1=10/bucket-0/data-51a45441-6037-4af3-b67b-5cefd75dc6f2-0.orc")); + ASSERT_TRUE(exists); + ASSERT_EQ(catalog->AcceptedSnapshots().size(), 1u); +} + TEST_F(FileStoreCommitImplTest, TestDropPartitionAndExpireSnapshot) { CommitContextBuilder context_builder(table_path_, "commit_user_1"); ASSERT_OK_AND_ASSIGN(std::unique_ptr commit_context, @@ -2538,14 +3805,34 @@ TEST_F(FileStoreCommitImplTest, TestObjectStoreAllowedWithRESTCatalogCommit) { ASSERT_OK_AND_ASSIGN(auto ctx, builder.UseRESTCatalogCommit(true).Finish()); ASSERT_OK_AND_ASSIGN(auto commit, FileStoreCommit::Create(std::move(ctx))); - auto msgs = - GetCommitMessages(paimon::test::GetDataDir() + - "/orc/append_09.db/append_09/commit_messages/commit_messages-01", - 3); - ASSERT_GT(msgs.size(), 0); + std::vector> msgs = CommitMessagesOfRound(1); ASSERT_OK(commit->Commit(msgs)); ASSERT_OK_AND_ASSIGN(auto json, commit->GetLastCommitTableRequest()); ASSERT_FALSE(json.empty()); + + auto catalog = CreateCatalog(); + CommitContextBuilder catalog_builder(table_path_, "commit_user_1"); + ASSERT_OK_AND_ASSIGN(auto catalog_ctx, + catalog_builder.WithCatalog(catalog, Identifier("db", "tbl")).Finish()); + ASSERT_OK_AND_ASSIGN(auto catalog_commit, FileStoreCommit::Create(std::move(catalog_ctx))); + ASSERT_OK(catalog_commit->Commit(msgs)); + ASSERT_EQ(catalog->CommitCalls().size(), 1u); + + const std::string object_store_path = "oss://bucket/db.db/tbl"; + ASSERT_OK_AND_ASSIGN(bool path_is_object_store, FileSystem::IsObjectStore(object_store_path)); + ASSERT_TRUE(path_is_object_store); + auto object_store_catalog = CreateCatalog(); + object_store_catalog->SetFileSystem(file_system_); + CommitContextBuilder object_store_builder(object_store_path, "commit_user_1"); + ASSERT_OK_AND_ASSIGN( + auto object_store_ctx, + object_store_builder.WithCatalog(object_store_catalog, Identifier("db", "tbl")).Finish()); + ASSERT_OK(FileStoreCommit::Create(std::move(object_store_ctx))); + + CommitContextBuilder plain_builder(object_store_path, "commit_user_1"); + ASSERT_OK_AND_ASSIGN(auto plain_ctx, plain_builder.WithFileSystem(file_system_).Finish()); + ASSERT_NOK_WITH_MSG(FileStoreCommit::Create(std::move(plain_ctx)), + "invalid scheme oss for local file system"); } TEST_F(FileStoreCommitImplTest, TestFixedBucketPKTableCommitAllowed) { diff --git a/src/paimon/core/operation/file_store_commit_test.cpp b/src/paimon/core/operation/file_store_commit_test.cpp index 07d55eb82..10b7c7aaf 100644 --- a/src/paimon/core/operation/file_store_commit_test.cpp +++ b/src/paimon/core/operation/file_store_commit_test.cpp @@ -19,6 +19,7 @@ #include "paimon/file_store_commit.h" +#include #include #include "arrow/c/abi.h" @@ -41,7 +42,9 @@ #include "paimon/core/table/sink/commit_message_impl.h" #include "paimon/core/utils/snapshot_manager.h" #include "paimon/defs.h" +#include "paimon/executor.h" #include "paimon/fs/local/local_file_system.h" +#include "paimon/memory/memory_pool.h" #include "paimon/result.h" #include "paimon/testing/utils/binary_row_generator.h" #include "paimon/testing/utils/testharness.h" @@ -85,6 +88,21 @@ TEST(FileStoreCommitTest, TestCreate) { ASSERT_TRUE(commit_impl); } +TEST(FileStoreCommitTest, TestCreateWithCatalogRequiresIdentifier) { + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + std::map options = {{Options::FILE_SYSTEM, "local"}}; + ASSERT_OK_AND_ASSIGN(std::shared_ptr catalog, Catalog::Create(dir->Str(), options)); + auto commit_context = std::make_unique( + dir->Str(), "commit_user", /*ignore_empty_commit=*/true, + /*use_rest_catalog_commit=*/false, catalog, /*identifier=*/std::nullopt, + /*table_id=*/std::nullopt, /*append_commit_check_conflict=*/false, GetDefaultPool(), + CreateDefaultExecutor(), /*specific_file_system=*/nullptr, options, + /*format_table=*/nullptr); + ASSERT_NOK_WITH_MSG(FileStoreCommit::Create(std::move(commit_context)), + "a catalog commit requires a table identifier"); +} + TEST(FileStoreCommitTest, TestAppendDvIndexShouldUseOverwriteCommitKind) { auto string_field = arrow::field("f0", arrow::utf8()); auto int_field = arrow::field("f1", arrow::int32()); diff --git a/src/paimon/core/operation/file_store_write.cpp b/src/paimon/core/operation/file_store_write.cpp index a6dfe10cd..38686cbc0 100644 --- a/src/paimon/core/operation/file_store_write.cpp +++ b/src/paimon/core/operation/file_store_write.cpp @@ -23,9 +23,11 @@ #include #include "fmt/format.h" +#include "paimon/catalog/catalog.h" #include "paimon/common/data/blob_utils.h" #include "paimon/common/types/data_field.h" #include "paimon/common/utils/fields_comparator.h" +#include "paimon/core/catalog/version_managed_catalog.h" #include "paimon/core/core_options.h" #include "paimon/core/disk/io_manager.h" #include "paimon/core/manifest/index_manifest_file.h" @@ -50,6 +52,7 @@ #include "paimon/core/utils/snapshot_manager.h" #include "paimon/format/file_format.h" #include "paimon/result.h" +#include "paimon/schema/schema.h" #include "paimon/table/format/format_table.h" #include "paimon/write_context.h" @@ -86,6 +89,11 @@ Status RestoreRealtimeCommittedProgress(const std::shared_ptr& /// cannot honour rather than silently dropping it. Result> NewFormatTableWrite( const std::shared_ptr& table, const WriteContext& ctx) { + if (ctx.GetCatalog() != nullptr) { + return Status::Invalid( + "WithCatalog() requires a native table; use WriteContextBuilder(FormatTable) for a " + "format table"); + } if (ctx.IsStreamingMode()) { return Status::NotImplemented( "a format table has no snapshots, so there is nothing a streaming write could " @@ -142,9 +150,24 @@ Result> FileStoreWrite::Create(std::unique_ptrGetCatalog())); + std::shared_ptr specific_fs = ctx->GetSpecificFileSystem(); + if (specific_fs == nullptr && ctx->GetFileSystemSchemeToIdentifierMap().empty() && + ctx->GetCatalog() != nullptr) { + specific_fs = ctx->GetCatalog()->GetFileSystem(); + } PAIMON_ASSIGN_OR_RAISE(CoreOptions tmp_options, - CoreOptions::FromMap(ctx->GetOptions(), ctx->GetSpecificFileSystem(), + CoreOptions::FromMap(ctx->GetOptions(), specific_fs, ctx->GetFileSystemSchemeToIdentifierMap())); + std::optional catalog_table_schema; + if (ctx->GetCatalog() != nullptr) { + if (!ctx->GetIdentifier()) { + return Status::Invalid("a catalog write requires a table identifier"); + } + PAIMON_ASSIGN_OR_RAISE(std::shared_ptr schema, + ctx->GetCatalog()->LoadTableSchema(ctx->GetIdentifier().value())); + PAIMON_ASSIGN_OR_RAISE(catalog_table_schema, schema->GetJsonSchema()); + } std::string branch = ctx->GetBranch(); // A format table writes plain data files into a directory, so it never reaches the manifest // path below. @@ -154,8 +177,8 @@ Result> FileStoreWrite::Create(std::unique_ptr format_table, FormatTableLoader::TryLoad(tmp_options.GetFileSystem(), ctx->GetRootPath(), branch, - ctx->GetOptions(), /*specific_table_schema=*/std::nullopt, - schema_manager.get(), &latest_schema)); + ctx->GetOptions(), catalog_table_schema, schema_manager.get(), + &latest_schema)); if (format_table != nullptr) { return NewFormatTableWrite(format_table, *ctx); } @@ -171,9 +194,9 @@ Result> FileStoreWrite::Create(std::unique_ptrGetOptions()) { opts[key] = value; } - PAIMON_ASSIGN_OR_RAISE(CoreOptions options, - CoreOptions::FromMap(opts, ctx->GetSpecificFileSystem(), - ctx->GetFileSystemSchemeToIdentifierMap())); + PAIMON_ASSIGN_OR_RAISE( + CoreOptions options, + CoreOptions::FromMap(opts, specific_fs, ctx->GetFileSystemSchemeToIdentifierMap())); PAIMON_ASSIGN_OR_RAISE(std::shared_ptr partition_schema, FieldMapping::GetPartitionSchema(arrow_schema, schema->PartitionKeys())); @@ -191,6 +214,15 @@ Result> FileStoreWrite::Create(std::unique_ptrGetMemoryPool())); auto snapshot_manager = std::make_shared(options.GetFileSystem(), ctx->GetRootPath(), branch); + if (VersionManagedCatalog* versioned = AsVersionManaged(ctx->GetCatalog())) { + std::shared_ptr catalog = ctx->GetCatalog(); + Identifier identifier = ctx->GetIdentifier().value(); + // Keep the catalog alive while the writer uses its snapshot loader. + snapshot_manager->SetSnapshotLoader( + [catalog, versioned, identifier]() -> Result> { + return versioned->LoadSnapshot(identifier); + }); + } std::shared_ptr io_manager; const auto& io_temp_dir = ctx->GetTempDirectory(); if (!io_temp_dir.empty()) { @@ -291,7 +323,7 @@ Result> FileStoreWrite::Create(std::unique_ptrIsStreamingMode(), ctx->IgnoreNumBucketCheck(), ctx->GetWriteId(), ctx->GetFileSystemSchemeToIdentifierMap(), ctx->GetExecutor(), ctx->GetMemoryPool(), - ctx->GetSpecificFileSystem()); + specific_fs); } if (options.GetBucket() <= 0) { return Status::Invalid( diff --git a/src/paimon/core/operation/file_store_write_test.cpp b/src/paimon/core/operation/file_store_write_test.cpp index 23633488e..20d247fbd 100644 --- a/src/paimon/core/operation/file_store_write_test.cpp +++ b/src/paimon/core/operation/file_store_write_test.cpp @@ -23,25 +23,127 @@ #include #include +#include "arrow/array/array_base.h" #include "arrow/c/abi.h" #include "arrow/c/bridge.h" +#include "arrow/ipc/json_simple.h" #include "arrow/status.h" #include "arrow/type.h" #include "gtest/gtest.h" #include "paimon/catalog/catalog.h" #include "paimon/catalog/identifier.h" +#include "paimon/commit_context.h" +#include "paimon/common/utils/arrow/status_utils.h" #include "paimon/common/utils/path_util.h" #include "paimon/core/operation/key_value_file_store_write.h" +#include "paimon/core/realtime/realtime_context_impl.h" +#include "paimon/core/realtime/realtime_schema_layout.h" #include "paimon/core/schema/schema_manager.h" #include "paimon/core/schema/table_schema.h" #include "paimon/defs.h" +#include "paimon/file_store_commit.h" #include "paimon/fs/local/local_file_system.h" +#include "paimon/record_batch.h" #include "paimon/result.h" +#include "paimon/testing/mock/mock_catalog.h" #include "paimon/testing/utils/testharness.h" #include "paimon/write_context.h" namespace paimon::test { +TEST(FileStoreWriteTest, TestRealtimeCatalogCommitRefreshAndRecovery) { + for (RealtimeStoreMode mode : + {RealtimeStoreMode::APPEND_ONLY, RealtimeStoreMode::PRIMARY_KEY}) { + SCOPED_TRACE(static_cast(mode)); + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + const std::string table_path = dir->Str(); + const Identifier identifier("db", "table"); + const auto logical_schema = arrow::schema( + {arrow::field("id", arrow::int64(), false), arrow::field("value", arrow::utf8())}); + const std::map options = {{Options::BUCKET, "1"}, + {Options::BUCKET_KEY, "id"}, + {Options::FILE_FORMAT, "parquet"}, + {Options::WRITE_ONLY, "true"}, + {Options::REALTIME_ENABLED, "true"}}; + const std::vector primary_keys = mode == RealtimeStoreMode::PRIMARY_KEY + ? std::vector{"id"} + : std::vector{}; + ASSERT_OK_AND_ASSIGN(std::shared_ptr schema, + TableSchema::Create(0, logical_schema, {}, primary_keys, options)); + auto catalog = std::make_shared(); + catalog->SetTableSchema(schema); + catalog->SetFileSystem(dir->GetFileSystem()); + catalog->CheckBaseSnapshotUuid(); + ASSERT_OK_AND_ASSIGN(auto schema_layout, + RealtimeSchemaLayout::Create(mode, logical_schema)); + auto make_batch = [&](const std::string& json) -> Result> { + PAIMON_ASSIGN_OR_RAISE_FROM_ARROW( + std::shared_ptr array, + arrow::ipc::internal::json::ArrayFromJSON( + arrow::struct_(schema_layout->InputSchema()->fields()), json)); + ArrowArray c_array; + PAIMON_RETURN_NOT_OK_FROM_ARROW(arrow::ExportArray(*array, &c_array)); + return RecordBatchBuilder(&c_array).SetBucket(0).Finish(); + }; + auto create_writer = [&](const std::shared_ptr& realtime_context) + -> Result> { + WriteContextBuilder builder(table_path, "writer"); + PAIMON_ASSIGN_OR_RAISE(std::unique_ptr ctx, + builder.WithCatalog(catalog, identifier) + .WithStreamingMode(true) + .WithRealtimeContext(realtime_context) + .AddOption(Options::FILE_SYSTEM, "unregistered-file-system") + .Finish()); + return FileStoreWrite::Create(std::move(ctx)); + }; + ASSERT_OK_AND_ASSIGN(auto realtime_context, RealtimeContext::Create()); + ASSERT_OK_AND_ASSIGN(auto writer, create_writer(realtime_context)); + ASSERT_OK_AND_ASSIGN(auto batch, make_batch(R"([[10, 1, "a"], [20, 2, "b"]])")); + ASSERT_OK(writer->Write(std::move(batch))); + ASSERT_OK_AND_ASSIGN(auto progress, writer->PrepareCommitWithProgress(1)); + ASSERT_EQ(progress.size(), 1u); + CommitContextBuilder commit_builder(table_path, "writer"); + ASSERT_OK_AND_ASSIGN(auto commit_ctx, + commit_builder.WithCatalog(catalog, identifier).Finish()); + ASSERT_OK_AND_ASSIGN(auto commit, FileStoreCommit::Create(std::move(commit_ctx))); + ASSERT_OK_AND_ASSIGN(int64_t snapshot_id, + commit->CommitWithProgress(progress, 1, std::nullopt)); + ASSERT_EQ(snapshot_id, 1); + ASSERT_OK_AND_ASSIGN(bool has_schema, dir->GetFileSystem()->Exists( + PathUtil::JoinPath(table_path, "schema"))); + ASSERT_FALSE(has_schema); + ASSERT_OK_AND_ASSIGN(bool has_snapshot, dir->GetFileSystem()->Exists(PathUtil::JoinPath( + table_path, "snapshot/snapshot-1"))); + ASSERT_FALSE(has_snapshot); + + ASSERT_OK(writer->RefreshCommittedSnapshot(snapshot_id)); + ASSERT_OK_AND_ASSIGN(double retained_rows, + writer->GetMetrics()->GetGauge(RealtimeMetrics::kTotalRowCount)); + ASSERT_EQ(retained_rows, 0); + ASSERT_OK(writer->Close()); + + ASSERT_OK_AND_ASSIGN(auto recovered_context, RealtimeContext::Create()); + ASSERT_OK_AND_ASSIGN(auto recovered_writer, create_writer(recovered_context)); + ASSERT_OK_AND_ASSIGN(auto recovered_impl, RealtimeContextImpl::Cast(recovered_context)); + ASSERT_OK_AND_ASSIGN(auto read_state, recovered_impl->AcquireReadState()); + const RealtimeOffsetMap expected_offsets = {{RealtimePartitionBucket({}, 0), 21}}; + ASSERT_EQ(read_state.committed_offsets, expected_offsets); + ASSERT_OK_AND_ASSIGN(auto next_batch, make_batch(R"([[21, 1, "updated"]])")); + ASSERT_OK(recovered_writer->Write(std::move(next_batch))); + ASSERT_OK_AND_ASSIGN(auto next_progress, recovered_writer->PrepareCommitWithProgress(2)); + ASSERT_OK_AND_ASSIGN(snapshot_id, + commit->CommitWithProgress(next_progress, 2, std::nullopt)); + ASSERT_EQ(snapshot_id, 2); + ASSERT_OK(recovered_writer->RefreshCommittedSnapshot(snapshot_id)); + ASSERT_OK(recovered_writer->Close()); + + catalog->SetLoadSnapshotStatus(Status::IOError("catalog unavailable")); + ASSERT_OK_AND_ASSIGN(auto failed_context, RealtimeContext::Create()); + ASSERT_NOK_WITH_MSG(create_writer(failed_context), "catalog unavailable"); + } +} + TEST(FileStoreWriteTest, TestCreateWithInvalidInput) { auto dir = UniqueTestDirectory::Create(); WriteContextBuilder builder(dir->Str(), "commit_user_1"); @@ -68,11 +170,20 @@ TEST(FileStoreWriteTest, TestCreateAppendTable) { ASSERT_OK(catalog->CreateTable(Identifier("foo", "bar"), &schema, /*partition_keys=*/{"f0", "f3"}, /*primary_keys=*/{}, /*options=*/{}, /*ignore_if_exists=*/false)); - WriteContextBuilder context_builder(PathUtil::JoinPath(dir->Str(), "foo.db/bar"), - "commit_user_1"); - ASSERT_OK_AND_ASSIGN(std::unique_ptr write_context, context_builder.Finish()); - ASSERT_OK_AND_ASSIGN(std::unique_ptr file_store_write, - FileStoreWrite::Create(std::move(write_context))); + std::shared_ptr shared_catalog(std::move(catalog)); + ASSERT_FALSE(shared_catalog->SupportsVersionManagement()); + for (bool use_catalog : {false, true}) { + SCOPED_TRACE(use_catalog); + WriteContextBuilder context_builder(PathUtil::JoinPath(dir->Str(), "foo.db/bar"), + "commit_user_1"); + if (use_catalog) { + context_builder.WithCatalog(shared_catalog, Identifier("foo", "bar")); + } + ASSERT_OK_AND_ASSIGN(std::unique_ptr write_context, context_builder.Finish()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr file_store_write, + FileStoreWrite::Create(std::move(write_context))); + ASSERT_OK(file_store_write->Close()); + } } TEST(FileStoreWriteTest, TestCreateWriterForLoadedMapBlobTable) { diff --git a/src/paimon/core/operation/key_value_file_store_write.cpp b/src/paimon/core/operation/key_value_file_store_write.cpp index d6b6a7a48..5bb34a66c 100644 --- a/src/paimon/core/operation/key_value_file_store_write.cpp +++ b/src/paimon/core/operation/key_value_file_store_write.cpp @@ -178,7 +178,10 @@ Status KeyValueFileStoreWrite::RefreshCommittedSnapshot(int64_t snapshot_id) { if (!realtime_context_) { return Status::Invalid("refresh committed snapshot requires a real-time writer"); } - PAIMON_ASSIGN_OR_RAISE(Snapshot snapshot, snapshot_manager_->LoadSnapshot(snapshot_id)); + PAIMON_ASSIGN_OR_RAISE(std::optional latest, snapshot_manager_->LatestSnapshot()); + PAIMON_ASSIGN_OR_RAISE(Snapshot snapshot, latest && latest->Id() == snapshot_id + ? Result(latest.value()) + : snapshot_manager_->LoadSnapshot(snapshot_id)); PAIMON_ASSIGN_OR_RAISE( RealtimeOffsetMap committed_offsets, RealtimeCommitProperties::ReadOffsets(std::optional(std::move(snapshot)), diff --git a/src/paimon/core/operation/write_context.cpp b/src/paimon/core/operation/write_context.cpp index 0b9f89429..a62080c01 100644 --- a/src/paimon/core/operation/write_context.cpp +++ b/src/paimon/core/operation/write_context.cpp @@ -21,9 +21,11 @@ #include #include "arrow/util/thread_pool.h" +#include "fmt/format.h" #include "paimon/common/utils/arrow/status_utils.h" #include "paimon/common/utils/path_util.h" #include "paimon/core/utils/branch_manager.h" +#include "paimon/defs.h" #include "paimon/executor.h" #include "paimon/memory/memory_pool.h" #include "paimon/result.h" @@ -32,19 +34,18 @@ namespace paimon { -WriteContext::WriteContext(const std::string& root_path, const std::string& commit_user, - bool is_streaming_mode, bool ignore_num_bucket_check, - bool ignore_previous_files, bool enable_multi_thread_spill, - const std::optional& write_id, const std::string& branch, - const std::vector& write_schema, - const std::shared_ptr& memory_pool, - const std::shared_ptr& executor, - const std::string& temp_directory, - const std::shared_ptr& specific_file_system, - const std::map& fs_scheme_to_identifier_map, - const std::shared_ptr& realtime_context, - const std::map& options, - const std::shared_ptr& format_table) +WriteContext::WriteContext( + const std::string& root_path, const std::string& commit_user, bool is_streaming_mode, + bool ignore_num_bucket_check, bool ignore_previous_files, bool enable_multi_thread_spill, + const std::optional& write_id, const std::string& branch, + const std::vector& write_schema, const std::shared_ptr& memory_pool, + const std::shared_ptr& executor, const std::string& temp_directory, + const std::shared_ptr& specific_file_system, + const std::map& fs_scheme_to_identifier_map, + const std::shared_ptr& realtime_context, + const std::map& options, + const std::shared_ptr& format_table, const std::shared_ptr& catalog, + const std::optional& identifier) : root_path_(root_path), commit_user_(commit_user), branch_(branch), @@ -61,7 +62,9 @@ WriteContext::WriteContext(const std::string& root_path, const std::string& comm fs_scheme_to_identifier_map_(fs_scheme_to_identifier_map), realtime_context_(realtime_context), options_(options), - format_table_(format_table) {} + format_table_(format_table), + catalog_(catalog), + identifier_(identifier) {} WriteContext::~WriteContext() = default; @@ -83,6 +86,8 @@ class WriteContextBuilder::Impl { fs_scheme_to_identifier_map_.clear(); specific_file_system_.reset(); realtime_context_.reset(); + catalog_.reset(); + identifier_.reset(); options_.clear(); } @@ -106,6 +111,8 @@ class WriteContextBuilder::Impl { std::map fs_scheme_to_identifier_map_; std::shared_ptr specific_file_system_; std::shared_ptr realtime_context_; + std::shared_ptr catalog_; + std::optional identifier_; std::map options_; }; @@ -209,11 +216,21 @@ WriteContextBuilder& WriteContextBuilder::WithRealtimeContext( return *this; } +WriteContextBuilder& WriteContextBuilder::WithCatalog(const std::shared_ptr& catalog, + const Identifier& identifier) { + impl_->catalog_ = catalog; + impl_->identifier_.emplace(identifier); + return *this; +} + Result> WriteContextBuilder::Finish() { if (impl_->built_from_format_table_ && impl_->format_table_ == nullptr) { return Status::Invalid("cannot write with null format table"); } if (impl_->format_table_ != nullptr) { + if (impl_->catalog_ != nullptr) { + return Status::Invalid("WithCatalog() requires a native table"); + } // The table already answers each of these, and from a source this cannot see behind, so a // second answer is refused rather than silently dropped. if (impl_->specific_file_system_ != nullptr || @@ -233,6 +250,27 @@ Result> WriteContextBuilder::Finish() { } // The branch names a directory under the root path, so it must stay a single path component. PAIMON_RETURN_NOT_OK(BranchManager::CheckValidBranch(impl_->branch_)); + if (impl_->catalog_ == nullptr && impl_->identifier_) { + return Status::Invalid("cannot write through a null catalog"); + } + if (impl_->catalog_ != nullptr) { + PAIMON_ASSIGN_OR_RAISE(std::optional identifier_branch, + impl_->identifier_->GetBranchName()); + auto branch_option = impl_->options_.find(Options::BRANCH); + std::optional option_branch = + branch_option == impl_->options_.end() + ? std::nullopt + : std::optional(branch_option->second); + for (const std::optional& branch : + {identifier_branch, option_branch, std::optional(impl_->branch_)}) { + if (branch && + !BranchManager::IsMainBranch(BranchManager::NormalizeBranch(branch.value()))) { + return Status::Invalid( + fmt::format("a write through a catalog requires the main branch, but got '{}'", + branch.value())); + } + } + } bool enable_multi_thread_spill = impl_->spill_thread_number_ > 0; if (enable_multi_thread_spill) { PAIMON_RETURN_NOT_OK_FROM_ARROW( @@ -244,7 +282,7 @@ Result> WriteContextBuilder::Finish() { impl_->write_id_, impl_->branch_, impl_->write_schema_, impl_->memory_pool_, impl_->executor_, impl_->temp_directory_, impl_->specific_file_system_, impl_->fs_scheme_to_identifier_map_, impl_->realtime_context_, impl_->options_, - impl_->format_table_); + impl_->format_table_, impl_->catalog_, impl_->identifier_); impl_->Reset(); return ctx; } diff --git a/src/paimon/core/operation/write_context_test.cpp b/src/paimon/core/operation/write_context_test.cpp index b41d147fb..1193e3364 100644 --- a/src/paimon/core/operation/write_context_test.cpp +++ b/src/paimon/core/operation/write_context_test.cpp @@ -18,11 +18,15 @@ #include "paimon/write_context.h" +#include + #include "gtest/gtest.h" +#include "paimon/defs.h" #include "paimon/executor.h" #include "paimon/memory/memory_pool.h" #include "paimon/result.h" #include "paimon/status.h" +#include "paimon/testing/mock/mock_catalog.h" #include "paimon/testing/mock/mock_file_system.h" #include "paimon/testing/utils/testharness.h" @@ -46,6 +50,39 @@ TEST(WriteContextTest, TestDefaultValue) { ASSERT_TRUE(ctx->GetOptions().empty()); ASSERT_TRUE(ctx->GetFileSystemSchemeToIdentifierMap().empty()); ASSERT_FALSE(ctx->GetSpecificFileSystem()); + ASSERT_FALSE(ctx->GetCatalog()); + ASSERT_FALSE(ctx->GetIdentifier()); +} + +TEST(WriteContextTest, TestWithCatalog) { + auto catalog = std::make_shared(); + WriteContextBuilder builder("table_root_path", "commit_user_1"); + ASSERT_OK_AND_ASSIGN(auto ctx, builder.WithCatalog(catalog, Identifier("db1", "t1")).Finish()); + ASSERT_EQ(ctx->GetCatalog(), catalog); + ASSERT_EQ(ctx->GetIdentifier(), std::optional(Identifier("db1", "t1"))); + ASSERT_OK_AND_ASSIGN(auto next_ctx, builder.Finish()); + ASSERT_FALSE(next_ctx->GetCatalog()); + ASSERT_FALSE(next_ctx->GetIdentifier()); + + ASSERT_NOK_WITH_MSG(builder.WithCatalog(nullptr, Identifier("db1", "t1")).Finish(), + "cannot write through a null catalog"); +} + +TEST(WriteContextTest, TestCatalogRequiresMainBranch) { + auto catalog = std::make_shared(); + for (int32_t branch_source = 0; branch_source < 3; ++branch_source) { + SCOPED_TRACE(branch_source); + WriteContextBuilder builder("table_root_path", "commit_user_1"); + builder.WithCatalog(catalog, Identifier("db1", branch_source == 0 ? "t1$branch_dev" : "t1")) + .WithBranch(branch_source == 1 ? "dev" : "main") + .AddOption(Options::BRANCH, branch_source == 2 ? "dev" : "main"); + ASSERT_NOK_WITH_MSG(builder.Finish(), "requires the main branch"); + } + WriteContextBuilder builder("table_root_path", "commit_user_1"); + ASSERT_OK(builder.WithCatalog(catalog, Identifier("db1", "t1$branch_main")) + .WithBranch("") + .AddOption(Options::BRANCH, "main") + .Finish()); } TEST(WriteContextTest, TestSetContent) { diff --git a/src/paimon/core/snapshot.cpp b/src/paimon/core/snapshot.cpp index cd5f933fa..6b27c8fa1 100644 --- a/src/paimon/core/snapshot.cpp +++ b/src/paimon/core/snapshot.cpp @@ -20,10 +20,13 @@ #include #include +#include #include #include "paimon/common/utils/rapidjson_util.h" +#include "paimon/common/utils/uuid.h" #include "paimon/fs/file_system.h" +#include "paimon/macros.h" #include "paimon/result.h" #include "paimon/status.h" #include "rapidjson/allocators.h" @@ -78,6 +81,7 @@ bool Snapshot::TEST_Equal(const Snapshot& other) const { return false; } + // Ignore generated UUIDs when comparing against fixtures; operator== compares them. return version_ == other.version_ && id_ == other.id_ && schema_id_ == other.schema_id_ && index_manifest_ == other.index_manifest_ && commit_user_ == other.commit_user_ && commit_identifier_ == other.commit_identifier_ && commit_kind_ == other.commit_kind_ && @@ -92,8 +96,8 @@ bool Snapshot::operator==(const Snapshot& other) const { if (this == &other) { return true; } - return version_ == other.version_ && id_ == other.id_ && schema_id_ == other.schema_id_ && - base_manifest_list_ == other.base_manifest_list_ && + return version_ == other.version_ && uuid_ == other.uuid_ && id_ == other.id_ && + schema_id_ == other.schema_id_ && base_manifest_list_ == other.base_manifest_list_ && base_manifest_list_size_ == other.base_manifest_list_size_ && delta_manifest_list_ == other.delta_manifest_list_ && delta_manifest_list_size_ == other.delta_manifest_list_size_ && @@ -137,22 +141,29 @@ Snapshot::CommitKind Snapshot::CommitKind::FromString(const std::string& kind) { return Unknown(); } -Snapshot::Snapshot(const std::optional& version, int64_t id, int64_t schema_id, - const std::string& base_manifest_list, - const std::optional& base_manifest_list_size, - const std::string& delta_manifest_list, - const std::optional& delta_manifest_list_size, - const std::optional& changelog_manifest_list, - const std::optional& changelog_manifest_list_size, - const std::optional& index_manifest, const std::string& commit_user, - int64_t commit_identifier, CommitKind commit_kind, int64_t time_millis, - int64_t total_record_count, int64_t delta_record_count, - const std::optional& changelog_record_count, - const std::optional& watermark, - const std::optional& statistics, - const std::optional>& properties, - const std::optional& next_row_id) +Result Snapshot::GenerateUuid() { + std::string uuid; + if (PAIMON_UNLIKELY(!UUID::Generate(&uuid))) { + return Status::Invalid("fail to generate uuid for snapshot"); + } + return uuid; +} + +Snapshot::Snapshot( + const std::optional& version, int64_t id, int64_t schema_id, + const std::string& base_manifest_list, const std::optional& base_manifest_list_size, + const std::string& delta_manifest_list, const std::optional& delta_manifest_list_size, + const std::optional& changelog_manifest_list, + const std::optional& changelog_manifest_list_size, + const std::optional& index_manifest, const std::string& commit_user, + int64_t commit_identifier, CommitKind commit_kind, int64_t time_millis, + int64_t total_record_count, int64_t delta_record_count, + const std::optional& changelog_record_count, const std::optional& watermark, + const std::optional& statistics, + const std::optional>& properties, + const std::optional& next_row_id, const std::optional& uuid) : version_(version), + uuid_(uuid), id_(id), schema_id_(schema_id), base_manifest_list_(base_manifest_list), @@ -179,6 +190,10 @@ rapidjson::Value Snapshot::ToJson(rapidjson::Document::AllocatorType* allocator) rapidjson::Value obj(rapidjson::kObjectType); obj.AddMember(rapidjson::StringRef(FIELD_VERSION), RapidJsonUtil::SerializeValue(Version(), allocator).Move(), *allocator); + if (uuid_ != std::nullopt) { + obj.AddMember(rapidjson::StringRef(FIELD_UUID), + RapidJsonUtil::SerializeValue(uuid_.value(), allocator).Move(), *allocator); + } obj.AddMember(rapidjson::StringRef(FIELD_ID), RapidJsonUtil::SerializeValue(id_, allocator).Move(), *allocator); obj.AddMember(rapidjson::StringRef(FIELD_SCHEMA_ID), @@ -267,6 +282,7 @@ rapidjson::Value Snapshot::ToJson(rapidjson::Document::AllocatorType* allocator) void Snapshot::FromJson(const rapidjson::Value& obj) noexcept(false) { version_ = RapidJsonUtil::DeserializeKeyValue(obj, FIELD_VERSION, -1); + uuid_ = RapidJsonUtil::DeserializeKeyValue>(obj, FIELD_UUID); id_ = RapidJsonUtil::DeserializeKeyValue(obj, FIELD_ID); schema_id_ = RapidJsonUtil::DeserializeKeyValue(obj, FIELD_SCHEMA_ID); base_manifest_list_ = diff --git a/src/paimon/core/snapshot.h b/src/paimon/core/snapshot.h index 2d2cd061e..31adc6b13 100644 --- a/src/paimon/core/snapshot.h +++ b/src/paimon/core/snapshot.h @@ -72,6 +72,7 @@ class Snapshot : public Jsonizable { }; static constexpr char FIELD_VERSION[] = "version"; + static constexpr char FIELD_UUID[] = "uuid"; static constexpr char FIELD_ID[] = "id"; static constexpr char FIELD_SCHEMA_ID[] = "schemaId"; static constexpr char FIELD_BASE_MANIFEST_LIST[] = "baseManifestList"; @@ -95,6 +96,10 @@ class Snapshot : public Jsonizable { JSONIZABLE_FRIEND_AND_DEFAULT_CTOR(Snapshot); + /// Generates a UUID for a new snapshot, returning an error if generation fails. + static Result GenerateUuid(); + + /// @param uuid UUID from `GenerateUuid()` for new snapshots; null for legacy snapshots. Snapshot(int64_t id, int64_t schema_id, const std::string& base_manifest_list, const std::optional& base_manifest_list_size, const std::string& delta_manifest_list, @@ -107,12 +112,13 @@ class Snapshot : public Jsonizable { const std::optional& changelog_record_count, const std::optional& watermark, const std::optional& statistics, const std::optional>& properties, - const std::optional& next_row_id) + const std::optional& next_row_id, + const std::optional& uuid = std::nullopt) : Snapshot(CURRENT_VERSION, id, schema_id, base_manifest_list, base_manifest_list_size, delta_manifest_list, delta_manifest_list_size, changelog_manifest_list, changelog_manifest_list_size, index_manifest, commit_user, commit_identifier, commit_kind, time_millis, total_record_count, delta_record_count, - changelog_record_count, watermark, statistics, properties, next_row_id) {} + changelog_record_count, watermark, statistics, properties, next_row_id, uuid) {} Snapshot(const std::optional& version, int64_t id, int64_t schema_id, const std::string& base_manifest_list, @@ -127,7 +133,8 @@ class Snapshot : public Jsonizable { const std::optional& changelog_record_count, const std::optional& watermark, const std::optional& statistics, const std::optional>& properties, - const std::optional& next_row_id); + const std::optional& next_row_id, + const std::optional& uuid = std::nullopt); bool operator==(const Snapshot& other) const; bool TEST_Equal(const Snapshot& other) const; @@ -145,6 +152,11 @@ class Snapshot : public Jsonizable { return version_ == std::nullopt ? TABLE_STORE_02_VERSION : version_.value(); } + /// Generated on the client; null for snapshots created before uuid was introduced. + const std::optional& Uuid() const { + return uuid_; + } + int64_t Id() const { return id_; } @@ -237,6 +249,9 @@ class Snapshot : public Jsonizable { // version of snapshot // null for paimon <= 0.2 std::optional version_; + + std::optional uuid_; + int64_t id_ = -1; int64_t schema_id_ = -1; diff --git a/src/paimon/core/snapshot_test.cpp b/src/paimon/core/snapshot_test.cpp index 7562f38d6..6aab6563b 100644 --- a/src/paimon/core/snapshot_test.cpp +++ b/src/paimon/core/snapshot_test.cpp @@ -148,6 +148,49 @@ TEST_F(SnapshotTest, TestJsonizable) { ASSERT_EQ(ReplaceAll(json_str), ReplaceAll(new_json_str)); } +TEST_F(SnapshotTest, TestGenerateUuid) { + ASSERT_OK_AND_ASSIGN(std::string first, Snapshot::GenerateUuid()); + ASSERT_OK_AND_ASSIGN(std::string second, Snapshot::GenerateUuid()); + ASSERT_EQ(first.size(), 36u); + ASSERT_NE(first, second); +} + +TEST_F(SnapshotTest, TestUuid) { + std::string json_str = R"({ + "version" : 3, + "uuid" : "snapshot-uuid-1", + "id" : 1, + "schemaId" : 0, + "baseManifestList" : "manifest-list-d96fcc30-99e8-4f45-962b-a1157c56f378-0", + "baseManifestListSize" : 20, + "deltaManifestList" : "manifest-list-d96fcc30-99e8-4f45-962b-a1157c56f378-1", + "deltaManifestListSize" : 50, + "commitUser" : "0e4d92f7-53b0-40d6-a7c0-102bf3801e6a", + "commitIdentifier" : 9223372036854775807, + "commitKind" : "APPEND", + "timeMillis" : 1711692199281, + "totalRecordCount" : 3, + "deltaRecordCount" : 3, + "changelogRecordCount" : 0 + })"; + + ASSERT_OK_AND_ASSIGN(Snapshot snapshot, Snapshot::FromJsonString(json_str)); + ASSERT_EQ(snapshot.Uuid(), std::optional("snapshot-uuid-1")); + ASSERT_OK_AND_ASSIGN(std::string new_json_str, snapshot.ToJsonString()); + ASSERT_EQ(ReplaceAll(json_str), ReplaceAll(new_json_str)); + + Snapshot without_uuid(3, 1, 0, "manifest-list-d96fcc30-99e8-4f45-962b-a1157c56f378-0", 20, + "manifest-list-d96fcc30-99e8-4f45-962b-a1157c56f378-1", 50, std::nullopt, + std::nullopt, std::nullopt, "0e4d92f7-53b0-40d6-a7c0-102bf3801e6a", + 9223372036854775807ll, Snapshot::CommitKind::Append(), 1711692199281ll, 3, + 3, 0, std::nullopt, std::nullopt, std::nullopt, std::nullopt); + ASSERT_EQ(without_uuid.Uuid(), std::nullopt); + ASSERT_FALSE(without_uuid == snapshot); + ASSERT_TRUE(without_uuid.TEST_Equal(snapshot)); + ASSERT_OK_AND_ASSIGN(std::string no_uuid_json_str, without_uuid.ToJsonString()); + ASSERT_EQ(no_uuid_json_str.find("uuid"), std::string::npos); +} + TEST_F(SnapshotTest, TestSerializeAndDeserialize) { auto se_and_de = [&](const std::string& data_path) { auto fs = std::make_shared(); diff --git a/src/paimon/core/table/format/format_table_test.cpp b/src/paimon/core/table/format/format_table_test.cpp index f4d86cb82..c3e118951 100644 --- a/src/paimon/core/table/format/format_table_test.cpp +++ b/src/paimon/core/table/format/format_table_test.cpp @@ -65,6 +65,7 @@ #include "paimon/table/source/split.h" #include "paimon/table/source/table_read.h" #include "paimon/table/source/table_scan.h" +#include "paimon/testing/mock/mock_catalog.h" #include "paimon/testing/utils/testharness.h" #include "paimon/write_context.h" @@ -2689,8 +2690,6 @@ TEST(FormatTableTest, TestTheGenericEntryPointsReachAFormatTable) { ASSERT_NOK_WITH_MSG(FileStoreWrite::Create(std::move(write_id_context)), "a write id would name nothing"); - // The three `CommitContext` settings that describe snapshot machinery. Each is refused only - // when it is set away from its default, so an ordinary commit is unaffected. CommitContextBuilder empty_commit_builder(dir->Str(), "test-user"); empty_commit_builder.IgnoreEmptyCommit(false); ASSERT_OK_AND_ASSIGN(std::unique_ptr empty_commit_context, @@ -2703,6 +2702,21 @@ TEST(FormatTableTest, TestTheGenericEntryPointsReachAFormatTable) { ASSERT_OK_AND_ASSIGN(std::unique_ptr rest_context, rest_builder.Finish()); ASSERT_NOK_WITH_MSG(FileStoreCommit::Create(std::move(rest_context)), "rest catalog"); + CommitContextBuilder table_id_builder(dir->Str(), "test-user"); + table_id_builder.WithTableId("table-uuid"); + ASSERT_OK_AND_ASSIGN(std::unique_ptr table_id_context, + table_id_builder.Finish()); + ASSERT_NOK_WITH_MSG(FileStoreCommit::Create(std::move(table_id_context)), + "a format table sends no commit table request"); + + auto format_catalog = std::make_shared(); + format_catalog->SetTableSchema(table->LatestSchema()); + CommitContextBuilder catalog_builder(dir->Str(), "test-user"); + catalog_builder.WithCatalog(format_catalog, Identifier("db", "fmt")); + ASSERT_OK_AND_ASSIGN(std::unique_ptr catalog_context, catalog_builder.Finish()); + ASSERT_NOK_WITH_MSG(FileStoreCommit::Create(std::move(catalog_context)), + "keeps no snapshot for a catalog to take"); + CommitContextBuilder conflict_builder(dir->Str(), "test-user"); conflict_builder.AppendCommitCheckConflict(true); ASSERT_OK_AND_ASSIGN(std::unique_ptr conflict_context, @@ -2851,6 +2865,19 @@ TEST(FormatTableTest, TestAContextBuiltFromAFormatTableRefusesASecondAnswer) { write_fs_builder.WithFileSystem(dir->GetFileSystem()); ASSERT_NOK_WITH_MSG(write_fs_builder.Finish(), "carries the file system"); + auto catalog = std::make_shared(); + WriteContextBuilder write_catalog_builder(table); + ASSERT_NOK_WITH_MSG( + write_catalog_builder.WithCatalog(catalog, Identifier("db", "tbl")).Finish(), + "WithCatalog() requires a native table"); + catalog->SetTableSchema(latest.value()); + WriteContextBuilder path_catalog_builder(dir->Str(), "test-user"); + ASSERT_OK_AND_ASSIGN( + auto path_context, + path_catalog_builder.WithCatalog(catalog, Identifier("db", "tbl")).Finish()); + ASSERT_NOK_WITH_MSG(FileStoreWrite::Create(std::move(path_context)), + "use WriteContextBuilder(FormatTable)"); + CommitContextBuilder commit_fs_builder(table); commit_fs_builder.WithFileSystem(dir->GetFileSystem()); ASSERT_NOK_WITH_MSG(commit_fs_builder.Finish(), "carries the file system"); diff --git a/src/paimon/core/tag/tag.cpp b/src/paimon/core/tag/tag.cpp index 6ee31864d..67f87e582 100644 --- a/src/paimon/core/tag/tag.cpp +++ b/src/paimon/core/tag/tag.cpp @@ -76,7 +76,8 @@ Result Tag::TrimToSnapshot() const { DeltaManifestList(), DeltaManifestListSize(), ChangelogManifestList(), ChangelogManifestListSize(), IndexManifest(), CommitUser(), CommitIdentifier(), GetCommitKind(), TimeMillis(), TotalRecordCount(), DeltaRecordCount(), - ChangelogRecordCount(), Watermark(), Statistics(), Properties(), NextRowId()); + ChangelogRecordCount(), Watermark(), Statistics(), Properties(), NextRowId(), + Uuid()); } rapidjson::Value Tag::ToJson(rapidjson::Document::AllocatorType* allocator) const noexcept(false) { diff --git a/src/paimon/core/tag/tag_test.cpp b/src/paimon/core/tag/tag_test.cpp index c94d823b7..f3020ac7e 100644 --- a/src/paimon/core/tag/tag_test.cpp +++ b/src/paimon/core/tag/tag_test.cpp @@ -162,6 +162,41 @@ TEST_F(TagTest, TestJsonizable) { ASSERT_EQ(ReplaceAll(json_str, false), ReplaceAll(new_json_str, true)); } +TEST_F(TagTest, TestTrimToSnapshotKeepsTheUuid) { + const std::string json_str = R"({ + "version" : 3, + "uuid" : "snapshot-uuid-1", + "id" : 1, + "schemaId" : 0, + "baseManifestList" : "manifest-list-d96fcc30-99e8-4f45-962b-a1157c56f378-0", + "deltaManifestList" : "manifest-list-d96fcc30-99e8-4f45-962b-a1157c56f378-1", + "commitUser" : "0e4d92f7-53b0-40d6-a7c0-102bf3801e6a", + "commitIdentifier" : 9223372036854775807, + "commitKind" : "APPEND", + "timeMillis" : 1711692199281, + "totalRecordCount" : 3, + "deltaRecordCount" : 3, + "tagCreateTime" : [ 2026, 1, 3, 5, 7, 9, 11 ], + "tagTimeRetained" : 4.000000000 + })"; + + Tag tag; + ASSERT_OK(RapidJsonUtil::FromJsonString(json_str, &tag)); + ASSERT_EQ(tag.Uuid(), std::optional("snapshot-uuid-1")); + + ASSERT_OK_AND_ASSIGN(Snapshot snapshot, tag.TrimToSnapshot()); + ASSERT_EQ(snapshot.Uuid(), std::optional("snapshot-uuid-1")); + ASSERT_EQ(snapshot.Id(), 1); + ASSERT_EQ(snapshot.CommitUser(), "0e4d92f7-53b0-40d6-a7c0-102bf3801e6a"); + + const Tag without_uuid(3, 1, 0, "bml", std::nullopt, "dml", std::nullopt, std::nullopt, + std::nullopt, std::nullopt, "u", 1, Snapshot::CommitKind::Append(), 1, 1, + 1, std::nullopt, std::nullopt, std::nullopt, std::nullopt, std::nullopt, + std::nullopt, std::nullopt); + ASSERT_OK_AND_ASSIGN(Snapshot trimmed, without_uuid.TrimToSnapshot()); + ASSERT_EQ(trimmed.Uuid(), std::nullopt); +} + TEST_F(TagTest, TestSerializeAndDeserialize) { const auto se_and_de = [&](const std::string& data_path) { auto fs = std::make_shared(); diff --git a/src/paimon/core/utils/snapshot_manager.cpp b/src/paimon/core/utils/snapshot_manager.cpp index 9211d9665..3e6c85eaa 100644 --- a/src/paimon/core/utils/snapshot_manager.cpp +++ b/src/paimon/core/utils/snapshot_manager.cpp @@ -24,6 +24,7 @@ #include #include +#include "fmt/format.h" #include "paimon/common/utils/path_util.h" #include "paimon/common/utils/string_utils.h" #include "paimon/core/snapshot.h" @@ -57,41 +58,117 @@ const std::string& SnapshotManager::Branch() const { } Result> SnapshotManager::LatestSnapshotOfUser(const std::string& user) { - PAIMON_ASSIGN_OR_RAISE(std::optional latest_id, LatestSnapshotId()); - if (latest_id == std::nullopt) { + // Catalog snapshots may have no corresponding file in the table directory. + PAIMON_ASSIGN_OR_RAISE(LatestSnapshotResult latest, LatestSnapshotWithSource()); + return LatestSnapshotOfUserAtOrBefore(user, latest.snapshot, latest.from_catalog); +} + +Result> SnapshotManager::LatestSnapshotOfUserAtOrBefore( + const std::string& user, const std::optional& latest, + bool latest_from_catalog) const { + if (!latest) { return std::optional(); } - PAIMON_ASSIGN_OR_RAISE(std::optional earliest_id, EarliestSnapshotId()); - if (earliest_id == std::nullopt) { - return Status::Invalid( - "Latest snapshot id is not null, but earliest snapshot id is null. This is " - "unexpected."); + if (latest.value().CommitUser() == user) { + return latest; } - for (int64_t id = latest_id.value(); id >= earliest_id.value(); id--) { - PAIMON_ASSIGN_OR_RAISE(Snapshot snapshot, LoadSnapshot(id)); - if (snapshot.CommitUser() == user) { - return std::optional(snapshot); + // Only EARLIEST proves catalog history expired; the oldest local file may belong to + // an incompletely published history. + int64_t search_end = Snapshot::FIRST_SNAPSHOT_ID; + if (latest_from_catalog) { + search_end = ReadHint(EARLIEST, SnapshotDirectory()).value_or(Snapshot::FIRST_SNAPSHOT_ID); + } else { + PAIMON_ASSIGN_OR_RAISE(std::optional earliest_id, EarliestSnapshotId()); + search_end = earliest_id.value_or(Snapshot::FIRST_SNAPSHOT_ID); + } + search_end = std::max(search_end, Snapshot::FIRST_SNAPSHOT_ID); + for (int64_t id = latest.value().Id() - 1; id >= search_end; id--) { + Result snapshot = LoadSnapshot(id); + if (!snapshot.ok()) { + if (snapshot.status().IsNotExist()) { + if (latest_from_catalog) { + if (ExpiredSinceBoundaryWasRead(id)) { + break; + } + // Without proof of expiration, treating this gap as empty history could + // cause a recovering writer to commit twice. + return Status::Invalid(fmt::format( + "cannot tell which snapshot of table {} commit user {} wrote last: " + "snapshot #{} is not under the table directory although it is at or above " + "the earliest retained snapshot #{}, and this client loads only the " + "latest snapshot from the catalog which holds the versions of this table", + root_path_, user, id, search_end)); + } + break; + } + return snapshot.status(); + } + if (snapshot.value().CommitUser() == user) { + return std::optional(snapshot.value()); } } return std::optional(); } +bool SnapshotManager::ExpiredSinceBoundaryWasRead(int64_t id) const { + // Expiration deletes files before advancing EARLIEST; allow the hint time to catch up. + for (int32_t attempt = 0; attempt < READ_HINT_RETRY_NUM; ++attempt) { + std::optional earliest = ReadHint(EARLIEST, SnapshotDirectory()); + if (earliest && earliest.value() > id) { + return true; + } + std::this_thread::sleep_for(std::chrono::milliseconds(READ_HINT_RETRY_INTERVAL)); + } + return false; +} + Result SnapshotManager::LoadSnapshot(int64_t snapshot_id) const { return Snapshot::FromPath(fs_, SnapshotPath(snapshot_id)); } +void SnapshotManager::SetSnapshotLoader(SnapshotLoader loader) { + snapshot_loader_ = std::move(loader); +} + Result> SnapshotManager::LatestSnapshot() const { - PAIMON_ASSIGN_OR_RAISE(std::optional snapshot_id, LatestSnapshotId()); + PAIMON_ASSIGN_OR_RAISE(LatestSnapshotResult latest, LatestSnapshotWithSource()); + return latest.snapshot; +} + +Result SnapshotManager::LatestSnapshotWithSource() const { + if (snapshot_loader_) { + Result> loaded = snapshot_loader_(); + if (loaded.ok()) { + return LatestSnapshotResult{loaded.value(), /*from_catalog=*/true}; + } + if (!loaded.status().IsNotImplemented()) { + return loaded.status(); + } + } + PAIMON_ASSIGN_OR_RAISE(std::optional snapshot_id, LatestSnapshotIdFromFileSystem()); if (snapshot_id == std::nullopt) { - return std::optional(); - } else { - PAIMON_ASSIGN_OR_RAISE(Snapshot snapshot, LoadSnapshot(snapshot_id.value())); - return std::optional(snapshot); + return LatestSnapshotResult{std::optional(), /*from_catalog=*/false}; } + PAIMON_ASSIGN_OR_RAISE(Snapshot snapshot, LoadSnapshot(snapshot_id.value())); + return LatestSnapshotResult{std::optional(snapshot), /*from_catalog=*/false}; } Result> SnapshotManager::LatestSnapshotId() const { + if (snapshot_loader_) { + Result> loaded = snapshot_loader_(); + if (loaded.ok()) { + return loaded.value() ? std::optional(loaded.value().value().Id()) + : std::nullopt; + } + if (!loaded.status().IsNotImplemented()) { + return loaded.status(); + } + } + return LatestSnapshotIdFromFileSystem(); +} + +Result> SnapshotManager::LatestSnapshotIdFromFileSystem() const { return FindLatest( SnapshotDirectory(), std::string(SNAPSHOT_PREFIX), [this](int64_t snapshot_id) -> std::string { return SnapshotPath(snapshot_id); }); diff --git a/src/paimon/core/utils/snapshot_manager.h b/src/paimon/core/utils/snapshot_manager.h index 0df2ce2e3..5b346fe91 100644 --- a/src/paimon/core/utils/snapshot_manager.h +++ b/src/paimon/core/utils/snapshot_manager.h @@ -26,13 +26,13 @@ #include #include +#include "paimon/core/snapshot.h" #include "paimon/result.h" #include "paimon/status.h" #include "paimon/type_fwd.h" namespace paimon { -class Snapshot; class FileSystem; /// Manager for `Snapshot`, providing utility methods related to paths and snapshot hints. @@ -42,23 +42,55 @@ class SnapshotManager { static constexpr char EARLIEST[] = "EARLIEST"; static constexpr char LATEST[] = "LATEST"; + /// Loads the catalog's latest snapshot. Null means no snapshot; only `NotImplemented` + /// permits file-system fallback. + using SnapshotLoader = std::function>()>; + SnapshotManager(const std::shared_ptr& fs, const std::string& root_path); SnapshotManager(const std::shared_ptr& fs, const std::string& root_path, const std::string& branch); ~SnapshotManager(); + /// Sets the loader for `LatestSnapshot()` and `LatestSnapshotId()`. + /// Historical snapshots and manifests remain on the file system. + void SetSnapshotLoader(SnapshotLoader loader); + + /// Returns whether a catalog loader is configured, even if it falls back to the file system. + bool HasSnapshotLoader() const { + return static_cast(snapshot_loader_); + } + + /// Latest snapshot and its source. `from_catalog` is false after file-system fallback. + struct LatestSnapshotResult { + std::optional snapshot; + bool from_catalog = false; + }; + const std::shared_ptr& Fs() const; const std::string& RootPath() const; const std::string& Branch() const; Result> LatestSnapshot() const; + /// Returns the latest snapshot and source used to determine the history boundary. + Result LatestSnapshotWithSource() const; std::string SnapshotDirectory() const; std::string SnapshotPath(int64_t snapshot_id) const; + /// Finds the user's newest snapshot, reading older snapshots from the table directory. + /// Expired history ends the search. Missing catalog history at or above EARLIEST is an error + /// to prevent duplicate commits during recovery. Returns null if no matching snapshot is found. Result> LatestSnapshotOfUser(const std::string& user); + + /// Searches from the supplied snapshot; null `latest` returns null. + /// Pass the source flag returned with `latest` to select the correct history boundary. + Result> LatestSnapshotOfUserAtOrBefore( + const std::string& user, const std::optional& latest, + bool latest_from_catalog) const; Status CommitLatestHint(int64_t snapshot_id); Status CommitEarliestHint(int64_t snapshot_id); Result LoadSnapshot(int64_t snapshot_id) const; Result> EarliestSnapshotId() const; Result> LatestSnapshotId() const; + /// Finds the latest snapshot published to the file system, without consulting the loader. + Result> LatestSnapshotIdFromFileSystem() const; Result SnapshotExists(int64_t snapshot_id) const; Result> TryGetNonSnapshotFiles(int64_t older_than_ms) const; Result> GetAllSnapshots() const; @@ -69,6 +101,10 @@ class SnapshotManager { static constexpr int32_t READ_HINT_RETRY_NUM = 3; static constexpr int32_t READ_HINT_RETRY_INTERVAL = 1; + /// Rechecks EARLIEST a bounded number of times to confirm expiration. + /// False leaves expiration unconfirmed, so the caller must report the missing snapshot. + bool ExpiredSinceBoundaryWasRead(int64_t id) const; + std::string BranchPath() const; Result> FindEarliest( @@ -89,6 +125,7 @@ class SnapshotManager { std::shared_ptr fs_; std::string root_path_; std::string branch_; + SnapshotLoader snapshot_loader_; }; } // namespace paimon diff --git a/src/paimon/core/utils/snapshot_manager_test.cpp b/src/paimon/core/utils/snapshot_manager_test.cpp index d1bc83091..984bbb137 100644 --- a/src/paimon/core/utils/snapshot_manager_test.cpp +++ b/src/paimon/core/utils/snapshot_manager_test.cpp @@ -18,8 +18,11 @@ #include "paimon/core/utils/snapshot_manager.h" +#include #include #include +#include +#include #include #include "gtest/gtest.h" @@ -27,6 +30,9 @@ #include "paimon/core/snapshot.h" #include "paimon/core/utils/branch_manager.h" #include "paimon/fs/local/local_file_system.h" +#include "paimon/result.h" +#include "paimon/status.h" +#include "paimon/testing/utils/snapshot_test_helper.h" #include "paimon/testing/utils/testharness.h" namespace paimon::test { @@ -81,6 +87,300 @@ TEST(SnapshotManagerTest, TestSimpleWithBranch) { ASSERT_EQ(snapshot.value().CommitIdentifier(), 1); } +TEST(SnapshotManagerTest, SnapshotLoaderAnswersWhichSnapshotIsLatest) { + std::string test_data_path = paimon::test::GetDataDir() + "/orc/append_09.db/append_09"; + auto file_system = std::make_shared(); + SnapshotManager mgr(file_system, test_data_path); + ASSERT_OK_AND_ASSIGN(std::optional from_files, mgr.LatestSnapshotId()); + ASSERT_EQ(from_files.value(), 5); + ASSERT_OK_AND_ASSIGN(Snapshot second, mgr.LoadSnapshot(2)); + + mgr.SetSnapshotLoader( + [second]() -> Result> { return std::optional(second); }); + ASSERT_OK_AND_ASSIGN(std::optional from_loader, mgr.LatestSnapshotId()); + ASSERT_EQ(from_loader.value(), 2); + ASSERT_OK_AND_ASSIGN(std::optional latest, mgr.LatestSnapshot()); + ASSERT_TRUE(latest.has_value()); + ASSERT_EQ(latest.value().Id(), 2); + + mgr.SetSnapshotLoader( + []() -> Result> { return std::optional(); }); + ASSERT_OK_AND_ASSIGN(std::optional empty_id, mgr.LatestSnapshotId()); + ASSERT_EQ(empty_id, std::nullopt); + ASSERT_OK_AND_ASSIGN(std::optional empty, mgr.LatestSnapshot()); + ASSERT_FALSE(empty.has_value()); + + ASSERT_OK_AND_ASSIGN(std::optional earliest, mgr.EarliestSnapshotId()); + ASSERT_EQ(earliest.value(), 1); + ASSERT_OK_AND_ASSIGN(Snapshot loaded, mgr.LoadSnapshot(5)); + ASSERT_EQ(loaded.Id(), 5); +} + +TEST(SnapshotManagerTest, LatestSnapshotOfUserReadsTheLoadedSnapshot) { + std::string test_data_path = paimon::test::GetDataDir() + "/orc/append_09.db/append_09"; + auto file_system = std::make_shared(); + SnapshotManager mgr(file_system, test_data_path); + ASSERT_OK_AND_ASSIGN(Snapshot fifth, mgr.LoadSnapshot(5)); + + mgr.SetSnapshotLoader( + [fifth]() -> Result> { return std::optional(fifth); }); + ASSERT_OK_AND_ASSIGN(std::optional found, + mgr.LatestSnapshotOfUser(fifth.CommitUser())); + ASSERT_TRUE(found.has_value()); + ASSERT_EQ(found.value().Id(), 5); + + ASSERT_OK_AND_ASSIGN(std::optional missing, mgr.LatestSnapshotOfUser("nobody")); + ASSERT_FALSE(missing.has_value()); + + mgr.SetSnapshotLoader( + []() -> Result> { return std::optional(); }); + ASSERT_OK_AND_ASSIGN(std::optional none, + mgr.LatestSnapshotOfUser(fifth.CommitUser())); + ASSERT_FALSE(none.has_value()); +} + +TEST(SnapshotManagerTest, LatestSnapshotOfUserFailsWhenTheHistoryIsUnreadable) { + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + auto file_system = std::make_shared(); + SnapshotManager mgr(file_system, dir->Str()); + + Snapshot only_in_catalog = BuildTestSnapshot(9, "snapshot-uuid-9"); + mgr.SetSnapshotLoader([only_in_catalog]() -> Result> { + return std::optional(only_in_catalog); + }); + + ASSERT_OK_AND_ASSIGN(std::optional own, + mgr.LatestSnapshotOfUser(only_in_catalog.CommitUser())); + ASSERT_TRUE(own.has_value()); + ASSERT_EQ(own.value().Id(), 9); + + ASSERT_NOK_WITH_MSG(mgr.LatestSnapshotOfUser("somebody-else"), + "cannot tell which snapshot of table"); +} + +TEST(SnapshotManagerTest, LatestSnapshotOfUserPropagatesCorruptHistory) { + for (bool from_catalog : {false, true}) { + SCOPED_TRACE(from_catalog); + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + auto file_system = std::make_shared(); + SnapshotManager mgr(file_system, dir->Str()); + ASSERT_OK(file_system->Mkdirs(mgr.SnapshotDirectory())); + ASSERT_OK(file_system->WriteFile(mgr.SnapshotPath(1), "not json", false)); + Snapshot latest = BuildTestSnapshot(2); + ASSERT_OK_AND_ASSIGN(std::string json, latest.ToJsonString()); + ASSERT_OK(file_system->WriteFile(mgr.SnapshotPath(2), json, false)); + if (from_catalog) { + mgr.SetSnapshotLoader([latest]() -> Result> { + return std::optional(latest); + }); + } + + ASSERT_NOK_WITH_MSG(mgr.LatestSnapshotOfUser("somebody-else"), "deserialize failed"); + } +} + +TEST(SnapshotManagerTest, LatestSnapshotOfUserStopsAtTheEarliestRetainedSnapshot) { + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + auto file_system = std::make_shared(); + SnapshotManager mgr(file_system, dir->Str()); + ASSERT_OK(file_system->Mkdirs(mgr.SnapshotDirectory())); + for (int64_t id : {5, 6}) { + ASSERT_OK_AND_ASSIGN(std::string json, BuildTestSnapshot(id).ToJsonString()); + ASSERT_OK(file_system->WriteFile(mgr.SnapshotPath(id), json, false)); + } + ASSERT_OK(mgr.CommitEarliestHint(5)); + + Snapshot sixth = BuildTestSnapshot(6, "snapshot-uuid-6"); + mgr.SetSnapshotLoader( + [sixth]() -> Result> { return std::optional(sixth); }); + ASSERT_OK_AND_ASSIGN(std::optional other, mgr.LatestSnapshotOfUser("somebody-else")); + ASSERT_FALSE(other.has_value()); + + ASSERT_OK_AND_ASSIGN(std::optional bounded, + mgr.LatestSnapshotOfUserAtOrBefore("somebody-else", sixth, true)); + ASSERT_FALSE(bounded.has_value()); + + ASSERT_OK(mgr.CommitEarliestHint(2)); + ASSERT_NOK_WITH_MSG(mgr.LatestSnapshotOfUser("somebody-else"), + "is not under the table directory"); + ASSERT_NOK_WITH_MSG(mgr.LatestSnapshotOfUserAtOrBefore("somebody-else", sixth, true), + "is not under the table directory"); +} + +TEST(SnapshotManagerTest, LatestSnapshotOfUserAtOrBeforeWalksBelowTheGivenSnapshot) { + std::string test_data_path = paimon::test::GetDataDir() + "/orc/append_09.db/append_09"; + auto file_system = std::make_shared(); + SnapshotManager mgr(file_system, test_data_path); + ASSERT_OK_AND_ASSIGN(Snapshot third, mgr.LoadSnapshot(3)); + + ASSERT_OK_AND_ASSIGN(std::optional own, + mgr.LatestSnapshotOfUserAtOrBefore(third.CommitUser(), third, false)); + ASSERT_TRUE(own.has_value()); + ASSERT_EQ(own.value().Id(), 3); + + ASSERT_OK_AND_ASSIGN(std::optional none, + mgr.LatestSnapshotOfUserAtOrBefore("nobody", third, false)); + ASSERT_FALSE(none.has_value()); + + ASSERT_OK_AND_ASSIGN(std::optional empty, + mgr.LatestSnapshotOfUserAtOrBefore("whoever", std::nullopt, false)); + ASSERT_FALSE(empty.has_value()); +} + +TEST(SnapshotManagerTest, LatestSnapshotOfUserStopsAtAnExpiredSnapshotAfterFallingBack) { + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + auto file_system = std::make_shared(); + SnapshotManager mgr(file_system, dir->Str()); + ASSERT_OK(file_system->Mkdirs(mgr.SnapshotDirectory())); + for (int64_t id : {5, 6}) { + ASSERT_OK_AND_ASSIGN(std::string json, BuildTestSnapshot(id).ToJsonString()); + ASSERT_OK(file_system->WriteFile(mgr.SnapshotPath(id), json, false)); + } + mgr.SetSnapshotLoader([]() -> Result> { + return Status::NotImplemented("this catalog does not serve snapshots"); + }); + + ASSERT_OK_AND_ASSIGN(SnapshotManager::LatestSnapshotResult latest, + mgr.LatestSnapshotWithSource()); + ASSERT_FALSE(latest.from_catalog); + ASSERT_OK_AND_ASSIGN(std::optional other, mgr.LatestSnapshotOfUser("somebody-else")); + ASSERT_FALSE(other.has_value()); + + ASSERT_OK_AND_ASSIGN( + std::optional bounded, + mgr.LatestSnapshotOfUserAtOrBefore("somebody-else", latest.snapshot, latest.from_catalog)); + ASSERT_FALSE(bounded.has_value()); +} + +namespace { +class ExpiringHintFileSystem : public LocalFileSystem { + public: + ExpiringHintFileSystem(const std::string& hint_path, const std::string& refreshed, + int32_t stale_reads) + : hint_path_(hint_path), refreshed_(refreshed), stale_reads_(stale_reads) {} + + Status ReadFile(const std::string& path, std::string* content) override { + if (path == hint_path_ && hint_reads_++ >= stale_reads_) { + *content = refreshed_; + return Status::OK(); + } + return LocalFileSystem::ReadFile(path, content); + } + + int32_t HintReads() const { + return hint_reads_; + } + + private: + std::string hint_path_; + std::string refreshed_; + int32_t stale_reads_; + int32_t hint_reads_ = 0; +}; + +std::shared_ptr SetUpDeletedButUnannouncedSnapshots( + const std::string& root_path, int32_t stale_reads) { + SnapshotManager paths(std::make_shared(), root_path); + auto file_system = std::make_shared( + PathUtil::JoinPath(paths.SnapshotDirectory(), SnapshotManager::EARLIEST), "5", stale_reads); + SnapshotManager writer(file_system, root_path); + EXPECT_OK(file_system->Mkdirs(writer.SnapshotDirectory())); + for (int64_t id : {5, 6}) { + EXPECT_OK_AND_ASSIGN(std::string json, BuildTestSnapshot(id).ToJsonString()); + EXPECT_OK(file_system->WriteFile(writer.SnapshotPath(id), json, false)); + } + EXPECT_OK(writer.CommitEarliestHint(1)); + return file_system; +} + +void SetCatalogHoldingTheSixthSnapshot(SnapshotManager* mgr) { + Snapshot sixth = BuildTestSnapshot(6, "snapshot-uuid-6"); + mgr->SetSnapshotLoader( + [sixth]() -> Result> { return std::optional(sixth); }); +} +} // namespace + +TEST(SnapshotManagerTest, LatestSnapshotOfUserRetriesTheBoundaryWhileItIsStillStale) { + for (int32_t stale_reads : {1, 2, 3}) { + SCOPED_TRACE(stale_reads); + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + std::shared_ptr file_system = + SetUpDeletedButUnannouncedSnapshots(dir->Str(), stale_reads); + ASSERT_NE(nullptr, file_system); + SnapshotManager mgr(file_system, dir->Str()); + SetCatalogHoldingTheSixthSnapshot(&mgr); + + ASSERT_OK_AND_ASSIGN(std::optional other, + mgr.LatestSnapshotOfUser("somebody-else")); + ASSERT_FALSE(other.has_value()); + ASSERT_EQ(stale_reads + 1, file_system->HintReads()); + } +} + +TEST(SnapshotManagerTest, LatestSnapshotOfUserStillReportsAGapItCannotExplain) { + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + std::shared_ptr file_system = + SetUpDeletedButUnannouncedSnapshots(dir->Str(), 100); + ASSERT_NE(nullptr, file_system); + SnapshotManager mgr(file_system, dir->Str()); + SetCatalogHoldingTheSixthSnapshot(&mgr); + + ASSERT_NOK_WITH_MSG(mgr.LatestSnapshotOfUser("somebody-else"), + "is not under the table directory"); + ASSERT_GT(file_system->HintReads(), 1); + ASSERT_LT(file_system->HintReads(), 100); +} + +TEST(SnapshotManagerTest, LatestSnapshotOfUserStopsAtAnExpiredSnapshot) { + auto dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + auto file_system = std::make_shared(); + SnapshotManager mgr(file_system, dir->Str()); + ASSERT_OK(file_system->Mkdirs(mgr.SnapshotDirectory())); + for (int64_t id : {1, 3}) { + ASSERT_OK_AND_ASSIGN(std::string json, BuildTestSnapshot(id).ToJsonString()); + ASSERT_OK(file_system->WriteFile(mgr.SnapshotPath(id), json, false)); + } + + Snapshot latest = BuildTestSnapshot(3); + ASSERT_OK_AND_ASSIGN(std::optional own, + mgr.LatestSnapshotOfUser(latest.CommitUser())); + ASSERT_TRUE(own.has_value()); + ASSERT_EQ(own.value().Id(), 3); + ASSERT_OK_AND_ASSIGN(std::optional other, mgr.LatestSnapshotOfUser("somebody-else")); + ASSERT_FALSE(other.has_value()); +} + +TEST(SnapshotManagerTest, SnapshotLoaderFallsBackAndPropagates) { + std::string test_data_path = paimon::test::GetDataDir() + "/orc/append_09.db/append_09"; + auto file_system = std::make_shared(); + SnapshotManager mgr(file_system, test_data_path); + + int32_t asked = 0; + mgr.SetSnapshotLoader([&asked]() -> Result> { + ++asked; + return Status::NotImplemented("this catalog does not serve snapshots"); + }); + ASSERT_OK_AND_ASSIGN(std::optional id, mgr.LatestSnapshotId()); + ASSERT_EQ(id.value(), 5); + ASSERT_EQ(asked, 1); + ASSERT_OK_AND_ASSIGN(std::optional latest, mgr.LatestSnapshot()); + ASSERT_TRUE(latest.has_value()); + ASSERT_EQ(latest.value().Id(), 5); + ASSERT_EQ(asked, 2); + + mgr.SetSnapshotLoader( + []() -> Result> { return Status::IOError("catalog unreachable"); }); + ASSERT_NOK_WITH_MSG(mgr.LatestSnapshotId(), "catalog unreachable"); + ASSERT_NOK_WITH_MSG(mgr.LatestSnapshot(), "catalog unreachable"); +} + TEST(SnapshotManagerTest, TestGetAllSnapshots) { std::string test_data_path = paimon::test::GetDataDir() + "/orc/append_09.db/append_09"; auto file_system = std::make_shared(); diff --git a/src/paimon/rest/resource_paths.cpp b/src/paimon/rest/resource_paths.cpp index d4b6adba4..75ce9f7b3 100644 --- a/src/paimon/rest/resource_paths.cpp +++ b/src/paimon/rest/resource_paths.cpp @@ -63,4 +63,14 @@ std::string ResourcePaths::Snapshots(const std::string& database_name, return Table(database_name, table_name) + "/snapshots"; } +std::string ResourcePaths::CommitTable(const std::string& database_name, + const std::string& table_name) const { + return Table(database_name, table_name) + "/commit"; +} + +std::string ResourcePaths::TableSnapshot(const std::string& database_name, + const std::string& table_name) const { + return Table(database_name, table_name) + "/snapshot"; +} + } // namespace paimon diff --git a/src/paimon/rest/resource_paths.h b/src/paimon/rest/resource_paths.h index 009e6e073..2fc58e20c 100644 --- a/src/paimon/rest/resource_paths.h +++ b/src/paimon/rest/resource_paths.h @@ -38,6 +38,10 @@ class ResourcePaths { std::string Table(const std::string& database_name, const std::string& table_name) const; std::string RenameTable() const; std::string Snapshots(const std::string& database_name, const std::string& table_name) const; + std::string CommitTable(const std::string& database_name, const std::string& table_name) const; + /// Endpoint for the table's current snapshot. + std::string TableSnapshot(const std::string& database_name, + const std::string& table_name) const; private: /// "/v1" or "/v1/{encoded prefix}". diff --git a/src/paimon/rest/resource_paths_test.cpp b/src/paimon/rest/resource_paths_test.cpp index a0ea565a4..560b1d196 100644 --- a/src/paimon/rest/resource_paths_test.cpp +++ b/src/paimon/rest/resource_paths_test.cpp @@ -31,6 +31,8 @@ TEST(ResourcePathsTest, WithPrefix) { ASSERT_EQ("/v1/my+prefix/databases/db/tables/t1", paths.Table("db", "t1")); ASSERT_EQ("/v1/my+prefix/tables/rename", paths.RenameTable()); ASSERT_EQ("/v1/my+prefix/databases/db/tables/t1/snapshots", paths.Snapshots("db", "t1")); + ASSERT_EQ("/v1/my+prefix/databases/db/tables/t1/commit", paths.CommitTable("db", "t1")); + ASSERT_EQ("/v1/my+prefix/databases/db/tables/t1/snapshot", paths.TableSnapshot("db", "t1")); } TEST(ResourcePathsTest, WithoutPrefix) { diff --git a/src/paimon/rest/rest_api.cpp b/src/paimon/rest/rest_api.cpp index a28e19916..86a4eb5c8 100644 --- a/src/paimon/rest/rest_api.cpp +++ b/src/paimon/rest/rest_api.cpp @@ -19,14 +19,18 @@ #include "paimon/rest/rest_api.h" #include +#include #include #include +#include #include "fmt/format.h" #include "paimon/catalog_options.h" +#include "paimon/common/utils/checked_cast.h" #include "paimon/common/utils/options_utils.h" #include "paimon/common/utils/rapidjson_util.h" #include "paimon/common/utils/sensitive_config_utils.h" +#include "paimon/core/catalog/commit_table_request.h" #include "paimon/logging.h" #include "paimon/rest/rest_util.h" @@ -131,6 +135,7 @@ Status RestApi::ErrorToStatus(const RestHttpClient::Response& response, bool fol int64_t code = response.code; std::string message; std::string resource_info; + std::string resource_type; bool body_parsed = false; if (!response.body.empty()) { ErrorResponse error; @@ -139,6 +144,7 @@ Status RestApi::ErrorToStatus(const RestHttpClient::Response& response, bool fol // The message may embed secrets (e.g. "password=..."), so it is redacted. message = SensitiveConfigUtils::RedactText(error.GetMessage()); if (!error.GetResourceType().empty()) { + resource_type = error.GetResourceType(); resource_info = fmt::format(" (resource type: {}, resource name: {})", error.GetResourceType(), error.GetResourceName()); } @@ -190,12 +196,13 @@ Status RestApi::ErrorToStatus(const RestHttpClient::Response& response, bool fol Status::IOError(fmt::format("rest request failed with code {}: {}", code, message)); break; } - return status.WithDetail(std::make_shared(code)); + return status.WithDetail(std::make_shared(code, resource_type)); } Result RestApi::Execute( const std::string& method, const std::string& path, - const std::map& query_params, const std::string& body) const { + const std::map& query_params, const std::string& body, + bool retry_safe) const { RestAuthParameter auth_parameter = RestAuthParameter::Create(method, path, query_params, body); StringMap request_headers = base_headers_; if (!body.empty()) { @@ -207,7 +214,7 @@ Result RestApi::Execute( bool follow_redirects = auth_provider_->AllowsRedirects(); PAIMON_ASSIGN_OR_RAISE( RestHttpClient::Response response, - client_->Execute(method, path, query_params, headers, body, follow_redirects)); + client_->Execute(method, path, query_params, headers, body, follow_redirects, retry_safe)); if (!response.IsSuccessful()) { return ErrorToStatus(response, follow_redirects); } @@ -295,4 +302,41 @@ Result> RestApi::ListSnapshots(const Identifier& identifie resource_paths_.Snapshots(identifier.GetDatabaseName(), identifier.GetTableName())); } +Result> RestApi::LoadSnapshot(const Identifier& identifier) const { + std::string path = + resource_paths_.TableSnapshot(identifier.GetDatabaseName(), identifier.GetTableName()); + Result response = + GetEntity(path, /*query_params=*/{}); + if (response.ok()) { + return response.value().GetSnapshot(); + } + // A snapshot-specific 404 means no snapshot yet; a missing table remains an error. + const std::shared_ptr& detail = response.status().detail(); + if (response.status().IsNotExist() && detail != nullptr && + std::string(detail->type_id()) == RestErrorDetail::kTypeId && + checked_cast(detail.get())->GetResourceType() == + ErrorResponse::kResourceTypeSnapshot) { + return std::optional(); + } + return response.status(); +} + +Result RestApi::CommitSnapshot(const Identifier& identifier, + const std::optional& table_uuid, + const std::optional& base_snapshot_uuid, + const Snapshot& snapshot, + const std::vector& statistics) const { + CommitTableRequest request(table_uuid, base_snapshot_uuid, snapshot, statistics); + PAIMON_ASSIGN_OR_RAISE(std::string body, request.ToJsonString()); + std::string path = + resource_paths_.CommitTable(identifier.GetDatabaseName(), identifier.GetTableName()); + // Replaying an accepted commit can report a conflict and trigger manifest deletion. + // Send once and let FilterAndCommit() recover an unknown outcome. + PAIMON_ASSIGN_OR_RAISE(RestHttpClient::Response response, + Execute("POST", path, {}, body, /*retry_safe=*/false)); + CommitTableResponse entity; + PAIMON_RETURN_NOT_OK(ParseResponseBody(response.body, path, &entity)); + return entity.IsSuccess(); +} + } // namespace paimon diff --git a/src/paimon/rest/rest_api.h b/src/paimon/rest/rest_api.h index 5d642a194..500de03d5 100644 --- a/src/paimon/rest/rest_api.h +++ b/src/paimon/rest/rest_api.h @@ -20,10 +20,12 @@ #include #include +#include #include #include #include "paimon/catalog/identifier.h" +#include "paimon/core/partition/partition_statistics.h" #include "paimon/core/snapshot.h" #include "paimon/rest/resource_paths.h" #include "paimon/rest/rest_auth.h" @@ -41,7 +43,8 @@ class RestErrorDetail : public StatusDetail { public: static constexpr const char* kTypeId = "rest-error-detail"; - explicit RestErrorDetail(int64_t code) : code_(code) {} + explicit RestErrorDetail(int64_t code, const std::string& resource_type = "") + : code_(code), resource_type_(resource_type) {} const char* type_id() const override { return kTypeId; @@ -55,8 +58,15 @@ class RestErrorDetail : public StatusDetail { return code_; } + /// Resource type reported by the server, or empty if absent. + /// A SNAPSHOT-specific 404 denotes an existing table without a snapshot. + const std::string& GetResourceType() const { + return resource_type_; + } + private: int64_t code_; + std::string resource_type_; }; /// The client of the REST catalog server. This layer only talks HTTP + JSON and never @@ -105,6 +115,16 @@ class RestApi { Result> ListSnapshots(const Identifier& identifier) const; + /// The table's current snapshot, or null when it has none. + Result> LoadSnapshot(const Identifier& identifier) const; + + /// See `VersionManagedCatalog::CommitSnapshot()` for arguments and result semantics. + Result CommitSnapshot(const Identifier& identifier, + const std::optional& table_uuid, + const std::optional& base_snapshot_uuid, + const Snapshot& snapshot, + const std::vector& statistics) const; + /// Maps a non-successful http response to a status: 404 becomes `NotExist`, 409 /// becomes `Exist`, 400 becomes `Invalid`, 501 becomes `NotImplemented` and the /// other codes become `IOError`. A redirect returned while `follow_redirects` is @@ -118,11 +138,11 @@ class RestApi { const std::map& base_headers, const std::map& options, const ResourcePaths& paths); - /// Executes one request with the authentication headers merged in, mapping a - /// non-successful response to an error status. + /// Executes an authenticated request and maps unsuccessful responses to errors. + /// `retry_safe=false` disables retries and redirects. Result Execute(const std::string& method, const std::string& path, const std::map& query_params, - const std::string& body) const; + const std::string& body, bool retry_safe = true) const; template Result GetEntity(const std::string& path, diff --git a/src/paimon/rest/rest_catalog.cpp b/src/paimon/rest/rest_catalog.cpp index c2cb2075c..953263764 100644 --- a/src/paimon/rest/rest_catalog.cpp +++ b/src/paimon/rest/rest_catalog.cpp @@ -348,16 +348,24 @@ Result> RestCatalog::ToTableSchema( Result> RestCatalog::LoadDataTableSchema( const Identifier& data_identifier, const std::optional& branch, - std::string* table_path) const { + std::string* table_path, std::string* table_id) const { PAIMON_ASSIGN_OR_RAISE(GetTableResponse response, api_->GetTable(data_identifier)); if (table_path != nullptr) { *table_path = response.GetPath(); } + if (table_id != nullptr) { + *table_id = response.GetId(); + } PAIMON_ASSIGN_OR_RAISE(std::unique_ptr schema, ToTableSchema(response, branch)); return std::shared_ptr(std::move(schema)); } Result> RestCatalog::LoadTableSchema(const Identifier& identifier) const { + return LoadTableSchema(identifier, /*table_id=*/nullptr); +} + +Result> RestCatalog::LoadTableSchema(const Identifier& identifier, + std::string* table_id) const { // Serve the global system tables of the "sys" database locally, like // FileSystemCatalog. if (CatalogUtils::IsSystemDatabase(identifier.GetDatabaseName())) { @@ -390,7 +398,8 @@ Result> RestCatalog::LoadTableSchema(const Identifier& i } std::string table_path; PAIMON_ASSIGN_OR_RAISE(std::shared_ptr latest_schema, - LoadDataTableSchema(load_identifier, branch, &table_path)); + LoadDataTableSchema(load_identifier, branch, &table_path, + /*table_id=*/nullptr)); std::map dynamic_options; if (branch) { dynamic_options[Options::BRANCH] = branch.value(); @@ -403,7 +412,7 @@ Result> RestCatalog::LoadTableSchema(const Identifier& i return std::make_shared(std::move(arrow_schema)); } PAIMON_ASSIGN_OR_RAISE(std::shared_ptr schema, - LoadDataTableSchema(load_identifier, branch, nullptr)); + LoadDataTableSchema(load_identifier, branch, nullptr, table_id)); return checked_pointer_cast(schema); } @@ -415,8 +424,9 @@ Result> RestCatalog::LoadFormatTable( branch = NormalizeBranch(std::move(branch)); PAIMON_ASSIGN_OR_RAISE(Identifier load_identifier, ToLoadIdentifier(identifier)); std::string location; - PAIMON_ASSIGN_OR_RAISE(std::shared_ptr schema, - LoadDataTableSchema(load_identifier, branch, &location)); + PAIMON_ASSIGN_OR_RAISE( + std::shared_ptr schema, + LoadDataTableSchema(load_identifier, branch, &location, /*table_id=*/nullptr)); // A rest catalog holds the schema itself, so everything below the location is data. return FormatTable::Create(fs_, location, identifier, checked_pointer_cast(schema), /*location_carries_paimon_metadata=*/false, @@ -424,10 +434,13 @@ Result> RestCatalog::LoadFormatTable( } Result> RestCatalog::GetTable(const Identifier& identifier) const { - PAIMON_ASSIGN_OR_RAISE(std::shared_ptr schema, LoadTableSchema(identifier)); + // Read UUID and schema together to avoid mixing table generations after recreation. + std::string uuid; + PAIMON_ASSIGN_OR_RAISE(std::shared_ptr schema, LoadTableSchema(identifier, &uuid)); PAIMON_RETURN_NOT_OK( CatalogUtils::CheckManagedTableType(identifier, schema, "Catalog::GetTable")); - return std::make_shared(schema, identifier.GetDatabaseName(), identifier.GetTableName()); + return std::make_shared
(schema, identifier.GetDatabaseName(), identifier.GetTableName(), + uuid); } std::string RestCatalog::GetRootPath() const { @@ -461,4 +474,21 @@ Result> RestCatalog::ListSnapshots(const Identifier& i return result; } +Result> RestCatalog::LoadSnapshot(const Identifier& identifier) const { + PAIMON_RETURN_NOT_OK(CatalogUtils::CheckNotSystemTable(identifier, "loadSnapshot")); + PAIMON_ASSIGN_OR_RAISE(Identifier load_identifier, ToLoadIdentifier(identifier)); + return api_->LoadSnapshot(load_identifier); +} + +Result RestCatalog::CommitSnapshot(const Identifier& identifier, + const std::optional& table_uuid, + const std::optional& base_snapshot_uuid, + const Snapshot& snapshot, + const std::vector& statistics) { + PAIMON_RETURN_NOT_OK(CatalogUtils::CheckNotSystemTable(identifier, "commitSnapshot")); + PAIMON_ASSIGN_OR_RAISE(Identifier load_identifier, ToLoadIdentifier(identifier)); + return api_->CommitSnapshot(load_identifier, table_uuid, base_snapshot_uuid, snapshot, + statistics); +} + } // namespace paimon diff --git a/src/paimon/rest/rest_catalog.h b/src/paimon/rest/rest_catalog.h index 8999a7449..1c0b6b525 100644 --- a/src/paimon/rest/rest_catalog.h +++ b/src/paimon/rest/rest_catalog.h @@ -25,6 +25,7 @@ #include #include "paimon/catalog/catalog.h" +#include "paimon/core/catalog/version_managed_catalog.h" #include "paimon/logging.h" #include "paimon/rest/rest_api.h" #include "paimon/result.h" @@ -39,7 +40,7 @@ class TableSchema; /// A catalog backed by a REST catalog server. Metadata operations are delegated to /// `RestApi`; table data is accessed through the file system configured by the /// server-merged options. -class RestCatalog : public Catalog { +class RestCatalog : public Catalog, public VersionManagedCatalog { public: /// Creates the catalog: fetches and merges "/v1/config" from the server configured /// by `CatalogOptions::URI`, then builds the file system from the merged options. @@ -75,6 +76,18 @@ class RestCatalog : public Catalog { Result> ListSnapshots(const Identifier& identifier, const std::string& branch) const override; + bool SupportsVersionManagement() const override { + return true; + } + + Result> LoadSnapshot(const Identifier& identifier) const override; + + Result CommitSnapshot(const Identifier& identifier, + const std::optional& table_uuid, + const std::optional& base_snapshot_uuid, + const Snapshot& snapshot, + const std::vector& statistics) override; + /// Options merged with the server side config. const std::map& GetOptions() const override; @@ -88,11 +101,15 @@ class RestCatalog : public Catalog { RestCatalog(std::unique_ptr api, const std::shared_ptr& fs, const std::string& warehouse); - /// Loads the table from the server and converts the response to a `TableSchema` - /// (options are enriched with the table path, audit info and branch). + /// Loads the schema and catalog table ID from the same response. + Result> LoadTableSchema(const Identifier& identifier, + std::string* table_id) const; + + /// Loads the schema with table path, audit and branch options. + /// Fills `table_path` and `table_id` when supplied. Result> LoadDataTableSchema( const Identifier& data_identifier, const std::optional& branch, - std::string* table_path) const; + std::string* table_path, std::string* table_id) const; static Result> ToTableSchema( const GetTableResponse& response, const std::optional& branch); diff --git a/src/paimon/rest/rest_catalog_test.cpp b/src/paimon/rest/rest_catalog_test.cpp index 6ac385e6a..d48bd2adf 100644 --- a/src/paimon/rest/rest_catalog_test.cpp +++ b/src/paimon/rest/rest_catalog_test.cpp @@ -36,14 +36,23 @@ #include "paimon/catalog/catalog.h" #include "paimon/catalog/table.h" #include "paimon/catalog_options.h" +#include "paimon/commit_context.h" +#include "paimon/commit_message.h" #include "paimon/common/utils/checked_cast.h" +#include "paimon/common/utils/path_util.h" #include "paimon/common/utils/string_utils.h" +#include "paimon/core/catalog/commit_table_request.h" +#include "paimon/core/partition/partition_statistics.h" #include "paimon/core/schema/table_schema.h" #include "paimon/defs.h" +#include "paimon/file_store_commit.h" +#include "paimon/fs/file_system.h" +#include "paimon/memory/memory_pool.h" #include "paimon/rest/mock_rest_server.h" #include "paimon/rest/rest_api.h" #include "paimon/schema/schema.h" #include "paimon/table/format/format_table.h" +#include "paimon/testing/utils/snapshot_test_helper.h" #include "paimon/testing/utils/testharness.h" namespace paimon::test { @@ -60,6 +69,7 @@ struct MockCatalogState { std::string schema_json; int64_t schema_id = 0; std::string path; + std::string id = "1"; }; std::map> databases; // headers of the last request, with lower-cased names @@ -72,6 +82,17 @@ struct MockCatalogState { // how many create-table requests reached the server, so a schema the client should have // refused can be shown never to have been sent int32_t create_table_requests = 0; + std::string last_commit_body; + std::string last_commit_table; + bool refuse_commit = false; + bool break_commit_response = false; + bool commit_response_without_outcome = false; + bool accept_commit_then_report_unavailable = false; + int32_t accepted_commit_response_code = 503; + int32_t commit_requests = 0; + std::optional current_snapshot; + bool null_snapshot_response = false; + std::string last_snapshot_table; // guards all fields above: the handler runs on the server's accept thread while // tests seed and inspect the state std::mutex mutex; @@ -99,10 +120,11 @@ std::string SnapshotJson(int64_t id) { } std::string TableResponseJson(const std::string& name, const MockCatalogState::TableData& table) { + std::string id_member = table.id.empty() ? "" : fmt::format(R"("id":"{}",)", table.id); return fmt::format( - R"({{"id":"1","name":"{}","path":"{}","isExternal":false,"schemaId":{},"schema":{},)" + R"({{{}"name":"{}","path":"{}","isExternal":false,"schemaId":{},"schema":{},)" R"("owner":"owner1","updatedAt":123}})", - name, table.path, table.schema_id, table.schema_json); + id_member, name, table.path, table.schema_id, table.schema_json); } // Serves `names` one item per page to exercise the pagination loop of the client. @@ -276,20 +298,101 @@ MockRestServer::Response HandleCatalogRequest(MockCatalogState* state, return MockError(400, "", "", "unsupported method"); } - // "/{table}" or "/{table}/snapshots" std::string table_name = table_part.substr(1); bool list_snapshots = false; + bool commit_table = false; + bool load_snapshot = false; const std::string snapshots_suffix = "/snapshots"; + const std::string snapshot_suffix = "/snapshot"; + const std::string commit_suffix = "/commit"; if (table_name.size() > snapshots_suffix.size() && table_name.compare(table_name.size() - snapshots_suffix.size(), snapshots_suffix.size(), snapshots_suffix) == 0) { table_name = table_name.substr(0, table_name.size() - snapshots_suffix.size()); list_snapshots = true; + } else if (table_name.size() > commit_suffix.size() && + table_name.compare(table_name.size() - commit_suffix.size(), commit_suffix.size(), + commit_suffix) == 0) { + table_name = table_name.substr(0, table_name.size() - commit_suffix.size()); + commit_table = true; + } else if (table_name.size() > snapshot_suffix.size() && + table_name.compare(table_name.size() - snapshot_suffix.size(), + snapshot_suffix.size(), snapshot_suffix) == 0) { + table_name = table_name.substr(0, table_name.size() - snapshot_suffix.size()); + load_snapshot = true; } auto table_iter = tables.find(table_name); if (table_iter == tables.end()) { return MockError(404, ErrorResponse::kResourceTypeTable, table_name, "table not found"); } + if (commit_table) { + if (request.method != "POST") { + return MockError(400, "", "", "unsupported method"); + } + state->last_commit_body = request.body; + state->last_commit_table = table_name; + ++state->commit_requests; + if (state->accept_commit_then_report_unavailable) { + if (state->commit_requests == 1) { + Result taken = CommitTableRequest::FromJsonString(request.body); + if (taken.ok()) { + Result json = taken.value().GetSnapshot().ToJsonString(); + if (json.ok()) { + state->current_snapshot = json.value(); + } + } + if (state->accepted_commit_response_code == 200) { + return JsonResponse(200, R"({"success": null})"); + } + if (state->accepted_commit_response_code == 204) { + return JsonResponse(204, ""); + } + MockRestServer::Response response = MockError(state->accepted_commit_response_code, + "", "", "commit outcome unknown"); + response.headers["Location"] = request.path; + return response; + } + bool based_on_held = false; + Result later = CommitTableRequest::FromJsonString(request.body); + if (later.ok() && state->current_snapshot) { + Result held = Snapshot::FromJsonString(state->current_snapshot.value()); + based_on_held = + held.ok() && later.value().GetBaseSnapshotUuid() == held.value().Uuid(); + if (based_on_held) { + Result json = later.value().GetSnapshot().ToJsonString(); + if (json.ok()) { + state->current_snapshot = json.value(); + } + } + } + return JsonResponse(200, CommitTableResponse(based_on_held).ToJsonString().value()); + } + if (state->break_commit_response) { + return JsonResponse(200, "not json"); + } + if (state->commit_response_without_outcome) { + return JsonResponse(200, R"({"success": null})"); + } + CommitTableResponse response(!state->refuse_commit); + return JsonResponse(200, response.ToJsonString().value()); + } + if (load_snapshot) { + if (request.method != "GET") { + return MockError(400, "", "", "unsupported method"); + } + state->last_snapshot_table = table_name; + if (state->null_snapshot_response) { + return JsonResponse(200, R"({"snapshot": null})"); + } + if (!state->current_snapshot) { + return MockError(404, ErrorResponse::kResourceTypeSnapshot, table_name, + "snapshot not found"); + } + return JsonResponse(200, fmt::format(R"({{"snapshot":{{"snapshot":{},"recordCount":1,)" + R"("fileSizeInBytes":2,"fileCount":3,)" + R"("lastFileCreationTime":4}}}})", + state->current_snapshot.value())); + } if (list_snapshots) { // two pages, out of order to exercise pagination and sorting auto token_iter = request.query_params.find(RestApi::kQueryParamPageToken); @@ -341,8 +444,15 @@ class RestCatalogTest : public ::testing::Test { } } - Result> CreateRestCatalog() { - return RestCatalog::Create(kWarehouse, options_, nullptr); + Result> CreateRestCatalog( + const RestHttpClient::Config& http_config = RestHttpClient::Config()) { + return RestCatalog::Create(kWarehouse, options_, nullptr, http_config); + } + + static RestHttpClient::Config FastRetryConfig() { + RestHttpClient::Config config; + config.retry_base_delay_ms = 1; + return config; } Status CreateSampleTable(Catalog* catalog, const Identifier& identifier, @@ -503,8 +613,18 @@ TEST_F(RestCatalogTest, TableOperations) { ASSERT_OK_AND_ASSIGN(std::string location, catalog->GetTableLocation(identifier)); ASSERT_EQ("wh1/db1.db/t1", location); + int32_t requests_before_get_table = 0; + { + std::lock_guard lock(state_->mutex); + requests_before_get_table = state_->get_table_requests; + } ASSERT_OK_AND_ASSIGN(std::shared_ptr
table, catalog->GetTable(identifier)); ASSERT_EQ("t1", table->Name()); + ASSERT_EQ("1", table->Uuid()); + { + std::lock_guard lock(state_->mutex); + ASSERT_EQ(requests_before_get_table + 1, state_->get_table_requests); + } std::shared_ptr schema = std::dynamic_pointer_cast(table->LatestSchema()); ASSERT_NE(nullptr, schema); @@ -565,6 +685,42 @@ TEST_F(RestCatalogTest, TableOperations) { ASSERT_TRUE(drop_missing.IsNotExist()) << drop_missing.ToString(); } +TEST_F(RestCatalogTest, TableUuidFallsBackToFullNameWithoutServerId) { + ASSERT_OK_AND_ASSIGN(std::unique_ptr catalog, CreateRestCatalog()); + ASSERT_OK(catalog->CreateDatabase("db1", {}, false)); + Identifier identifier("db1", "t1"); + ASSERT_OK(CreateSampleTable(catalog.get(), identifier)); + { + std::lock_guard lock(state_->mutex); + state_->databases["db1"]["t1"].id.clear(); + } + + ASSERT_OK_AND_ASSIGN(std::shared_ptr
table, catalog->GetTable(identifier)); + ASSERT_EQ("db1.t1", table->Uuid()); + ASSERT_EQ(table->CatalogUuid(), std::nullopt); + + ASSERT_OK_AND_ASSIGN(bool committed, + catalog->CommitSnapshot(identifier, table->CatalogUuid(), std::nullopt, + BuildTestSnapshot(1), {})); + ASSERT_TRUE(committed); + std::string commit_body; + { + std::lock_guard lock(state_->mutex); + commit_body = state_->last_commit_body; + } + ASSERT_OK_AND_ASSIGN(CommitTableRequest sent, CommitTableRequest::FromJsonString(commit_body)); + ASSERT_EQ(sent.GetTableId(), std::nullopt); + ASSERT_NE(commit_body.find("\"tableId\": null"), std::string::npos) << commit_body; + + { + std::lock_guard lock(state_->mutex); + state_->databases["db1"]["t1"].id = "server-side-id"; + } + ASSERT_OK_AND_ASSIGN(std::shared_ptr
identified, catalog->GetTable(identifier)); + ASSERT_EQ(identified->CatalogUuid(), std::optional("server-side-id")); + ASSERT_EQ("server-side-id", identified->Uuid()); +} + TEST_F(RestCatalogTest, ClientAndServerHeadersAreSent) { ASSERT_OK_AND_ASSIGN(std::unique_ptr catalog, CreateRestCatalog()); ASSERT_OK_AND_ASSIGN(std::vector databases, catalog->ListDatabases()); @@ -606,9 +762,19 @@ TEST_F(RestCatalogTest, SystemTableSchema) { catalog->LoadTableSchema(system_identifier)); ASSERT_EQ((std::vector{"key", "value"}), schema->FieldNames()); + int32_t requests_before = 0; + { + std::lock_guard lock(state_->mutex); + requests_before = state_->get_table_requests; + } ASSERT_OK_AND_ASSIGN(std::shared_ptr
table, catalog->GetTable(system_identifier)); ASSERT_EQ("t1$options", table->Name()); ASSERT_EQ((std::vector{"key", "value"}), table->LatestSchema()->FieldNames()); + ASSERT_EQ("db1.t1$options", table->Uuid()); + { + std::lock_guard lock(state_->mutex); + ASSERT_EQ(requests_before + 1, state_->get_table_requests); + } Status unsupported = catalog->LoadTableSchema(Identifier("db1", "t1$unsupported")).status(); ASSERT_TRUE(unsupported.IsNotExist()) << unsupported.ToString(); @@ -812,8 +978,6 @@ TEST_F(RestCatalogTest, FormatTableIsLoadedInOneRequest) { // directory named "schema" there is a partition value, not metadata to skip. ASSERT_FALSE(table->LocationCarriesPaimonMetadata()); - // And the other way round: a `Table` promises snapshots and manifests a format table never - // had, so it is refused there in the same words the file system catalog uses. ASSERT_NOK_WITH_MSG(catalog->GetTable(Identifier("db1", "fmt")), "Cannot open format table"); // A managed table stays out of this path, and so does a system table. @@ -1125,6 +1289,356 @@ TEST_F(RestCatalogTest, ListSnapshots) { "branch table"); } +TEST_F(RestCatalogTest, FileStoreCommitIsBuiltFromTheCatalog) { + std::unique_ptr dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + MockCatalogState::TableData table_data; + table_data.schema_json = + R"({"fields": [{"id": 0, "name": "f0", "type": "INT NOT NULL"}],)" + R"( "partitionKeys": [], "primaryKeys": [],)" + R"( "options": {"file.format": "mock_format", "manifest.format": "avro"}})"; + table_data.path = dir->Str(); + ASSERT_OK_AND_ASSIGN(std::unique_ptr rest_catalog, CreateRestCatalog()); + ASSERT_OK(rest_catalog->CreateDatabase("db1", {}, false)); + { + std::lock_guard lock(state_->mutex); + state_->databases["db1"]["t1"] = table_data; + } + ASSERT_TRUE(rest_catalog->SupportsVersionManagement()); + + int32_t requests_before = 0; + { + std::lock_guard lock(state_->mutex); + requests_before = state_->get_table_requests; + } + std::shared_ptr shared_catalog(std::move(rest_catalog)); + CommitContextBuilder builder(dir->Str(), "commit-user"); + ASSERT_OK_AND_ASSIGN(std::unique_ptr context, + builder.WithCatalog(shared_catalog, Identifier("db1", "t1")).Finish()); + ASSERT_OK(FileStoreCommit::Create(std::move(context))); + { + std::lock_guard lock(state_->mutex); + ASSERT_GT(state_->get_table_requests, requests_before); + } +} + +TEST_F(RestCatalogTest, CommitSnapshot) { + ASSERT_OK_AND_ASSIGN(std::unique_ptr catalog, CreateRestCatalog()); + ASSERT_OK(catalog->CreateDatabase("db1", {}, false)); + Identifier identifier("db1", "t1"); + ASSERT_OK(CreateSampleTable(catalog.get(), identifier)); + + ASSERT_TRUE(catalog->SupportsVersionManagement()); + + Snapshot snapshot = BuildTestSnapshot(2); + std::vector statistics = { + PartitionStatistics({{"dt", "20240101"}}, 1, 541, 1, 1724090888743, -1)}; + + ASSERT_OK_AND_ASSIGN(bool success, + catalog->CommitSnapshot(identifier, "table-uuid", "base-snapshot-uuid", + snapshot, statistics)); + ASSERT_TRUE(success); + + std::string commit_body; + std::string commit_table; + { + std::lock_guard lock(state_->mutex); + commit_body = state_->last_commit_body; + commit_table = state_->last_commit_table; + } + ASSERT_EQ("t1", commit_table); + ASSERT_OK_AND_ASSIGN(CommitTableRequest request, + CommitTableRequest::FromJsonString(commit_body)); + ASSERT_EQ(request.GetTableId(), std::optional("table-uuid")); + ASSERT_EQ(request.GetBaseSnapshotUuid(), std::optional("base-snapshot-uuid")); + ASSERT_EQ(request.GetSnapshot().Id(), 2); + ASSERT_EQ(request.GetStatistics(), statistics); + + MockCatalogState::TableData branch_data; + branch_data.schema_json = R"({"fields": [{"id": 0, "name": "f0", "type": "INT NOT NULL"}],)" + R"( "partitionKeys": [], "primaryKeys": [], "options": {}})"; + branch_data.path = "wh1/db1.db/t1"; + { + std::lock_guard lock(state_->mutex); + state_->databases["db1"]["t1$branch_b1"] = branch_data; + state_->last_commit_table.clear(); + state_->last_commit_body.clear(); + } + ASSERT_OK_AND_ASSIGN(bool branch_success, + catalog->CommitSnapshot(Identifier("db1", "t1$branch_b1"), std::nullopt, + std::nullopt, snapshot, statistics)); + ASSERT_TRUE(branch_success); + { + std::lock_guard lock(state_->mutex); + ASSERT_EQ("t1$branch_b1", state_->last_commit_table); + } + + { + std::lock_guard lock(state_->mutex); + state_->last_commit_table.clear(); + } + ASSERT_OK_AND_ASSIGN(bool main_success, + catalog->CommitSnapshot(Identifier("db1", "t1$branch_main"), std::nullopt, + std::nullopt, snapshot, statistics)); + ASSERT_TRUE(main_success); + { + std::lock_guard lock(state_->mutex); + ASSERT_EQ("t1", state_->last_commit_table); + } +} + +TEST_F(RestCatalogTest, LoadSnapshot) { + ASSERT_OK_AND_ASSIGN(std::unique_ptr catalog, CreateRestCatalog()); + ASSERT_OK(catalog->CreateDatabase("db1", {}, false)); + Identifier identifier("db1", "t1"); + ASSERT_OK(CreateSampleTable(catalog.get(), identifier)); + + ASSERT_OK_AND_ASSIGN(std::optional empty, catalog->LoadSnapshot(identifier)); + ASSERT_FALSE(empty.has_value()); + + { + std::lock_guard lock(state_->mutex); + state_->null_snapshot_response = true; + } + ASSERT_OK_AND_ASSIGN(empty, catalog->LoadSnapshot(identifier)); + ASSERT_FALSE(empty.has_value()); + + { + std::lock_guard lock(state_->mutex); + state_->null_snapshot_response = false; + state_->current_snapshot = SnapshotJson(7); + } + ASSERT_OK_AND_ASSIGN(std::optional loaded, catalog->LoadSnapshot(identifier)); + ASSERT_TRUE(loaded.has_value()); + ASSERT_EQ(loaded.value().Id(), 7); + ASSERT_EQ(loaded.value().BaseManifestList(), "bml"); + { + std::lock_guard lock(state_->mutex); + ASSERT_EQ("t1", state_->last_snapshot_table); + } + + MockCatalogState::TableData branch_data; + branch_data.schema_json = R"({"fields": [{"id": 0, "name": "f0", "type": "INT NOT NULL"}],)" + R"( "partitionKeys": [], "primaryKeys": [], "options": {}})"; + branch_data.path = "wh1/db1.db/t1"; + { + std::lock_guard lock(state_->mutex); + state_->databases["db1"]["t1$branch_b1"] = branch_data; + } + ASSERT_OK_AND_ASSIGN(std::optional branch_snapshot, + catalog->LoadSnapshot(Identifier("db1", "t1$branch_b1"))); + ASSERT_TRUE(branch_snapshot.has_value()); + { + std::lock_guard lock(state_->mutex); + ASSERT_EQ("t1$branch_b1", state_->last_snapshot_table); + } + ASSERT_OK_AND_ASSIGN(std::optional main_snapshot, + catalog->LoadSnapshot(Identifier("db1", "t1$branch_main"))); + ASSERT_TRUE(main_snapshot.has_value()); + { + std::lock_guard lock(state_->mutex); + ASSERT_EQ("t1", state_->last_snapshot_table); + } + + Status missing = catalog->LoadSnapshot(Identifier("db1", "t9")).status(); + ASSERT_TRUE(missing.IsNotExist()) << missing.ToString(); + + ASSERT_NOK_WITH_MSG(catalog->LoadSnapshot(Identifier("db1", "t1$snapshots")), + "Cannot 'loadSnapshot' for system table"); +} + +TEST_F(RestCatalogTest, CommitSnapshotErrors) { + ASSERT_OK_AND_ASSIGN(std::unique_ptr catalog, CreateRestCatalog()); + ASSERT_OK(catalog->CreateDatabase("db1", {}, false)); + Identifier identifier("db1", "t1"); + ASSERT_OK(CreateSampleTable(catalog.get(), identifier)); + + Snapshot snapshot = BuildTestSnapshot(2); + std::vector statistics; + + { + std::lock_guard lock(state_->mutex); + state_->refuse_commit = true; + } + ASSERT_OK_AND_ASSIGN(bool refused, + catalog->CommitSnapshot(identifier, "table-uuid", "base-snapshot-uuid", + snapshot, statistics)); + ASSERT_FALSE(refused); + { + std::lock_guard lock(state_->mutex); + state_->refuse_commit = false; + } + + Status missing = catalog + ->CommitSnapshot(Identifier("db1", "t9"), std::nullopt, std::nullopt, + snapshot, statistics) + .status(); + ASSERT_TRUE(missing.IsNotExist()) << missing.ToString(); + + { + std::lock_guard lock(state_->mutex); + state_->break_commit_response = true; + } + ASSERT_NOK_WITH_MSG( + catalog->CommitSnapshot(identifier, "table-uuid", std::nullopt, snapshot, statistics), + "failed to deserialize the response"); + { + std::lock_guard lock(state_->mutex); + state_->break_commit_response = false; + } + + { + std::lock_guard lock(state_->mutex); + state_->commit_response_without_outcome = true; + } + ASSERT_NOK_WITH_MSG( + catalog->CommitSnapshot(identifier, "table-uuid", std::nullopt, snapshot, statistics), + "failed to deserialize the response"); + { + std::lock_guard lock(state_->mutex); + state_->commit_response_without_outcome = false; + } + + ASSERT_NOK_WITH_MSG(catalog->CommitSnapshot(Identifier("db1", "t1$snapshots"), std::nullopt, + std::nullopt, snapshot, statistics), + "Cannot 'commitSnapshot' for system table"); +} + +namespace { +Result>> ReadFixtureCommitMessages( + const std::shared_ptr& fs) { + std::string bytes; + PAIMON_RETURN_NOT_OK( + fs->ReadFile(paimon::test::GetDataDir() + + "/orc/append_09.db/append_09/commit_messages/commit_messages-01", + &bytes)); + return CommitMessage::DeserializeList(3, bytes.data(), bytes.size(), GetDefaultPool()); +} +} // namespace + +class RestCatalogCommitRecoveryTest : public RestCatalogTest, + public ::testing::WithParamInterface {}; + +TEST_P(RestCatalogCommitRecoveryTest, CommitAcceptedThenLostRecoversAfterRestart) { + std::unique_ptr dir = UniqueTestDirectory::Create(); + ASSERT_TRUE(dir); + MockCatalogState::TableData table_data; + table_data.schema_json = R"({"fields": [{"id": 0, "name": "f0", "type": "STRING"},)" + R"( {"id": 1, "name": "f1", "type": "INT"},)" + R"( {"id": 2, "name": "f2", "type": "INT"},)" + R"( {"id": 3, "name": "f3", "type": "DOUBLE"}],)" + R"( "partitionKeys": ["f1"], "primaryKeys": [],)" + R"( "options": {"file.format": "orc", "manifest.format": "avro"}})"; + table_data.path = dir->Str(); + ASSERT_OK_AND_ASSIGN(std::unique_ptr rest_catalog, + CreateRestCatalog(FastRetryConfig())); + ASSERT_OK(rest_catalog->CreateDatabase("db1", {}, false)); + { + std::lock_guard lock(state_->mutex); + state_->databases["db1"]["t1"] = table_data; + state_->accept_commit_then_report_unavailable = true; + state_->accepted_commit_response_code = GetParam(); + state_->commit_requests = 0; + } + Identifier identifier("db1", "t1"); + RestCatalog* raw_catalog = rest_catalog.get(); + std::shared_ptr catalog(std::move(rest_catalog)); + + auto create_commit = [&]() -> Result> { + CommitContextBuilder builder(dir->Str(), "commit-user"); + PAIMON_ASSIGN_OR_RAISE(std::unique_ptr context, + builder.WithCatalog(catalog, identifier).Finish()); + return FileStoreCommit::Create(std::move(context)); + }; + ASSERT_OK_AND_ASSIGN(std::unique_ptr commit, create_commit()); + + ASSERT_OK_AND_ASSIGN(std::vector> msgs, + ReadFixtureCommitMessages(dir->GetFileSystem())); + ASSERT_GT(msgs.size(), 0u); + ASSERT_NOK_WITH_MSG(commit->Commit(msgs, 7), "call FilterAndCommit"); + { + std::lock_guard lock(state_->mutex); + ASSERT_EQ(1, state_->commit_requests); + } + + ASSERT_OK_AND_ASSIGN(std::optional held, raw_catalog->LoadSnapshot(identifier)); + ASSERT_TRUE(held.has_value()); + ASSERT_EQ(held.value().Id(), 1); + ASSERT_EQ(held.value().CommitIdentifier(), 7); + ASSERT_EQ(held.value().CommitUser(), "commit-user"); + for (const std::string& manifest_list : + {held.value().BaseManifestList(), held.value().DeltaManifestList()}) { + ASSERT_OK_AND_ASSIGN(bool exist, dir->GetFileSystem()->Exists(PathUtil::JoinPath( + dir->Str(), "manifest/" + manifest_list))); + ASSERT_TRUE(exist) << manifest_list; + } + + std::map>> inputs; + inputs[7] = msgs; + commit.reset(); + ASSERT_OK_AND_ASSIGN(commit, create_commit()); + ASSERT_OK_AND_ASSIGN(int32_t again, commit->FilterAndCommit(inputs, 10)); + ASSERT_EQ(0, again); + ASSERT_OK_AND_ASSIGN(std::optional recovered, raw_catalog->LoadSnapshot(identifier)); + ASSERT_EQ(recovered, held); + { + std::lock_guard lock(state_->mutex); + ASSERT_EQ(1, state_->commit_requests); + state_->accept_commit_then_report_unavailable = false; + } +} + +INSTANTIATE_TEST_SUITE_P(UncertainResponses, RestCatalogCommitRecoveryTest, + ::testing::Values(200, 204, 307, 429, 503), + [](const ::testing::TestParamInfo& info) { + return fmt::format("Http{}", info.param); + }); + +TEST_F(RestCatalogTest, CommitTakenThenReportedUnavailableIsNotReplayed) { + ASSERT_OK_AND_ASSIGN(std::unique_ptr catalog, + CreateRestCatalog(FastRetryConfig())); + ASSERT_OK(catalog->CreateDatabase("db1", {}, false)); + Identifier identifier("db1", "t1"); + ASSERT_OK(CreateSampleTable(catalog.get(), identifier)); + + { + std::lock_guard lock(state_->mutex); + state_->accept_commit_then_report_unavailable = true; + state_->commit_requests = 0; + } + const Snapshot taken = BuildTestSnapshot(2, "snapshot-uuid-2"); + Status unavailable = + catalog->CommitSnapshot(identifier, "table-uuid", "base-snapshot-uuid", taken, {}).status(); + ASSERT_FALSE(unavailable.ok()) << "a 503 must not read as a refusal"; + ASSERT_TRUE(unavailable.IsIOError()) << unavailable.ToString(); + { + std::lock_guard lock(state_->mutex); + ASSERT_EQ(1, state_->commit_requests); + } + + ASSERT_OK_AND_ASSIGN(std::optional held, catalog->LoadSnapshot(identifier)); + ASSERT_TRUE(held.has_value()); + ASSERT_EQ(held.value().Id(), 2); + ASSERT_EQ(held.value().Uuid(), std::optional("snapshot-uuid-2")); + + ASSERT_OK_AND_ASSIGN(bool replayed, catalog->CommitSnapshot(identifier, "table-uuid", + "base-snapshot-uuid", taken, {})); + ASSERT_FALSE(replayed); + + ASSERT_OK_AND_ASSIGN(bool rebased, + catalog->CommitSnapshot(identifier, "table-uuid", "snapshot-uuid-2", + BuildTestSnapshot(3, "snapshot-uuid-3"), {})); + ASSERT_TRUE(rebased); + ASSERT_OK_AND_ASSIGN(std::optional now_held, catalog->LoadSnapshot(identifier)); + ASSERT_TRUE(now_held.has_value()); + ASSERT_EQ(now_held.value().Id(), 3); + { + std::lock_guard lock(state_->mutex); + ASSERT_EQ(3, state_->commit_requests); + state_->accept_commit_then_report_unavailable = false; + } +} + TEST(RestApiErrorTest, ErrorToStatus) { RestHttpClient::Response response; response.code = 404; @@ -1134,6 +1648,24 @@ TEST(RestApiErrorTest, ErrorToStatus) { ASSERT_TRUE(status.IsNotExist()); ASSERT_TRUE(status.ToString().find("requestId:req-123") != std::string::npos) << status.ToString(); + ASSERT_NE(status.detail(), nullptr); + ASSERT_STREQ(status.detail()->type_id(), RestErrorDetail::kTypeId); + ASSERT_EQ(static_cast(status.detail().get())->GetResourceType(), + ErrorResponse::kResourceTypeTable); + + response.body = R"({"message": "no snapshot", "resourceType": "SNAPSHOT",)" + R"( "resourceName": "t1"})"; + Status snapshot_status = RestApi::ErrorToStatus(response); + ASSERT_TRUE(snapshot_status.IsNotExist()); + ASSERT_EQ( + static_cast(snapshot_status.detail().get())->GetResourceType(), + ErrorResponse::kResourceTypeSnapshot); + + response.body = R"({"message": "boom"})"; + Status plain = RestApi::ErrorToStatus(response); + ASSERT_TRUE( + static_cast(plain.detail().get())->GetResourceType().empty()); + response.body = R"({"message": "no table", "resourceType": "TABLE", "resourceName": "t1"})"; response.code = 409; Status exist_status = RestApi::ErrorToStatus(response); diff --git a/src/paimon/rest/rest_http_client.cpp b/src/paimon/rest/rest_http_client.cpp index f4964bf57..e78c62fee 100644 --- a/src/paimon/rest/rest_http_client.cpp +++ b/src/paimon/rest/rest_http_client.cpp @@ -404,7 +404,7 @@ Result RestHttpClient::Execute( const std::string& method, const std::string& path, const std::map& query_params, const std::map& headers, const std::string& body, - bool follow_redirects) const { + bool follow_redirects, bool retry_safe) const { if (method != "GET" && method != "POST" && method != "DELETE") { return Status::Invalid(fmt::format("unsupported http method: {}", method)); } @@ -423,6 +423,8 @@ Result RestHttpClient::Execute( url += "?" + BuildQueryString(query_params); } bool idempotent = IsIdempotent(method); + // Redirects can replay requests inside ExecuteOnce(). + follow_redirects = follow_redirects && retry_safe; std::chrono::steady_clock::time_point start = std::chrono::steady_clock::now(); int32_t execution_count = 0; while (true) { @@ -436,6 +438,7 @@ Result RestHttpClient::Execute( } else { retriable = idempotent && transport_retriable; } + retriable = retriable && retry_safe; int64_t elapsed_ms = std::chrono::duration_cast( std::chrono::steady_clock::now() - start) .count(); diff --git a/src/paimon/rest/rest_http_client.h b/src/paimon/rest/rest_http_client.h index 3ebd89472..60bbb0d0f 100644 --- a/src/paimon/rest/rest_http_client.h +++ b/src/paimon/rest/rest_http_client.h @@ -100,10 +100,14 @@ class RestHttpClient { /// established connection breaking mid-request or a truncated response body) are /// retried; every other transport failure fails immediately. `follow_redirects` /// must be false when authentication headers are bound to the original request. + /// + /// `retry_safe=false` disables retries and redirects, overriding `follow_redirects`, + /// to prevent replay when the original request's outcome is unknown. Result Execute(const std::string& method, const std::string& path, const std::map& query_params, const std::map& headers, - const std::string& body, bool follow_redirects = true) const; + const std::string& body, bool follow_redirects = true, + bool retry_safe = true) const; const std::string& GetBaseUri() const { return base_uri_; diff --git a/src/paimon/rest/rest_http_client_test.cpp b/src/paimon/rest/rest_http_client_test.cpp index 9b93705a2..d03005f5c 100644 --- a/src/paimon/rest/rest_http_client_test.cpp +++ b/src/paimon/rest/rest_http_client_test.cpp @@ -140,6 +140,53 @@ TEST(RestHttpClientTest, ServiceUnavailableIsRetried) { ASSERT_EQ(3, request_count.load()); } +TEST(RestHttpClientTest, NonRetrySafeRequestDoesNotRetryTransientResponses) { + for (int32_t code : {429, 503}) { + SCOPED_TRACE(code); + std::atomic request_count{0}; + ASSERT_OK_AND_ASSIGN(std::unique_ptr server, + MockRestServer::Start([&](const MockRestServer::Request& request) { + request_count++; + MockRestServer::Response response; + response.code = code; + return response; + })); + ASSERT_OK_AND_ASSIGN(std::unique_ptr client, + RestHttpClient::Create(server->GetBaseUri(), FastRetryConfig(5))); + ASSERT_OK_AND_ASSIGN(RestHttpClient::Response response, + client->Execute("POST", "/v1/databases/db1/tables/t1/commit", {}, {}, + "{}", true, false)); + ASSERT_EQ(code, response.code); + ASSERT_EQ(1, request_count.load()); + } +} + +TEST(RestHttpClientTest, NonRetrySafeRequestDoesNotFollowRedirects) { + for (int32_t redirect_code : {301, 302, 303, 307, 308}) { + SCOPED_TRACE(redirect_code); + std::atomic request_count{0}; + ASSERT_OK_AND_ASSIGN(std::unique_ptr server, + MockRestServer::Start([&](const MockRestServer::Request& request) { + request_count++; + MockRestServer::Response response; + if (request.path == "/commit") { + response.code = redirect_code; + response.headers["Location"] = "/redirected-commit"; + } else { + response.body = R"({"success": false})"; + } + return response; + })); + ASSERT_OK_AND_ASSIGN(std::unique_ptr client, + RestHttpClient::Create(server->GetBaseUri(), FastRetryConfig(5))); + ASSERT_OK_AND_ASSIGN( + RestHttpClient::Response response, + client->Execute("POST", "/commit", {}, {}, R"({"tableId": "t1"})", true, false)); + ASSERT_EQ(response.code, redirect_code); + ASSERT_EQ(request_count.load(), 1); + } +} + TEST(RestHttpClientTest, RetryAfterHeaderIsHonored) { std::atomic request_count{0}; ASSERT_OK_AND_ASSIGN(std::unique_ptr server, diff --git a/src/paimon/rest/rest_messages.cpp b/src/paimon/rest/rest_messages.cpp index 590e58c48..6cc2041f3 100644 --- a/src/paimon/rest/rest_messages.cpp +++ b/src/paimon/rest/rest_messages.cpp @@ -54,6 +54,8 @@ constexpr const char kFieldDatabase[] = "database"; constexpr const char kFieldObject[] = "object"; constexpr const char kFieldSource[] = "source"; constexpr const char kFieldDestination[] = "destination"; +constexpr const char kFieldSuccess[] = "success"; +constexpr const char kFieldSnapshot[] = "snapshot"; void AddOptionalStringMember(rapidjson::Value* obj, const char* key, const std::optional& value, @@ -390,4 +392,45 @@ void RenameTableRequest::FromJson(const rapidjson::Value& obj) noexcept(false) { DeserializeIdentifier(obj, kFieldDestination, &destination_database_, &destination_table_); } +rapidjson::Value GetTableSnapshotResponse::ToJson( + rapidjson::Document::AllocatorType* allocator) const noexcept(false) { + rapidjson::Value wrapper(rapidjson::kNullType); + if (snapshot_) { + wrapper.SetObject(); + wrapper.AddMember(rapidjson::StringRef(kFieldSnapshot), + RapidJsonUtil::SerializeValue(snapshot_.value(), allocator).Move(), + *allocator); + } + rapidjson::Value obj(rapidjson::kObjectType); + obj.AddMember(rapidjson::StringRef(kFieldSnapshot), wrapper.Move(), *allocator); + return obj; +} + +void GetTableSnapshotResponse::FromJson(const rapidjson::Value& obj) noexcept(false) { + if (!obj.IsObject() || !obj.HasMember(kFieldSnapshot) || + (!obj[kFieldSnapshot].IsObject() && !obj[kFieldSnapshot].IsNull())) { + throw std::invalid_argument(std::string("member '") + kFieldSnapshot + + "' must exist and be an object or null"); + } + if (obj[kFieldSnapshot].IsNull()) { + snapshot_.reset(); + return; + } + snapshot_ = RapidJsonUtil::DeserializeKeyValue(obj[kFieldSnapshot], kFieldSnapshot); +} + +rapidjson::Value CommitTableResponse::ToJson(rapidjson::Document::AllocatorType* allocator) const + noexcept(false) { + rapidjson::Value obj(rapidjson::kObjectType); + obj.AddMember(rapidjson::StringRef(kFieldSuccess), + RapidJsonUtil::SerializeValue(success_, allocator).Move(), *allocator); + return obj; +} + +void CommitTableResponse::FromJson(const rapidjson::Value& obj) noexcept(false) { + // Require an explicit boolean: treating an unknown outcome as false could delete + // manifests belonging to an accepted commit. + success_ = RapidJsonUtil::DeserializeKeyValue(obj, kFieldSuccess); +} + } // namespace paimon diff --git a/src/paimon/rest/rest_messages.h b/src/paimon/rest/rest_messages.h index 0244fa362..bc3e909b0 100644 --- a/src/paimon/rest/rest_messages.h +++ b/src/paimon/rest/rest_messages.h @@ -41,6 +41,7 @@ class ErrorResponse : public Jsonizable { public: static constexpr const char* kResourceTypeDatabase = "DATABASE"; static constexpr const char* kResourceTypeTable = "TABLE"; + static constexpr const char* kResourceTypeSnapshot = "SNAPSHOT"; ErrorResponse(const std::string& resource_type, const std::string& resource_name, const std::string& message, int32_t code) @@ -388,4 +389,44 @@ class RenameTableRequest : public Jsonizable { std::string destination_table_; }; +/// Reads the current snapshot from `snapshot.snapshot`, ignoring aggregate totals. +/// An outer null `snapshot` means the table has no snapshot yet. +class GetTableSnapshotResponse : public Jsonizable { + public: + explicit GetTableSnapshotResponse(const std::optional& snapshot) + : snapshot_(snapshot) {} + + rapidjson::Value ToJson(rapidjson::Document::AllocatorType* allocator) const + noexcept(false) override; + void FromJson(const rapidjson::Value& obj) noexcept(false) override; + + const std::optional& GetSnapshot() const { + return snapshot_; + } + + GetTableSnapshotResponse() = default; + + private: + std::optional snapshot_; +}; + +/// Commit outcome: `success=false` indicates a concurrent commit conflict. +class CommitTableResponse : public Jsonizable { + public: + explicit CommitTableResponse(bool success) : success_(success) {} + + rapidjson::Value ToJson(rapidjson::Document::AllocatorType* allocator) const + noexcept(false) override; + void FromJson(const rapidjson::Value& obj) noexcept(false) override; + + bool IsSuccess() const { + return success_; + } + + CommitTableResponse() = default; + + private: + bool success_ = false; +}; + } // namespace paimon diff --git a/src/paimon/rest/rest_messages_test.cpp b/src/paimon/rest/rest_messages_test.cpp index 28b2f317b..53fbd5a5f 100644 --- a/src/paimon/rest/rest_messages_test.cpp +++ b/src/paimon/rest/rest_messages_test.cpp @@ -267,6 +267,67 @@ TEST(RestMessagesTest, RenameTableRequestSerialize) { ASSERT_EQ("t2", parsed.GetDestinationTable()); } +TEST(RestMessagesTest, GetTableSnapshotResponseRoundTrip) { + std::string json = R"({"snapshot": {"snapshot": {"version": 3, "id": 7, "schemaId": 0,)" + R"( "baseManifestList": "bml", "deltaManifestList": "dml",)" + R"( "commitUser": "u", "commitIdentifier": 1, "commitKind": "APPEND",)" + R"( "timeMillis": 100, "totalRecordCount": 10, "deltaRecordCount": 1},)" + R"( "recordCount": 10, "fileSizeInBytes": 20, "fileCount": 3,)" + R"( "lastFileCreationTime": 4}})"; + ASSERT_OK_AND_ASSIGN(GetTableSnapshotResponse parsed, + GetTableSnapshotResponse::FromJsonString(json)); + ASSERT_TRUE(parsed.GetSnapshot()); + ASSERT_EQ(parsed.GetSnapshot()->Id(), 7); + ASSERT_EQ(parsed.GetSnapshot()->BaseManifestList(), "bml"); + + ASSERT_OK_AND_ASSIGN(std::string written, parsed.ToJsonString()); + ASSERT_OK_AND_ASSIGN(GetTableSnapshotResponse round_tripped, + GetTableSnapshotResponse::FromJsonString(written)); + ASSERT_EQ(round_tripped.GetSnapshot(), parsed.GetSnapshot()); + + ASSERT_OK(RapidJsonUtil::FromJsonString(R"({"snapshot": null})", &parsed)); + ASSERT_FALSE(parsed.GetSnapshot()); + ASSERT_OK_AND_ASSIGN(std::string empty_json, parsed.ToJsonString()); + ASSERT_OK_AND_ASSIGN(GetTableSnapshotResponse empty_round_trip, + GetTableSnapshotResponse::FromJsonString(empty_json)); + ASSERT_FALSE(empty_round_trip.GetSnapshot()); + + ASSERT_NOK_WITH_MSG(GetTableSnapshotResponse::FromJsonString("{}"), + "must exist and be an object"); + ASSERT_NOK_WITH_MSG(GetTableSnapshotResponse::FromJsonString(R"({"snapshot": {}})"), + "key must exist"); + ASSERT_NOK_WITH_MSG( + GetTableSnapshotResponse::FromJsonString(R"({"snapshot": {"snapshot": null}})"), + "key must exist"); + ASSERT_NOK_WITH_MSG(GetTableSnapshotResponse::FromJsonString(R"({"snapshot": 7})"), + "must exist and be an object or null"); + ASSERT_NOK_WITH_MSG(GetTableSnapshotResponse::FromJsonString("[]"), + "must exist and be an object"); +} + +TEST(RestMessagesTest, CommitTableResponseRoundTrip) { + CommitTableResponse response(true); + ASSERT_OK_AND_ASSIGN(std::string json, response.ToJsonString()); + ASSERT_OK_AND_ASSIGN(CommitTableResponse parsed, CommitTableResponse::FromJsonString(json)); + ASSERT_TRUE(parsed.IsSuccess()); + + ASSERT_OK_AND_ASSIGN(CommitTableResponse refused, + CommitTableResponse::FromJsonString(R"({"success": false})")); + ASSERT_FALSE(refused.IsSuccess()); + + ASSERT_NOK_WITH_MSG(CommitTableResponse::FromJsonString("{}"), "key must exist"); + ASSERT_NOK_WITH_MSG(CommitTableResponse::FromJsonString(R"({"success": null})"), + "key must exist"); + ASSERT_NOK_WITH_MSG(CommitTableResponse::FromJsonString(R"({"success": "true"})"), + "value must be bool"); + ASSERT_NOK_WITH_MSG(CommitTableResponse::FromJsonString(R"({"success": 1})"), + "value must be bool"); + ASSERT_NOK_WITH_MSG(CommitTableResponse::FromJsonString("[]"), "value must be an object"); + ASSERT_NOK_WITH_MSG(CommitTableResponse::FromJsonString("null"), "value must be an object"); + ASSERT_NOK_WITH_MSG(CommitTableResponse::FromJsonString("true"), "value must be an object"); + ASSERT_NOK(CommitTableResponse::FromJsonString("not json")); +} + TEST(RestMessagesTest, CreateDatabaseRequestRoundTrip) { CreateDatabaseRequest request("db1", {{"k1", "v1"}}); ASSERT_OK_AND_ASSIGN(std::string json, request.ToJsonString()); diff --git a/src/paimon/testing/mock/mock_catalog.h b/src/paimon/testing/mock/mock_catalog.h new file mode 100644 index 000000000..0931ba799 --- /dev/null +++ b/src/paimon/testing/mock/mock_catalog.h @@ -0,0 +1,259 @@ +/* + * 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 +#include +#include +#include +#include +#include +#include +#include +#include + +#include "paimon/catalog/catalog.h" +#include "paimon/catalog/identifier.h" +#include "paimon/catalog/table.h" +#include "paimon/core/catalog/version_managed_catalog.h" +#include "paimon/core/partition/partition_statistics.h" +#include "paimon/core/snapshot.h" +#include "paimon/result.h" +#include "paimon/schema/schema.h" +#include "paimon/status.h" + +namespace paimon::test { + +class MockVersionManagedCatalog : public Catalog, public VersionManagedCatalog { + public: + struct CommitCall { + Identifier identifier{"", ""}; + std::optional table_uuid; + std::optional base_snapshot_uuid; + Snapshot snapshot; + std::vector statistics; + }; + + explicit MockVersionManagedCatalog(Result answer = true, + const std::string& table_uuid = "mock-table-uuid") + : answer_(std::move(answer)), table_uuid_(table_uuid) {} + + bool SupportsVersionManagement() const override { + return supports_version_management_; + } + + void SetSupportsVersionManagement(bool supports_version_management) { + supports_version_management_ = supports_version_management; + } + + Result CommitSnapshot(const Identifier& identifier, + const std::optional& table_uuid, + const std::optional& base_snapshot_uuid, + const Snapshot& snapshot, + const std::vector& statistics) override { + commit_calls_.push_back( + CommitCall{identifier, table_uuid, base_snapshot_uuid, snapshot, statistics}); + if (check_table_uuid_ && table_uuid && table_uuid.value() != table_uuid_) { + return Status::NotExist("no table with id " + table_uuid.value()); + } + Result answer = answer_; + if (check_base_snapshot_uuid_) { + answer = base_snapshot_uuid == current_snapshot_uuid_; + if (answer.value()) { + current_snapshot_uuid_ = snapshot.Uuid(); + accepted_.push_back(snapshot); + } + } + if (on_commit_) { + on_commit_(); + } + if (!lost_answer_.ok() && answer.ok() && answer.value()) { + return lost_answer_; + } + return answer; + } + + void SetLostAnswer(const Status& status) { + lost_answer_ = status; + } + + Result> GetTable(const Identifier& identifier) const override { + ++get_table_calls_; + return std::make_shared
(table_schema_, identifier.GetDatabaseName(), + identifier.GetTableName(), table_uuid_); + } + + void SetTableSchema(const std::shared_ptr& table_schema) { + table_schema_ = table_schema; + } + + Result> LoadTableSchema(const Identifier& identifier) const override { + ++load_table_schema_calls_; + if (table_schema_ == nullptr) { + return Status::NotExist(identifier.ToString() + " not exist"); + } + return table_schema_; + } + + size_t LoadTableSchemaCalls() const { + return load_table_schema_calls_; + } + + Result> LoadSnapshot(const Identifier& identifier) const override { + load_snapshot_identifiers_.push_back(identifier); + if (!serve_snapshots_) { + return Status::NotImplemented("this mock does not serve snapshots"); + } + if (!load_snapshot_status_.ok()) { + return load_snapshot_status_; + } + if (accepted_.empty()) { + return std::optional(); + } + return std::optional(accepted_.back()); + } + + void SetLoadSnapshotStatus(const Status& status) { + serve_snapshots_ = true; + load_snapshot_status_ = status; + } + + const std::vector& LoadSnapshotIdentifiers() const { + return load_snapshot_identifiers_; + } + + size_t LoadSnapshotCalls() const { + return load_snapshot_identifiers_.size(); + } + + void CheckTableUuid() { + check_table_uuid_ = true; + } + + void CheckBaseSnapshotUuid() { + check_base_snapshot_uuid_ = true; + serve_snapshots_ = true; + } + + const std::vector& AcceptedSnapshots() const { + return accepted_; + } + + void SetHeldSnapshot(const Snapshot& snapshot) { + serve_snapshots_ = true; + current_snapshot_uuid_ = snapshot.Uuid(); + accepted_.push_back(snapshot); + } + + void SetAnswer(Result answer) { + answer_ = std::move(answer); + } + + void SetOnCommit(std::function on_commit) { + on_commit_ = std::move(on_commit); + } + + const std::vector& CommitCalls() const { + return commit_calls_; + } + + size_t GetTableCalls() const { + return get_table_calls_; + } + + Status CreateDatabase(const std::string&, const std::map&, + bool) override { + return Unsupported(); + } + Status CreateTable(const Identifier&, ArrowSchema*, const std::vector&, + const std::vector&, const std::map&, + bool) override { + return Unsupported(); + } + Result> ListDatabases() const override { + return Unsupported(); + } + Result> ListTables(const std::string&) const override { + return Unsupported(); + } + Status DropDatabase(const std::string&, bool, bool) override { + return Unsupported(); + } + Status DropTable(const Identifier&, bool) override { + return Unsupported(); + } + Status RenameTable(const Identifier&, const Identifier&, bool) override { + return Unsupported(); + } + Result DatabaseExists(const std::string&) const override { + return Unsupported(); + } + Result TableExists(const Identifier&) const override { + return Unsupported(); + } + Result GetDatabaseLocation(const std::string&) const override { + return Unsupported(); + } + Result GetTableLocation(const Identifier&) const override { + return Unsupported(); + } + std::string GetRootPath() const override { + return ""; + } + void SetFileSystem(const std::shared_ptr& file_system) { + file_system_ = file_system; + } + + std::shared_ptr GetFileSystem() const override { + return file_system_; + } + const std::map& GetOptions() const override { + return options_; + } + Result> ListSnapshots(const Identifier&, + const std::string&) const override { + return Unsupported(); + } + + private: + static Status Unsupported() { + return Status::NotImplemented("not used by this mock"); + } + + Result answer_; + std::string table_uuid_; + std::shared_ptr table_schema_; + std::shared_ptr file_system_; + bool supports_version_management_ = true; + std::function on_commit_; + bool check_table_uuid_ = false; + bool check_base_snapshot_uuid_ = false; + Status lost_answer_; + std::optional current_snapshot_uuid_; + std::vector accepted_; + mutable size_t get_table_calls_ = 0; + mutable size_t load_table_schema_calls_ = 0; + mutable std::vector load_snapshot_identifiers_; + bool serve_snapshots_ = false; + Status load_snapshot_status_; + std::vector commit_calls_; + std::map options_; +}; + +} // namespace paimon::test diff --git a/src/paimon/testing/utils/snapshot_test_helper.h b/src/paimon/testing/utils/snapshot_test_helper.h new file mode 100644 index 000000000..871875cdc --- /dev/null +++ b/src/paimon/testing/utils/snapshot_test_helper.h @@ -0,0 +1,39 @@ +/* + * 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 +#include +#include + +#include "paimon/core/snapshot.h" + +namespace paimon::test { + +inline Snapshot BuildTestSnapshot(int64_t id, + const std::optional& uuid = std::nullopt) { + return Snapshot(Snapshot::CURRENT_VERSION, id, 0, + "manifest-list-3879e56f-2f27-49ae-a2f3-3dcbb8eb0beb-0", 291, + "manifest-list-3879e56f-2f27-49ae-a2f3-3dcbb8eb0beb-1", 1342, std::nullopt, + std::nullopt, std::nullopt, "commit_user_1", 9223372036854775807, + Snapshot::CommitKind::Append(), 1758097357597, 5, 5, 0, std::nullopt, + std::nullopt, std::nullopt, 0, uuid); +} + +} // namespace paimon::test