apache / apache/datafusion

ComposedPhysicalExtensionCodec should not rely on positional encoding

Aperta
#24,331 4 commenti 0 reazioni 1 assegnatario Rivendicata da @imtherealnaska Vedi su GitHub
enhancement
Lingua principale
Rust
Stelle
9.3k
Fork
2.4k
Merge medio
3g 11h
PR unite (30g)
362

Descrizione

### 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_

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.