JoeryH opened a new issue, #24776: URL: https://github.com/apache/datafusion/issues/24776
### Describe the bug `datafusion-proto` serializes a column's qualifier as a single unquoted string and re-parses it on the way back: - encode — [`proto-common/src/to_proto/mod.rs`](https://github.com/apache/datafusion/blob/8332cfafb37aa9209eacdbb098afc3e4bcd7f59a/datafusion/proto-common/src/to_proto/mod.rs#L245-L253): ```rust relation: c.relation.map(|relation| protobuf::ColumnRelation { relation: relation.to_string(), }), ``` - decode — [`proto-common/src/from_proto/mod.rs`](https://github.com/apache/datafusion/blob/8332cfafb37aa9209eacdbb098afc3e4bcd7f59a/datafusion/proto-common/src/from_proto/mod.rs#L145-L149): ```rust impl From<protobuf::ColumnRelation> for TableReference { fn from(rel: protobuf::ColumnRelation) -> Self { Self::parse_str_normalized(rel.relation.as_str(), true) } } ``` `TableReference`'s `Display` writes the parts joined by `.` **without quoting**, while `parse_str_normalized` splits on unquoted `.`. So any `TableReference` whose *segments* contain a `.` does not round-trip. The same applies to `DfField.qualifier`, which is encoded the same way. This is silent: no error is raised, the reference is just re-partitioned into different segments. A `Partial { schema: "my.schema", table: "t" }` comes back as `Full { catalog: "my", schema: "schema", table: "t" }`, which then resolves against a different (or non-existent) table. Identifiers containing dots are legal — any dialect that allows quoted identifiers can produce them, and they show up in practice when an external catalog's naming is mapped into DataFusion. ### To Reproduce Through the actual proto conversions (`datafusion-proto-common`, main @ 8332cfafb): ```rust use datafusion_common::{Column, TableReference}; use datafusion_proto_common as protobuf; fn roundtrip(col: Column) { let encoded: protobuf::Column = (&col).into(); let decoded: Column = (&encoded).into(); let ok = if decoded == col { "OK " } else { "LOSS" }; println!("{ok} {col:?}\n -> proto relation {:?}\n -> {decoded:?}\n", encoded.relation.as_ref().map(|r| &r.relation)); } fn main() { roundtrip(Column::new(Some(TableReference::full("c", "s", "t")), "x")); roundtrip(Column::new(Some(TableReference::partial("my.schema", "t")), "x")); roundtrip(Column::new(Some(TableReference::bare("has.dot")), "x")); } ``` Output: ``` OK Column { relation: Some(Full { catalog: "c", schema: "s", table: "t" }), name: "x" } -> proto relation Some("c.s.t") -> Column { relation: Some(Full { catalog: "c", schema: "s", table: "t" }), name: "x" } LOSS Column { relation: Some(Partial { schema: "my.schema", table: "t" }), name: "x" } -> proto relation Some("my.schema.t") -> Column { relation: Some(Full { catalog: "my", schema: "schema", table: "t" }), name: "x" } LOSS Column { relation: Some(Bare { table: "has.dot" }), name: "x" } -> proto relation Some("has.dot") -> Column { relation: Some(Partial { schema: "has", table: "dot" }), name: "x" } ``` The underlying `TableReference` round-trip (`Display` -> `parse_str_normalized`), which is what the proto layer relies on, fails the same way and also has a case that collapses to a single bare segment: ``` LOSS Full { catalog: "cat", schema: "my.schema", table: "t" } -> "cat.my.schema.t" -> Bare { table: "cat.my.schema.t" } LOSS Partial { schema: "s", table: "tbl.with.dots" } -> "s.tbl.with.dots" -> Bare { table: "s.tbl.with.dots" } ``` ### Expected behavior A `Column`/`DfField` qualifier round-trips through `datafusion-proto` unchanged, whatever characters its segments contain. ### Additional context Two possible fixes: 1. **Quote on the way out.** Encode with `TableReference::to_quoted_string()` and decode with a parser that honours the quoting. This is a wire-compatible change in the sense that the proto schema is untouched, but blobs written by older versions still decode by the old rules, so it is only correct if both ends move together. 2. **Make `ColumnRelation` structured**, e.g. adding `repeated string parts = 2;` alongside the existing `string relation = 1;`. New writers populate both, readers prefer `parts` when present. This is properly backward and forward compatible and removes the parse round-trip entirely. I'd lean towards (2), since it also removes the ambiguity for readers of blobs written by older versions. Happy to put up a PR for whichever the maintainers prefer. -- 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]
