[default values] Tables: apply server-supplied per-column defaults at read time - #645
[default values] Tables: apply server-supplied per-column defaults at read time#645cbb330 wants to merge 3 commits into
Conversation
c6641c7 to
bc2d69c
Compare
9b5e906 to
14c82c3
Compare
7c276f0 to
fdfaf48
Compare
d4ac8fd to
7df59f3
Compare
| } | ||
| // TODO(read-bridge): overlay columnDefaults onto raw.schemas() via withSchemaOverlay; future V3 | ||
| // features bridged from config are applied here too. | ||
| return raw; |
There was a problem hiding this comment.
This method is currently a no-op. The apply() method currently always returns raw unchanged, and I couldn’t find a call site where this method is invoked during table metadata refresh/load. The server stamps the read-bridge config into LoadTableResponse, but the client never consumes it, so the feature isn’t active end-to-end. Do we plan to extend this further in the future PRs as a follow-up?
There was a problem hiding this comment.
Yes, by design this #645 is the O.S. substrate and the overlay transform marked TODO, and defaults only arrive once a ColumnDefaultsSource is supplied in closed/source since it relies on linkedin schema object "avro.schema.literal"
| int fieldId = Integer.parseInt(entry.getKey().substring(COLUMN_DEFAULT_PREFIX.length())); | ||
| byFieldId.put(fieldId, MAPPER.readTree(entry.getValue())); | ||
| } catch (RuntimeException | JsonProcessingException e) { | ||
| log.warn( |
There was a problem hiding this comment.
Minor: It would be useful to log the COLUMN_DEFAULT_PREFIX to aid debugging
There was a problem hiding this comment.
Thanks, I actually considered the below comment and converted the behavior to fail loud instead of softly with logging. a decode failure on a column-default.* entry now fails with IllegalStateException with the offending key=value in the message, so it surfaces immediately instead of hiding in a warn.
| try { | ||
| int fieldId = Integer.parseInt(entry.getKey().substring(COLUMN_DEFAULT_PREFIX.length())); | ||
| byFieldId.put(fieldId, MAPPER.readTree(entry.getValue())); | ||
| } catch (RuntimeException | JsonProcessingException e) { |
There was a problem hiding this comment.
Is it safe to silently not consider the (key, value) pairs that fails with exception and chose the other values. Are we expecting a fixed set of configuration key as json in this case? If so, would be reasonable to set default if the values for config-key are malformed?
There was a problem hiding this comment.
Good question, I changed the behavior to address it. Three parts:
-
converted a Silent Skip into Fail Loud (for known keys within openhouse.read-bridge.column-default.*). By construction the value is always a valid field within the schema. That means a decode failure isn't an expected input to the client and it can only be an encoder bug or transport corruption / issue within the ASL. Silently skipping it would hide a real defect (a column silently reads NULL instead of its default), so we now throw instead.
-
"Fixed set of keys as json?" no its not fixed. the keyspace is open: the field-id in the suffix is dynamic per table, so we can't enumerate expected keys. But each key's shape is encoded with int suffix + single-value JSON.
-
"Set a default if malformed?" We deliberately don't, there's no safe fallback for "the table's default is unreadable," and fabricating one would risk returning a wrong value and hard coding NULL data (worse than a recognizable failure). So we fail loud rather than guess.
Caveat for follow-up work: this PR guarantees the value returned from server is well-formed, but not that it's the correct default for its column. When the schema overlay (e.g. ASL) PR lands, the apply step must fail loud if a default can't bind to its column, and the server must validate default-vs-schema consistency. we can't trust the client to be semantically correct on its own.
shanthoosh
left a comment
There was a problem hiding this comment.
Looks good overall. Have few minor clarification questions/comments.
7df59f3 to
747c7ef
Compare
… GetTableResponseBody and capture it client-side (#644) ## Summary Adds a generic, server-stamped, per-table client **`config`** map to `GetTableResponseBody` and captures it in `OpenHouseTableOperations` so subclasses can read it. It follows the **Iceberg REST `LoadTableResponse.config` convention** — a `Map<String,String>` of client-side behavior overrides the server controls at runtime, without re-rolling the slow-to-upgrade Java client fleet. **No behavior change**: the map is null/empty until a server stamps it, and the base client has nothing to act on. ## Changes - [x] Client-facing API Changes - [x] Internal API Changes - [ ] Bug Fixes - [x] New Features - [x] Tests **Client-facing API Changes** - READ_ONLY, nullable `config` field (`Map<String,String>`) on `GetTableResponseBody`, modeled on the Iceberg REST load-table `config`. Namespaced keys (e.g. `openhouse.read-bridge`); clients ignore keys they do not understand. - Why a response field: `doRefresh` already fetches a live `GetTableResponseBody` on every table load (and on commit responses) but discarded all but `getTableLocation()` — it is the natural, already-present, server-controlled, zero-staleness delivery channel. The generated client sets `FAIL_ON_UNKNOWN_PROPERTIES = false`, so a new response field cannot break older clients (and unknown config keys are simply carried). Same additive, nullable, READ_ONLY pattern as `sortOrder` etc. **Internal API Changes** - `OpenHouseTableOperations.doRefresh` keeps the full response and stashes `config` in an `AtomicReference`, exposed to subclasses via the new `protected currentConfig()`. READ_ONLY + side-channel: never sent back on writes. - `TablesMapper` (the table DTO serializer into gettableresponsebody) ignores `config` (`@Mapping(target = "config", ignore = true)`) — it is stamped separately, not sourced from `TableDto`. **New Features** - A flat string map keyed by namespaced keys means new features become new `config` entries rather than an API/schema change or client regen. ## Testing Done - [x] Added new tests. Compile/codegen verified (`:services:tables`, `:client:tableclient`, `:integrations:java:iceberg-1.2:openhouse-java-runtime`); the client regenerates with `getConfig()` returning `Map<String,String>`. `OpenHouseTableOperationsTest` covers: `currentConfig()` null before refresh; `doRefresh` captures/clears `config`; the REST-style string map deserializes from a response and tolerates unknown fields. ## Stack **#644 (channel)** → #645 (read-bridge mechanism) → [li-openhouse #2166](https://github.com/linkedin-multiproduct/li-openhouse/pull/2166) (li column-default source from avro.schema.literal) This is the substrate PR — intentionally behaviorless on its own. ## Next steps (this PR) - [ ] Review + merge first — it is the base of the stack; #645 and #2166 depend on it. This PR delivers only the channel; it stays inert until two further pieces exist, both **required**, and both delivered by the next PR in the stack: - **A server stamp** to populate `config` (it is null until something stamps it) — required, delivered by #645 via `ReadBridgeConfigResolver` + the `ColumnDefaultsSource` seam. - **A client consumer** that reads `currentConfig()` and acts on it — required, delivered by #645's read-bridge metadata overlay, driven by the column defaults that #2166 supplies. --------- Co-authored-by: Cursor <[email protected]>
747c7ef to
112cdab
Compare
112cdab to
2f2fccb
Compare
e6ae89e to
2f66d83
Compare
| } | ||
|
|
||
| public Map<String, String> resolve(String databaseId, String tableId, TableDto tableDto) { | ||
| Map<Integer, JsonNode> columnDefaults = columnDefaultsSource.defaults(tableDto); |
There was a problem hiding this comment.
Does this mean that if the table was to update the defaults we may read the newly changed defaults?
There was a problem hiding this comment.
Yes. And the defaults are never persisted into Iceberg metadata. there are two ways to prevent it:
- remove the initial-defualts attribute from the serialization method in iceberg fork
- sanitize the schema in OH server prior to writing the metadata file
I plan to implement 2. in a follow up PR. i prefer sanitizing the schema on OH server because we can change the runtime behavior easily in the future. Option 1 would tie us to the client version for changing behavior.
With that, a thing to clarify on consistency in the transaction: In upstream iceberg, linkedin's hive reader, and openhouse's iceberg client, the default value only takes action after the table has been committed and subsequently read. In all cases, an existing DataFrame that is evolved with a default column does not acquire a default value until after the evolution has been committed and a subsequent read has created a new dataframe.
There was a problem hiding this comment.
I think the defaults need to be immutable to the table. Otherwise a currently running query reloading the table will get inconsistent results on the same data for the same version. Under what conditions does the default value change and can it be immutable once set?
There was a problem hiding this comment.
answered below. we keep default value immutable in a follow up pr #678
| } | ||
|
|
||
| public Map<String, String> resolve(String databaseId, String tableId, TableDto tableDto) { | ||
| Map<Integer, JsonNode> columnDefaults = columnDefaultsSource.defaults(tableDto); |
There was a problem hiding this comment.
How does it work with respect to MVCC? time travel?
There was a problem hiding this comment.
MVCC works same as other concurrent schema evolution operations -- config and the metadata location come from the same getTable response, and the overlay is applied inside the metadata load, so a given TableMetadata is always overlaid with config from that same server version. there isn't mixing across versions. A losing commit throws CommitFailedException, and Iceberg's retry calls ops.refresh() on each attempt, which re-fetches config and re-applies the overlay together.
Time travel is a good point to call out and must be handled carefully otherwise the client may fail while applying the overlay. the client overlay is in a follow up PR. It will look like this:
- The server returns the default-value config together with the metadata location.
- During metadata loading, the client decodes the config into a field-ID-to-default map, then applies it independently to every schema in the TableMetadata object.
- For each schema, the client attaches defaults only to field IDs present in that schema and ignores entries for fields that did not yet exist.
- The resulting schema-ID-to-overlaid-schema map is returned to the object, allowing normal latest, time-travel, tag, and branch reads to select the appropriate schema without unexpected errors.
There was a problem hiding this comment.
But basically time traveling will not actually work on these table if the default value changes over the livetime and will give incorrect results. The problem is not there if we don't allow chaning the default.
There was a problem hiding this comment.
yes, we handle time traveling correctly because we keep default value immutable in a follow up pr #678
|
e96bcf1 to
3df3d5c
Compare
@mkuchenbecker we actually need both. there was a mistake in the PR description, corrected it. |
## Summary Problem: I have a feature which I want to ramp on the server. but I also don't want to prevent table owners from self-serve opting in. A generic function to handle that overlap doesn't exist today. Extend the existing `TableFeatureToggle` with self-service table overrides while preserving its server-managed targeting API. An explicit `<featureId>.enabled=true|false` table property wins. When the property is absent, activation delegates to the server toggle. Server rules now support trailing-`*` prefix matching independently for database and table names. ## Changes - [ ] Client-facing API Changes - [x] Internal API Changes - [ ] Bug Fixes - [x] New Features - [ ] Performance Improvements - [ ] Code Style - [ ] Refactoring - [ ] Documentation - [x] Tests Adds a binary-compatible default method to `TableFeatureToggle`: ```java isFeatureActivatedWithOverride(TableDto tableDto, String featureId) ``` It is deliberately **not** an overload of `isFeatureActivated`. The two carry different safety contracts, and a distinct name makes the difference visible at the call site: authorization gates such as `enable_mor` decide whether a user may write a preserved table property, so they must keep using the server-only `isFeatureActivated(String, String, String)`. The override-honoring form reads a property the gated user can write. An override that is neither `true` nor `false` fails closed: it is logged and the feature is treated as inactive. The gate is evaluated on the table-load path, so throwing would turn a typo like `read-bridge.enabled=flase` into a `400` and make the table unloadable. Extends the existing toggle rule matcher while preserving exact and `*` matching: - `tracking.events` matches exactly. - `tracking_*.events_*` matches database and table prefixes. - `*.*` matches every table. ## Testing Done - [ ] Manually Tested on local docker setup. Please include commands ran, and their output. - [x] Added new tests for the changes made. - [ ] Updated existing tests to reflect the changes made. - [ ] No tests added or updated. Please explain why. If unsure, please feel free to ask for help. - [ ] Some other form of testing like staging or soak time in production. Please explain. Ran: ```shell JAVA_HOME=$(/usr/libexec/java_home -v 17) ./gradlew \ :services:tables:test \ --tests 'com.linkedin.openhouse.tables.toggle.TableFeatureToggleTest' \ :services:housetables:test \ --tests 'com.linkedin.openhouse.housetables.mock.WildcardTableToggleRuleMatcherTest' \ -x CopyGitHooksTask ``` All 12 focused tests passed, covering server fallback, explicit opt-in and opt-out, fail-closed handling of unparseable overrides, exact matching, wildcard matching, and paired database/table prefix matching. # Additional Information - [ ] Breaking Changes - [ ] Deprecations - [x] Large PR broken into smaller PRs, and PR plan linked in the description. This is an independent OSS foundation for the read-bridge stack in #645 and the corresponding `li-openhouse` implementation PRs. Feature-specific default derivation remains outside this PR. Note for reviewers: the matcher change widens any existing `table_toggle_rule` row whose pattern ends in `*` but is not exactly `*`. Those previously matched nothing. Worth auditing HTS before merge.
Adds the open-source read-bridge feature on top of the per-table `config` channel (linkedin#644): - ColumnDefaultsSource: the single open-source/closed-source seam (field-id -> Iceberg single-value JSON). Open-source default is a no-op lambda in ApiConfig; a deployment overrides this bean (e.g. li-openhouse, from avro.schema.literal). - ReadBridgeConfigResolver: server-side encoder that stamps each default as a flat namespaced config entry (openhouse.read-bridge.column-default.<fieldId> = single-value JSON) — no envelope/POJO; the config map carries the structure. - ReadBridge (client): decodes those entries and applies the overlay at metadata-load time (the column-default transform is a marked TODO, and the place further V3 features get backported). Keeps loadMetadata to one call. Behaviorless until a ColumnDefaultsSource is supplied; fail-closed throughout. Co-authored-by: Cursor <[email protected]>
A known openhouse.read-bridge.column-default.* entry is produced by the server encoder from a typed JsonNode keyed by an integer field-id, so its suffix always parses as an int and its value always round-trips through readTree. A decode failure is therefore an encoder bug or transport corruption, not an expected input -- skipping it would silently read NULL instead of the column's default and hide a real defect. Throw instead. Unknown keys (a newer server feature this client doesn't recognize) are still ignored, preserving forward compatibility. Update javadoc/comments to the encoder round-trip rationale and note the guarantee covers well-formedness, not default-to-schema correctness (a write-time concern). Tests updated to assert fail-loud on bad field-id and unparseable value, plus forward-compat skip of unknown keys.
The fail-loud change updated ReadBridge but left two docs stating the old fail-closed behavior: - ColumnDefaultsSource told implementers they "must never throw", the opposite of the policy, and unimplementable alongside validating that a declared default binds to its column. Restate it as the capability-gap vs invariant-violation split: an empty map means nothing to bridge, while a declared-but-unhonorable default throws. - OpenHouseTableOperations.loadMetadata still claimed "unparseable config leaves the raw metadata untouched", which no longer holds: ReadBridge throws on a malformed known entry and loadMetadata is its only caller. Comment-only; no behavior change. Co-authored-by: Cursor <[email protected]>
3df3d5c to
86c62f6
Compare
mkuchenbecker
left a comment
There was a problem hiding this comment.
Restricting changing the default mitigates a major concern. Is there a problem doing so?
|
@mkuchenbecker we can do that, no problem. I have a follow up PR #678 that prevents schema from evolving to drop the default values. once they exist on table, they are there forever. this is exactly what the upstream datasource has in their contract, as well as Hive. so no change in behavior for customer needed. It is also how the behavior exists in V3: once initial-defaults is set, it cannot be removed or changed while that column is in the table. |
| */ | ||
| protected TableMetadata loadMetadata(String metadataLocation) { | ||
| return TableMetadataParser.read(io(), metadataLocation); | ||
| TableMetadata raw = TableMetadataParser.read(io(), metadataLocation); |
There was a problem hiding this comment.
can we add a config to fall back to old behabiour so this is config gated?
There was a problem hiding this comment.
this may or may not b ethe right spot but the concern is exposing all customers the to codepath when this lands
There was a problem hiding this comment.
The intent of this PR is already to deploy without a behavior change. This line is a stub, empty config returns raw, and even a stamped config is not overlaid yet (that's #679). Iceberg readers therefore see the same schema as today, without initial-default, so fills don't change.
First I'd smoke test this client jar in spark-shell. and then deploy the client fleet and bake. Overlay is #679, and only for tables the server has stamped, that stamp is gated by the table feature flag in #674.
Summary
Adds the open-source read-bridge feature on top of the per-table
configchannel (#644): the OH server stamps per-column initial-defaults ontoGetTableResponseBody.config, and the Java client overlays them at metadata-load time so a column added after data exists reads its declared default instead of NULL (a v2→v3 read-time bridge).The whole feature is open source; it exposes exactly one deployment seam:
ColumnDefaultsSource(interface) — the single open-source/closed-source line:field-id -> Iceberg single-value JSON. The open-source default is a no-op lambda inApiConfig; a deployment overrides this bean (li-openhouse derives values fromavro.schema.literal).ReadBridgeConfigResolver(server) — stamps each default as a flat, namespaced config entry:openhouse.read-bridge.column-default.<fieldId> = <single-value-json>. No envelope/POJO — the RESTconfigstring map carries the structure directly.ReadBridge(client) — decodes those entries and applies the read-time overlay inOpenHouseTableOperations.loadMetadata(thewithInitialDefault/withSchemaOverlaytransform is a marked TODO).loadMetadatastays one delegating call.Scope (engines)
integrations/java/iceberg-1.2/openhouse-java-runtime, but it is not 1.2-only: the 1.5 runtime compiles the same source, because itsbuild.gradleadds the 1.2 module's source dirs to its own sourceSet (srcDirs += project(':integrations:java:iceberg-1.2:openhouse-java-runtime').sourceSets.main.java.srcDirs). SoReadBridgeand theloadMetadatahook are built into both runtimes, and there is no separate 1.5 port. Neither line applies Iceberg v3initial-defaultnatively, so both need the bridge.Stack
#644 (config channel, merged) → #645 (read-bridge mechanism) → li-openhouse #2204 (the
ColumnDefaultsSourceseam) → li-openhouse #2203 (derive the defaults fromavro.schema.literal)Rebased onto
mainnow that #644 has merged, so the diff is the read-bridge change only.Next steps (this PR)
javapagainst the pinned jars:TableMetadata.withSchemaOverlayis absent from1.2.0.19, and1.5.2.15has neitherwithSchemaOverlaynorNestedField.initialDefault(the backport Bump linkedin/iceberg 1.2 to 1.2.0.19 (NestedField column-default APIs) #642 brought to the 1.2 line). Nothing in this PR uses them — the decode path is only Jackson +TableMetadata— butReadBridge.applycannot be implemented until they land.ReadBridge.apply. It must cover every schema-id inschemasByIdthat carries a bridged field-id, not just the current schema: Iceberg resolves a scan's schema from the snapshot's ownschemaId(SnapshotUtil.schemaFor), so time-travel, tag, and non-main-branch reads would otherwise return NULL while latest reads return the default.withSchemaOverlaytakes a multi-schema map for exactly this.Testing Done
:services:tables+ iceberg-1.2 runtime compile (JDK 17), and repo-widespotlessCheckgreen.ReadBridgeConfigResolverTest(server): each default round-trips as a flatopenhouse.read-bridge.column-default.<fieldId>entry; empty when the source supplies nothing.ReadBridgeTest(client): decodes the flat entries by field-id; fails loud (IllegalStateException, with the offendingkey=value) on a malformed knowncolumn-default.*entry, since the server encoder guarantees an int field-id and a value that round-trips throughreadTree— so a decode failure is an encoder bug or transport corruption, not an expected input. Unknown keys are ignored, preserving forward compatibility.OpenHouseTableOperationsTest: config capture and deserialization.