mavam opened a new pull request, #1017:
URL: https://github.com/apache/iceberg-cpp/pull/1017
Fixes #1016
## Root cause
`ManifestEntryAdapterV{1,2,3}::Init()` stored
`ToJsonString(*partition_spec_)` as the `partition-spec` Avro metadata. That is
the general partition-spec serializer, which produces the full object
(`{"fields":[],"spec-id":0}`). The [manifest
spec](https://github.com/apache/iceberg/blob/apache-iceberg-1.10.1/format/spec.md#manifests)
requires only the partition fields array (`[]` for an unpartitioned manifest).
The spec ID already has its own `partition-spec-id` entry.
The adapters also never wrote the separate `schema-id` metadata. The spec
makes it optional in v1 and required in v2 and later. The `schema-id` inside
the JSON `schema` value doesn't count as this entry.
ClickHouse 26.7.1.1315 and 26.9.17.8 parse `partition-spec` as the array the
spec describes. On these manifests, data scans fail with `Poco::Bad cast
exception`. PyIceberg reads the old output without error, but that doesn't make
the output conformant.
## Changes
- Add `PartitionFieldsToJson` / `PartitionFieldsToJsonString` to
`json_serde_internal.h`. They serialize only the fields array, like Java's
`PartitionSpecParser.toJsonFields`. `ToJson(PartitionSpec)` now builds its
`fields` member with the same helper, so its output doesn't change. Table
metadata, REST requests, and other callers that need the full object are
unaffected.
- The v1, v2, and v3 manifest adapters now write `partition-spec` with the
new helper and `schema-id` from the schema used to write the manifest. v1 also
gets `schema-id`: the spec allows it there, and it keeps all versions
consistent.
Note for reviewers: Java's `ManifestWriter` doesn't write `schema-id` today
either. This PR follows the spec table, which requires it for v2 and later.
## Tests
`ManifestWriterHeaderTest` is new and runs for format versions 1, 2, and 3.
Each case writes a manifest and reads the key-value metadata back from the Avro
file header with `Reader::Metadata()`. It doesn't do a writer/reader round
trip, which could accept the wrong encoding.
- Unpartitioned spec with the initial schema ID: `partition-spec` must be
`[]`, `partition-spec-id` must be `0`, and `schema-id` must be present and
equal to `0`.
- Partitioned spec (spec ID 3) with a nonzero schema ID (7):
`partition-spec` must be an array that keeps the order and the `name`,
`transform`, `source-id`, and `field-id` of all three fields. `schema-id` must
be `7` and match the ID in the embedded schema.
`JsonInternalTest.PartitionFieldsToJson` checks the array serialization. It
also checks that `ToJson(PartitionSpec)` still returns the full `{"spec-id",
"fields"}` object. The existing REST test for `CreateTableRequest`
(`"partition-spec":{"spec-id":0,"fields":[]}`) still passes.
Results (macOS arm64, Apple clang 21, Debug, vendored dependencies,
`ICEBERG_BUILD_REST=ON`):
- Without the fix (new tests only): `manifest_test
--gtest_filter='*ManifestWriterHeader*'` has **6 of 6 failing**. Every version
fails with `partition-spec` equal to `{"fields":[],"spec-id":0}` (or the full
object for the partitioned case) and no `schema-id`.
- With the fix: the same filter passes 6 of 6.
- `manifest_test`: 216 passed, 18 skipped. The skips are existing
version-dependent `GTEST_SKIP`s.
- `json_serde_test`: 164 passed.
- `rest_catalog_test`: 436 passed.
- `ctest`: all 19 tests passed.
- `pre-commit run` on the changed files is clean.
## AI disclosure
I used an AI coding assistant to draft the change and the tests. I reviewed
the implementation, checked it against the spec and the Java reference writer,
and ran the tests listed above myself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
