apache / apache/datafusion

ComposedPhysicalExtensionCodec should not rely on positional encoding

Ouverte
#24,331 4 commentaires 0 réactions 1 personne assignée Réclamée par @imtherealnaska Voir sur GitHub
enhancement
Langage dominant
Rust
Étoiles
9.3k
Forks
2.4k
Merge moyen
3 j 11 h
PR mergées (30 j)
360

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_

Guide de contribution

Ouvrir le guide de contribution

Évaluation

Cette issue n'a pas encore été évaluée.

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.