apache / apache/datafusion

datafusion-proto: column qualifiers containing `.` are silently corrupted on round-trip

Open
#24,776 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
9.3k
Forks
2.4k
Avg merge
3d 7h
Merged PRs (30d)
344

Description

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

Contributor guide

Open the contributing guide

Research direction

Read proto-common/src/to_proto/mod.rs and proto-common/src/from_proto/mod.rs, focusing on ColumnRelation and DfField.qualifier conversions. Run the supplied round-trip reproduction with dotted TableReference segments, then inspect the underlying TableReference display and parsing behavior. Done means qualifiers round-trip unchanged while preserving compatibility expectations for existing proto data.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend-api-design
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.