feat(rest): support rest catalog with database/table operations and snapshot listing - #441
feat(rest): support rest catalog with database/table operations and snapshot listing#441SteNicholas wants to merge 1 commit into
Conversation
8fda86d to
93edb7d
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a new REST-backed catalog implementation selectable via metastore=rest, including HTTP client + auth + REST API layers plus unit tests and build/CI wiring.
Changes:
- Introduces
HttpClient,RestApi,RestCatalog, auth provider, message models, and URL/path utilities for the REST catalog protocol. - Adds an in-process mock REST server and a new REST unit test binary with coverage across retry logic, message round-trips, and catalog operations.
- Extends catalog factory/options and updates CMake/CI to optionally build/link REST support (libcurl) and run REST tests.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/paimon/rest/rest_util.h | Declares REST helper utilities (URL encode/decode, JSON helpers, prefix extraction). |
| src/paimon/rest/rest_util.cpp | Implements URL encoding/decoding, prefix extraction, and rapidjson serialization helpers. |
| src/paimon/rest/rest_messages_test.cpp | Adds unit tests for REST utilities, paths, and JSON message round-trips. |
| src/paimon/rest/rest_messages.h | Adds REST protocol request/response models (JSON-serializable). |
| src/paimon/rest/rest_messages.cpp | Implements JSON serialization/deserialization and config merge behavior. |
| src/paimon/rest/rest_catalog_test.cpp | End-to-end tests for REST catalog behavior against a mock server. |
| src/paimon/rest/rest_catalog.h | Declares RestCatalog implementing database/table operations + snapshot listing. |
| src/paimon/rest/rest_catalog.cpp | Implements REST catalog behavior and schema conversion/highestFieldId logic. |
| src/paimon/rest/rest_auth.h | Declares REST auth provider interface and bearer-token provider. |
| src/paimon/rest/rest_auth.cpp | Implements bearer-token header injection and provider selection via options. |
| src/paimon/rest/rest_api.h | Declares REST API layer (HTTP+JSON) and error mapping to Status. |
| src/paimon/rest/rest_api.cpp | Implements config fetching/merging, request execution, pagination, and error mapping. |
| src/paimon/rest/resource_paths.h | Declares URL path builder for REST endpoints with optional prefix. |
| src/paimon/rest/resource_paths.cpp | Implements REST endpoint path construction with URL-encoded segments. |
| src/paimon/rest/mock_rest_server.h | Declares a minimal blocking HTTP server for REST unit tests. |
| src/paimon/rest/mock_rest_server.cpp | Implements the mock HTTP server (parsing, dispatch, responses). |
| src/paimon/rest/http_client_test.cpp | Adds unit tests for URI normalization, query encoding, and retry behavior. |
| src/paimon/rest/http_client.h | Declares blocking libcurl client with retry/backoff behavior. |
| src/paimon/rest/http_client.cpp | Implements libcurl execution, retry classification, backoff, and header parsing. |
| src/paimon/core/snapshot.h | Adds SnapshotInfo include and Snapshot::ToSnapshotInfo() declaration. |
| src/paimon/core/snapshot.cpp | Implements Snapshot::ToSnapshotInfo() commit-kind mapping and field transfer. |
| src/paimon/core/catalog/file_system_catalog.cpp | Refactors snapshot listing to reuse Snapshot::ToSnapshotInfo(). |
| src/paimon/core/catalog/catalog.cpp | Adds metastore dispatch for filesystem vs rest (guarded by PAIMON_ENABLE_REST). |
| src/paimon/common/defs.cpp | Defines new option keys: metastore, uri, token, token.provider. |
| src/paimon/CMakeLists.txt | Adds REST sources/linking behind PAIMON_ENABLE_REST and registers REST tests. |
| include/paimon/defs.h | Exposes new public options for selecting/configuring the REST catalog. |
| ci/scripts/build_paimon.sh | Forces -DPAIMON_ENABLE_REST=ON in CI build script. |
| CMakeLists.txt | Adds PAIMON_ENABLE_REST option, finds libcurl, and defines compile macro. |
| .github/workflows/gcc8_test.yaml | Installs libcurl dev package for GCC8 CI. |
| .github/workflows/build_and_test.yaml | Installs libcurl dev package for general CI builds/tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
47960b5 to
9123abd
Compare
9f3b590 to
006176f
Compare
0ffb7ae to
9621d93
Compare
zjw1111
left a comment
There was a problem hiding this comment.
Thanks for putting this together — the alignment with the Java implementation is impressively thorough (config merge precedence, retry classification, isSuccessful status set, paging termination, audit option keys all match).
One small consistency point on snapshot listing, left inline.
…napshot listing Add a REST catalog implementation selected via the "metastore=rest" option: - RestHttpClient: blocking libcurl client with exponential-backoff retries (429/503 retried for all methods, transport errors only for idempotent ones, honoring a Retry-After header in both the delta-seconds and the HTTP-date form when it yields a positive delay, with a locale-independent date parse; a server closing the connection without responding is not retried) and per-request debug logging. - RestApi: HTTP + JSON layer with "/v1/config" option merging (overrides > client options > defaults), "header." options as request headers, paged listing and error-to-Status mapping that prefers the code of the parsed error body over the http status, redacts sensitive server messages, never echoes response bodies into error messages (they may carry credentials), carries the request id (x-request-id, falling back to any request-id header) and attaches a RestErrorDetail with the mapped code so callers can distinguish e.g. authentication failures programmatically. - RestCatalog: database/table operations and snapshot listing, "table-default." option defaults, system/branch table checks and schema conversion with the computed highestFieldId, rejecting duplicated field ids at any nesting level. Branch identifiers are passed through to the server so it resolves the branch and returns the branch's own schema (the default branch "main" resolves to the bare table), snapshots of a branch are listed under the branch object name, and the "sys" database serves the local global system tables like FileSystemCatalog. - Bear token authentication provider. - CatalogOptions: catalog-level option keys (metastore, uri, token, token.provider) and the "table-default." option prefix in a dedicated public header, keeping them separate from the table-level CoreOptions. - CatalogUtils: system database and system/branch table checks shared by FileSystemCatalog and RestCatalog; FileSystemCatalog's create/drop/rename checks now use them instead of inline messages. - SpecialFieldIds::SYSTEM_FIELD_ID_START names the reserved system field id bound, which is excluded from the highest field id. The pieces overlapping with the object store file systems are unified under common/utils: UrlUtils carries both URL encoding flavors (form-urlencoded for the REST api, RFC 3986 for S3), the generic HTTP client moved there from common/fs, and the rest client reuses its curl global-init guard, ScopeGuard cleanup and header-line parsing. libcurl detection in CMake is a single find_package guarded by PAIMON_ENABLE_S3 OR PAIMON_ENABLE_REST. libcurl is used from the system rather than bundled, so the new PAIMON_ENABLE_REST option defaults to OFF like the other optional components with external requirements and is documented alongside them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, WriteHeaderCallback); | ||
| curl_easy_setopt(curl, CURLOPT_HEADERDATA, &response.headers); | ||
| // Follow redirects transparently, restricted to http(s) targets. | ||
| curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L); |
There was a problem hiding this comment.
Optional hardening, non-blocking for this PR.
CURLOPT_CUSTOMREQUEST only changes the verb in the request line; CURLOPT_POSTFIELDS changes libcurl's internal method to HTTPREQ_POST, and that is the only thing libcurl looks at on a redirect. With CURLOPT_POSTREDIR unset, 301/302 rewrite POST to GET and 303 rewrites any non-GET method, dropping the body — and a custom verb is discarded too, since http_switch_to_get() sets http_ignorecustom. 307/308 are unaffected.
The DELETE path is not reachable today: DropDatabase and DropTable pass an empty body, so the internal method stays HTTPREQ_GET and no rewrite applies. It becomes reachable if a body-carrying delete (a purge/cascade parameter) is added later, since the non-empty-body branch below sets CURLOPT_POSTFIELDS.
POST is reachable, though only against a redirecting endpoint. CreateDatabase and CreateTable would be replayed as GET on /v1/{prefix}/databases and .../tables, which are the list endpoints and return 200; since both call sites only inspect Execute(...).status(), the client reports Status::OK() while nothing was created. A conforming server will not redirect these paths, so I do not think it blocks the PR.
Would it be possible to cover both whenever convenient — either CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL, or restricting CURLOPT_FOLLOWLOCATION to idempotent methods and surfacing 3xx to the caller for POST/DELETE? Happy for this to be a follow-up.
| } | ||
| // Non-positive values fall through to the exponential backoff. | ||
| if (retry_after_ms && retry_after_ms.value() > 0) { | ||
| return retry_after_ms.value(); |
There was a problem hiding this comment.
Could you bound this delay by a client retry budget? The clamp above only prevents overflow during the seconds-to-milliseconds conversion; every positive Retry-After value is returned verbatim and later passed to sleep_for. A malformed or overly large server value can therefore block the calling thread for hours or longer. Please add a maximum per-retry delay and an overall retry deadline; when Retry-After exceeds the remaining budget, stop retrying rather than retrying earlier than requested.
| case CURLE_SSL_ISSUER_ERROR: | ||
| return false; | ||
| default: | ||
| return true; |
There was a problem hiding this comment.
Could you make retriable transport errors an explicit allowlist instead of returning true for every unlisted CURLcode? Permanent failures such as CURLE_URL_MALFORMAT, CURLE_UNSUPPORTED_PROTOCOL, CURLE_TOO_MANY_REDIRECTS and CURLE_OUT_OF_MEMORY currently run through all retries for idempotent methods without any chance of recovering — a misconfigured uri costs six attempts and roughly 31s of backoff before returning the same error, and a redirect loop costs 6 x CURLOPT_MAXREDIRS real requests. The comment above also documents the opposite guarantee ("Returns false for the transport failure kinds that do not become healthy by retrying"), which a blacklist cannot provide.
IsRetryable in common/utils/http_client.cpp already uses the allowlist shape and can be aligned with. Please enumerate only transient failures that are known to benefit from retrying and add a negative test for a permanent error.
| if (is_system_table) { | ||
| return Status::Invalid(fmt::format("Cannot drop system table {}.", identifier.ToString())); | ||
| } | ||
| PAIMON_RETURN_NOT_OK(CatalogUtils::CheckNotSystemTable(identifier, "drop table")); |
There was a problem hiding this comment.
Could you also call CheckNotBranch from FileSystemCatalog::CreateTable, DropTable, and RenameTable (for both rename identifiers)? IsSystemTable() is false for a branch identifier, while GetTableLocation() ultimately uses GetDataTableName() and strips the $branch_* suffix. As a result, dropping t$branch_b resolves to the main t directory and can delete the entire table; rename has the same base-path problem. Java AbstractCatalog rejects branch identifiers for all three operations.
| // Disable the automatic "Expect: 100-continue" behavior of libcurl on POST. | ||
| bool headers_ok = append_header("Expect:"); | ||
| for (const auto& [name, value] : headers) { | ||
| headers_ok = headers_ok && append_header(name + ": " + value); |
There was a problem hiding this comment.
Could you validate every header name and value before appending it to the curl list? libcurl sends custom header strings verbatim without filtering control characters. Since these values can come from both client options and the server-provided defaults/overrides, a value containing CRLF can inject additional headers into subsequent requests. Please require a valid HTTP token for the name and reject CR, LF, and NUL in values before storing base_headers or before constructing the curl list.
|
|
||
| /// The lowest field ID reserved for system fields; IDs at or above this bound are | ||
| /// excluded when computing the highest field ID of a schema. Value: INT32_MAX / 2 | ||
| static const int32_t SYSTEM_FIELD_ID_START = std::numeric_limits<int32_t>::max() / 2; |
There was a problem hiding this comment.
Could you make this member static constexpr int32_t, or add the corresponding out-of-line definition in special_field_ids.cpp? A plain in-class static const integral member is not implicitly inline in C++17. The current comparison is constant-folded, but any external address-taking or reference-binding use of this public-header constant will ODR-use it and fail to link because this is the only SpecialFieldIds constant without a definition.
| * ``-DPAIMON_ENABLE_AVRO=ON``: Apache Avro libraries and Paimon integration | ||
| * ``-DPAIMON_ENABLE_JINDO=ON``: Support for Alibaba Jindo filesystems | ||
| * ``-DPAIMON_ENABLE_LUMINA=ON``: Support for Lumina vector index, lumina is only supported on gcc9 or higher. | ||
| * ``-DPAIMON_ENABLE_REST=ON``: Support for the REST catalog (``metastore=rest``), requires the libcurl development package. |
There was a problem hiding this comment.
Could you also update docs/source/user_guide/catalog.rst? It still says that C++ Paimon only supports the filesystem catalog and that REST catalog support is future work. The Catalog::Create documentation should also explain that root_path is the warehouse/instance name for metastore=rest and point users to the new CatalogOptions keys (metastore, uri, and token settings), otherwise the newly added feature is difficult to discover and the user guide directly contradicts this PR.
| context.catalog = this; | ||
| context.fs = fs_; | ||
| context.warehouse = warehouse_; | ||
| context.catalog_options = api_->GetMergedOptions(); |
There was a problem hiding this comment.
Could you filter or redact sensitive options before passing them to CatalogOptionsSystemTable? GetMergedOptions() still contains the bearer token (and may contain other storage/authentication credentials), while CatalogOptionsSystemTable::BuildRows() emits every key/value unchanged. Although the table is disabled by default, a server override can enable catalog-options-table.enabled, which can expose an engine service credential to users who can query sys.catalog_options. Please centralize sensitive-key masking and apply it when building these rows.
| out.AddMember(rapidjson::StringRef("fields"), fields.Move(), allocator); | ||
| for (const char* key : {"partitionKeys", "primaryKeys"}) { | ||
| rapidjson::Value keys(rapidjson::kArrayType); | ||
| if (src.HasMember(key) && src[key].IsArray()) { |
There was a problem hiding this comment.
Could you reject a missing partitionKeys, primaryKeys or options member instead of defaulting it to an empty value? The problem is not just strictness: an omitted or misspelled partitionKeys makes a partitioned table load as unpartitioned, and a dropped options map silently changes the table's configuration — both are silent semantic changes rather than a visible failure, and the resulting TableSchema is then used for reads.
For reference, Java does not default absent members either. Jackson passes null into Schema's @JsonCreator constructor, and construction then fails for all three: options fails immediately in new HashMap<>(options); partitionKeys survives normalizePartitionKeys as null and then fails in normalizeFields via duplicateFields(partitionKeys); primaryKeys fails on primaryKeys.isEmpty() in the same method.
Please distinguish among absent, wrong-typed, and valid, return Status::Invalid for the first two on all three members, and add a non-empty partition-key round-trip test plus negative tests for the missing and wrong-typed cases.
Purpose
Linked issue: #92
Add a REST catalog implementation, selected via the
metastore=restoption, that talks the Paimon REST catalog open API:RestHttpClient: blocking libcurl client with exponential-backoff retries — 429/503 responses are retried for all methods, transport errors only for idempotent methods (name-resolution and connect failures, timeouts, TLS failures and a server closing the connection without responding are never retried), and aRetry-Afterresponse header — delta-seconds or HTTP-date form, parsed locale-independently — takes precedence over the backoff when it yields a positive delay. Per-request debug logging carries the request id.RestApi: HTTP + JSON layer with/v1/configoption merging (overrides > client options > defaults),header.options sent as request headers, paged listing, and error mapping toStatus(404 →NotExist, 409 →Exist, 400 →Invalid, 501 →NotImplemented) that prefers the code of the parsed error body over the http status, redacts sensitive server messages, never echoes response bodies into error messages (they may carry credentials), carries the request id (x-request-id, falling back to anyrequest-idheader) and attaches aRestErrorDetailwith the mapped code so callers can distinguish e.g. authentication failures programmatically.RestCatalog: database/table create/drop/rename/list/get, snapshot listing,table-default.option defaults on table creation, system/branch table checks, and conversion of the server table response into aTableSchemawith the computedhighestFieldId(a duplicated field id at any nesting level is rejected). Branch identifiers are passed through to the server, which resolves the branch and returns the branch's own schema (the default branchmainmaps case-insensitively to the bare table); snapshots of a branch are listed under the branch object name; thesysdatabase serves the local global system tables likeFileSystemCatalog.token.provider=bear).The pieces overlapping with the object-store file systems are unified under
common/utils: the generic HTTP client moved there fromcommon/fsand is shared by the S3 file system and the REST catalog,UrlUtilscarries both URL-encoding flavors (form-urlencoded for the REST api, RFC 3986 for S3) with the S3 client reusing it, and libcurl detection in CMake is a singlefind_package(CURL)gated onPAIMON_ENABLE_S3 OR PAIMON_ENABLE_REST.CatalogUtilsholds the system-database and system/branch table checks shared byFileSystemCatalogandRestCatalog.libcurl is used from the system rather than bundled, so the new CMake option
PAIMON_ENABLE_RESTdefaults toOFF, consistent with the other optional components that need something outside the bundled third-party toolchain (jindo, lance, lumina, lucene, tantivy). CI enables it explicitly inci/scripts/build_paimon.shand installs the libcurl development package, so the REST code and its tests are still built and run on every job. With the option off,Catalog::Createreports thatmetastore=restrequires a build withPAIMON_ENABLE_REST=ON.Follow-ups (not in this PR): DLF signing, data-token file IO, paged listing parameters (
maxResults/name patterns), and the remaining endpoints (alter/commit/views/partitions/tags/functions).Tests
New
paimon-rest-testbinary (55 cases, registered under theunittestlabel so it runs in CI), all against an in-process mock HTTP/REST server:RestHttpClientTest: uri normalization, query encoding, retry classification (429/503 retried, 404 and non-retriable transport errors not, truncated responses retried only for idempotent methods),Retry-Afterprecedence in both forms, retry exhaustion, redirect following, and transport errors omitting the url and query.RestUtilTest/ResourcePathsTest/RestMessagesTest: prefix extraction, redaction of sensitive server messages, resource path building with url-encoded segments, JSON round trips of all request/response messages, config merge with null-value filtering.RestCatalogTest: client-side option validation (missing uri/token, unsupported token provider), end-to-end catalog operations including pagination,ignore_if_exists/ignore_if_not_existspaths, client/server headers and the json content type,table-default.defaults, system tables, branch tables resolved by the server, broken-schema rejection, server errors surfacing as errors instead of "does not exist", and snapshot listing with sorting.RestApiErrorTest: http-status-to-Statusmapping including the request-id fallback, redaction and theRestErrorDetailcode.The shared pieces are covered next to their implementations:
UrlUtilsTestandHttpClientUtilTestin the common utils suite,TableSchemaTest.TestComputeHighestFieldIdfor the highest-field-id computation andSnapshotTest.TestToSnapshotInfofor the snapshot-to-info conversion.The
PAIMON_ENABLE_REST=OFFdefault build, clang-tidy andpre-commitwere verified on an earlier revision of this branch; the final revision is validated by the CI run on this PR, which builds withPAIMON_ENABLE_REST=ONand runs the whole suite.API and Format
include/paimon/catalog_options.hwith aPAIMON_EXPORTedCatalogOptionsstruct holding the catalog-level option keysMETASTORE,URI,TOKEN,TOKEN_PROVIDERand theTABLE_DEFAULT_OPTION_PREFIX(table-default.) key prefix; table-level keys stay inOptions(defs.h), mirroring the CatalogOptions/CoreOptions separation of the Java implementation.RestCatalogimplements theCataloginterface includingGetOptions(), which returns the client options merged with the server-side/v1/config.Catalog::Createnow dispatches on themetastoreoption (filesystemremains the default; unknown values are rejected).Documentation
docs/source/building.rstlists-DPAIMON_ENABLE_REST=ONunder the optional components, including its libcurl requirement. The catalog itself is configured through the existing options mechanism (metastore=rest,uri,token.provider,token). No standalone documentation page is added in this PR.Generative AI tooling
Generated-by: Claude Code (claude-fable-5)
🤖 Generated with Claude Code