apache / apache/datafusion

ComposedPhysicalExtensionCodec should not rely on positional encoding

Open
#24,331 4 comments 0 reactions 1 assignee Claimed by @imtherealnaska View on GitHub
enhancement
Dominant language
Rust
Stars
9.3k
Forks
2.4k
Avg merge
3d 7h
Merged PRs (30d)
344

Description

### Is your feature request related to a problem or challenge?

ComposedPhysicalExtensionCodec should identify codecs by stable ID instead of list position

`ComposedPhysicalExtensionCodec` records the position of each codec in the `DataEncoderTuple`:

```rust
struct DataEncoderTuple {
pub encoder_position: u32,
pub blob: Vec,
}

The problem is that if someone adds, removes, or reorders a codec, you get really confusing error messages. For example:
1. A writer with [CodecA, CodecB] encodes protobuf `A` and specifies `encoder_position: 0`
2. A reader with [CodecB, CodecA] decodes the proto using `encoder_position: 0` and errors with "CodecB cannot decode message"
```
It's hard to see that this was caused a breaking protocol change. Since both codecs are present, it doesn't seem like a breaking change, but it is.

### Describe the solution you'd like

Maybe we can introduce an id to look up encoders:
```rust
struct DataEncoderTuple {
// Retained for decoding payloads written by older versions.
pub encoder_position: u32,
pub blob: Vec,
pub codec_id: Option,
}

Decoding would:

1. Use codec_id when present.
2. Return an explicit error containing the unknown identifier when it is not registered.
3. Fall back to encoder_position for legacy payloads.
```

### Describe alternatives you've considered

_No response_

### Additional context

_No response_

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.