apache / apache/datafusion

ComposedPhysicalExtensionCodec should not rely on positional encoding

Abierto
#24,331 4 comentarios 0 reacciones 1 asignado Reclamado por @imtherealnaska Ver en GitHub
enhancement
Lenguaje dominante
Rust
Estrellas
9.3k
Forks
2.4k
Merge medio
3 d 11 h
PR fusionados (30 d)
360

Descripción

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

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.