apache / apache/datafusion

Standardize the separator in name

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

Description

### Is your feature request related to a problem or challenge?

While working on replacing GetFieldAccess with `get_field` function, I got the [name checking error](https://github.com/apache/datafusion/blob/b21bf9e8527de82e901c3a61127d63779f230163/datafusion/physical-expr/src/equivalence/projection.rs#L70-L73) that due to the mismatch of separator in creating name. I found that most of the name built with single comma and space, so I think we should switch others to this format `.join(", ")`.

Error is like
```
External error: query failed: DataFusion error: Internal error: Input field name SUM(get_field(CASE WHEN get_field(t2.struct(t1.time,t1.load1,t1.load2,t1.host), Utf8("c3")) IS NOT NULL THEN t2.struct(t1.time,t1.load1,t1.load2,t1.host) END,Utf8("c2"))) does not match with the projection expression SUM(get_field(CASE WHEN get_field(t2.struct(t1.time,t1.load1,t1.load2,t1.host),Utf8("c3")) IS NOT NULL THEN t2.struct(t1.time,t1.load1,t1.load2,t1.host) END,Utf8("c2"))).
This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker
[SQL] select t2."struct(t1.time,t1.load1,t1.load2,t1.host)"['c3'] as host, sum((case when t2."struct(t1.time,t1.load1,t1.load2,t1.host)"['c3'] is not null then t2."struct(t1.time,t1.load1,t1.load2,t1.host)" end)['c2']) from (select struct(time,load1,load2,host) from t1) t2 where t2."struct(t1.time,t1.load1,t1.load2,t1.host)"['c3'] IS NOT NULL group by t2."struct(t1.time,t1.load1,t1.load2,t1.host)"['c3'] order by host;
```

I got one with `t2.struct(t1.time,t1.load1,t1.load2,t1.host), Utf8("c3")` and another `t2.struct(t1.time,t1.load1,t1.load2,t1.host),Utf8("c3")`. That is why I think we should fix the separator.

Since the change is quite large, I would like to wait for more comment.

### Describe the solution you'd like

_No response_

### Describe alternatives you've considered

_No response_

### Additional context

_No response_

You can find the error [here](https://github.com/jayzhan211/arrow-datafusion/commit/41c871d530cef177bf5784e09f48dd0f1044e6ab) with `cargo test --test sqllogictests -- expr`. Although, it is not on the main branch, but I think most of the changes are unrelated to the name checking issue.

Contributor guide

Open the contributing guide

Research direction

Start with datafusion/physical-expr/src/equivalence/projection.rs at the linked name-checking code, then use the reported cargo test --test sqllogictests -- expr command to reproduce the mismatch. Trace the name construction involved in the example and verify that the affected names consistently use ", " as their separator and that the relevant SQL logic tests pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.