Skip to content

feat(core): handle RelCommon data for every relation type - #1069

Open
nielspardon wants to merge 1 commit into
substrait-io:mainfrom
nielspardon:feat/rel-common-all-relations
Open

feat(core): handle RelCommon data for every relation type#1069
nielspardon wants to merge 1 commit into
substrait-io:mainfrom
nielspardon:feat/rel-common-all-relations

Conversation

@nielspardon

@nielspardon nielspardon commented Aug 5, 2026

Copy link
Copy Markdown
Member

Hint was wired into most but not all relations, and each of the ~31 newXxx methods in ProtoRelConverter repeated the same four RelCommon setters by hand. Two had drifted: Cross dropped its hint and TopN dropped its rel anchor when converting from proto.

The proto → POJO direction is now centralised in ProtoRelConverter.applyRelCommon, mirroring RelProtoConverter.common on the way out, so the emit mapping, common extension, hint and rel anchor are applied in one place. This needs type-agnostic withRemap / withCommonExtension / withHint copy methods on Rel alongside the existing withRelAnchor; they are useful for plan rewriting in their own right. applyRelCommon copies a field only when it differs, so it stays a no-op for a common { direct {} } message — including for custom, non-Immutables relations, which inherit Rel's throwing defaults.

Two data-loss bugs surfaced along the way:

  • RelProtoConverter.common never wrote RelCommon.Hint.advanced_extension, so Hint.getExtension() was silently dropped on every relation even though the inbound direction read it back.
  • UpdateRel has no common field (spec v0.99.0), but the RelCommon accessors live on Rel, so a NamedUpdate can hold them — and an emit mapping changes getRecordType(). Serialising one produced a plan whose schema disagreed with the POJO. visit(NamedUpdate) now rejects it instead of dropping it silently. This is a behaviour change: it makes SubstraitBuilder.namedUpdate(..., Rel.Remap) fail at serialisation rather than emit a plan that cannot be re-imported.

RelCommonRoundtripTest covers one sample of every RelVisitor relation type and fails when a new relation has no sample. It is keyed on POJO types rather than proto oneof cases, so a new sub-case mapping onto an existing POJO type still needs care — noted in AGENTS.md.

Closes #297

🤖 Generated with AI

`Hint` was wired into most but not all relations, and each of the ~31 `newXxx`
methods in `ProtoRelConverter` repeated the same four `RelCommon` setters by
hand. Two had drifted: `Cross` dropped its hint and `TopN` dropped its rel
anchor when converting from proto.

The proto -> POJO direction is now centralised in
`ProtoRelConverter.applyRelCommon`, mirroring `RelProtoConverter.common` on the
way out, so the emit mapping, common extension, hint and rel anchor are applied
in one place. This needs type-agnostic `withRemap` / `withCommonExtension` /
`withHint` copy methods on `Rel` alongside the existing `withRelAnchor`; they
are useful for plan rewriting in their own right. `applyRelCommon` copies a
field only when it differs, so it stays a no-op for a `common { direct {} }`
message - including for custom, non-Immutables relations, which inherit `Rel`'s
throwing defaults.

Two data-loss bugs surfaced along the way:

- `RelProtoConverter.common` never wrote `RelCommon.Hint.advanced_extension`,
  so `Hint.getExtension()` was silently dropped on every relation even though
  the inbound direction read it back.
- `UpdateRel` has no `common` field (spec v0.99.0), but the `RelCommon`
  accessors live on `Rel`, so a `NamedUpdate` can hold them - and an emit
  mapping changes `getRecordType()`. Serialising one produced a plan whose
  schema disagreed with the POJO. `visit(NamedUpdate)` now rejects it instead
  of dropping it silently. This is a behaviour change: it makes
  `SubstraitBuilder.namedUpdate(..., Rel.Remap)` fail at serialisation rather
  than emit a plan that cannot be re-imported.

`RelCommonRoundtripTest` covers one sample of every `RelVisitor` relation type
and fails when a new relation has no sample. It is keyed on POJO types rather
than proto `oneof` cases, so a new sub-case mapping onto an existing POJO type
still needs care - noted in AGENTS.md.

Closes substrait-io#297
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

handle hints for all relation types

1 participant