Skip to content

fix: preserve dotted column relation qualifiers - #25334

Open
AnuragRaut08 wants to merge 1 commit into
apache:mainfrom
AnuragRaut08:fix/proto-column-relation-dotted-identifiers
Open

AnuragRaut08 wants to merge 1 commit into
apache:mainfrom
AnuragRaut08:fix/proto-column-relation-dotted-identifiers

Conversation

@AnuragRaut08

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

ColumnRelation currently round-trips TableReference qualifiers through a single dotted string. When a catalog, schema, or table identifier itself contains ., the qualifier boundaries are lost during deserialization, causing the reconstructed TableReference to differ from the original.

For example, a schema named my.schema can be incorrectly interpreted as separate qualifier components after a protobuf round-trip.

What changes are included in this PR?

  • Add a structured parts field to ColumnRelation.
  • Serialize TableReference components using TableReference::to_vec().
  • Prefer the structured parts representation when deserializing.
  • Fall back to the existing relation string for legacy protobuf messages.
  • Add round-trip tests for dotted bare, partial, and full table references.
  • Add a test covering decoding of legacy relation-only messages.
  • Regenerate the protobuf bindings.

What is the testing strategy for this PR?

Added regression tests covering the Column -> protobuf -> TableReference round-trip for:

  • Bare table references containing .
  • Partial references with dotted schema identifiers
  • Full references with dotted schema identifiers
  • Legacy relation-only protobuf messages

Also ran:

cargo test -p datafusion-proto-common column_relation -- --nocapture

cargo test -p datafusion-proto-common

All tests pass.

Are there any user-facing changes?

No.

@github-actions

github-actions Bot commented Sep 15, 2026 •

Copy link
Copy Markdown

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details
     Cloning apache/main
    Building datafusion-proto-common v55.1.0 (current)
       Built [  22.128s] (current)
     Parsing datafusion-proto-common v55.1.0 (current)
      Parsed [   0.047s] (current)
    Building datafusion-proto-common v55.1.0 (baseline)
       Built [  21.138s] (baseline)
     Parsing datafusion-proto-common v55.1.0 (baseline)
      Parsed [   0.047s] (baseline)
    Checking datafusion-proto-common v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   1.122s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip

--- failure constructible_struct_adds_field: struct exhaustively constructible through public API adds field ---

Description:
A pub struct that could be exhaustively constructed with a literal using only public API has a new pub field, breaking existing exhaustive literals.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field ColumnRelation.parts in /home/runner/work/datafusion/datafusion/datafusion/proto-common/src/generated/prost.rs:7
  field ColumnRelation.parts in /home/runner/work/datafusion/datafusion/datafusion/proto-common/src/generated/prost.rs:7
  field ColumnRelation.parts in /home/runner/work/datafusion/datafusion/datafusion/proto-common/src/generated/prost.rs:7

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  45.624s] datafusion-proto-common
    Building datafusion-proto-models v55.1.0 (current)
       Built [  23.862s] (current)
     Parsing datafusion-proto-models v55.1.0 (current)
      Parsed [   0.135s] (current)
    Building datafusion-proto-models v55.1.0 (baseline)
       Built [  24.069s] (baseline)
     Parsing datafusion-proto-models v55.1.0 (baseline)
      Parsed [   0.137s] (baseline)
    Checking datafusion-proto-models v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   1.735s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip

--- failure constructible_struct_adds_field: struct exhaustively constructible through public API adds field ---

Description:
A pub struct that could be exhaustively constructed with a literal using only public API has a new pub field, breaking existing exhaustive literals.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field ColumnRelation.parts in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/datafusion_proto_common.rs:7
  field ColumnRelation.parts in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/datafusion_proto_common.rs:7

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  50.938s] datafusion-proto-models

@github-actions github-actions Bot added the auto detected api change Auto detected API change label Sep 15, 2026
@AnuragRaut08
AnuragRaut08 force-pushed the fix/proto-column-relation-dotted-identifiers branch from e3b61d9 to 7d2603c Compare September 15, 2026 16:17
@codecov-commenter

codecov-commenter commented Sep 15, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 78.78788% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.92%. Comparing base (a0631ed) to head (7d2603c).
⚠️ Report is 73 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/proto-common/src/generated/pbjson.rs 0.00% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25334      +/-   ##
==========================================
- Coverage   81.92%   81.92%   -0.01%     
==========================================
  Files        1135     1135              
  Lines      427573   427637      +64     
  Branches   427573   427637      +64     
==========================================
+ Hits       350279   350327      +48     
- Misses      56367    56383      +16     
  Partials    20927    20927              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@AnuragRaut08

Copy link
Copy Markdown
Contributor Author

The ColumnRelation protobuf change intentionally adds parts as a new field while retaining relation for compatibility. New writers populate both fields, while readers prefer parts and fall back to the legacy relation field. This preserves existing serialized messages while keeping dotted identifiers lossless across the round trip.

The auto detected api change label is expected here because the generated public protobuf struct gains the new field.

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnuragRaut08,

Thanks for working on this. The protobuf change preserves dotted qualifier boundaries while keeping the legacy relation field for compatibility. I only have one non-blocking test suggestion.

Comment thread datafusion/proto-common/src/to_proto/mod.rs
@AnuragRaut08
AnuragRaut08 force-pushed the fix/proto-column-relation-dotted-identifiers branch from 7d2603c to 9e15374 Compare October 5, 2026 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto detected api change Auto detected API change proto Related to proto crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

3 participants