andygrove commented on PR #2348: URL: https://github.com/apache/datafusion-ballista/pull/2348#issuecomment-5415705440
Heads up that this turned out to break the wire format for `CopyTo` plans across versions. I filed #2376 with the details. The short version is that the old private `FileFormatProto` and DataFusion's `FileFormatProto` are structurally identical on the wire, a varint followed by a bytes field, so an old message decodes cleanly and the format tag just gets reinterpreted. The old `encoder_position` indexed into `[Parquet, Csv, Json, Arrow, Avro]` while `FileFormatKind` is `UNSPECIFIED=0, CSV=1, JSON=2, PARQUET=3, ARROW=4, AVRO=5`. Feeding each old position to the new codec gives: ``` pos=0 (Parquet) -> Err(This feature is not implemented: Unspecified file format kind) pos=1 (Csv) -> Ok(csv) lines up by coincidence pos=2 (Json) -> Ok(json) lines up by coincidence pos=3 (Arrow) -> Ok(parquet) silently wrong format pos=4 (Avro) -> Ok(arrow) silently wrong format ``` Parquet at least fails loudly, and that is what CI caught in #2374 where a released 54.0.0 Python client talks to a cluster built from the branch. The Arrow and Avro cases are the ones that worry me. `ArrowLogicalExtensionCodec::try_encode_file_format` writes an empty payload and the Parquet decoder treats empty bytes as all defaults, so `COPY ... STORED AS ARROW` from a 54 client runs as Parquet on a 55 scheduler with no error at all. This is not a hypothetical skew for us because `pyballista` re-exports datafusion-python types, so the Python bindings cannot move to 55 until there is a matching `datafusion-python` release. Every Python user runs a 54 client against a 55 cluster in the meantime. No objection to the cleanup itself, the old positional scheme was clearly worth removing. It is more that we probably want an old client to get a clear version mismatch error instead of a mis-decode. Bumping `BALLISTA_PROTOCOL_VERSION` and validating it for clients, which is #2370, would cover this and the next change of this shape. Happy to hear what you think is cleanest. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
