Improve column naming by aliasing with expression name

[Dandandan]

Which issue does this PR close?

Closes #279

Rationale for this change

To support optimizations like constant folding the column names need to keep consistent.
This PR also improves the display of column names.

What changes are included in this PR?

• Wrap unnamed expressions in a alias using the formatted sql expression. This keeps us much closer to the original expression (which you can see in the tests).
• Don't display alias in logical plan for column expressions where the alias is the same as the column name. This avoids the problem that every formatted logical plan would look like #col AS col
• TODO: think a bit more about implications. Adding alias means that a query could use the alias in another projection, which is not what we want.

Are there any user-facing changes?

Yes, column names will be different.

[codecov-commenter]

Codecov Report

Merging #280 (8522633) into master (b8805d4) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #280      +/-   ##
==========================================
- Coverage   76.19%   76.13%   -0.06%     
==========================================
  Files         140      140              
  Lines       23595    23607      +12     
==========================================
- Hits        17978    17974       -4     
- Misses       5617     5633      +16     

Impacted Files	Coverage Δ
datafusion/src/execution/context.rs	92.66% <100.00%> (+0.02%)	⬆️
datafusion/src/execution/dataframe_impl.rs	89.10% <100.00%> (+0.21%)	⬆️
datafusion/src/logical_plan/expr.rs	81.68% <100.00%> (-2.72%)	⬇️
datafusion/src/sql/planner.rs	83.65% <100.00%> (+0.02%)	⬆️
datafusion/src/sql/utils.rs	48.12% <0.00%> (-0.63%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8805d4...8522633. Read the comment docs.

[Dandandan]

I will do some more research into what's needed for column names, I think we have to avoid reusing alias for this and see if we should support unnamed columns better.

[alamb]

This is an interesting idea -- and seems similar to what @wqc200 was working on in apache/arrow#9600

[alamb]

I don't really understand the concerns about column expressions and what might be bad here. The alternate is probably to keep a "name" field on all Exprs which was the approach in apache/arrow#9600

[alamb]

This seems like an improvement to me.

[Dandandan]

I don't really understand the concerns about column expressions and what might be bad here. The alternate is probably to keep a "name" field on all Exprs which was the approach in apache/arrow#9600

Happy to reconsider! My main concern is that introducing aliases for unnamed expressions will cause other "strange" issues like using the name of the expression, like:
select "1+1" from (select 1+1) t

Which should fail but would return 2?

I think we should make sure unnamed expressions can only be referenced by index, or with * and not by a generated name.

[alamb]

That makes sense, thank you for the clarification

…

On Fri, May 7, 2021 at 5:53 PM Daniël Heres ***@***.***> wrote: I don't really understand the concerns about column expressions and what might be bad here. The alternate is probably to keep a "name" field on all Exprs which was the approach in apache/arrow#9600 <apache/arrow#9600> Happy to reconsider! My main concern is that introducing aliases for unnamed expressions will cause other "strange" issues like using the name of the expression, like: select "1+1" from (select 1+1) t Which should fail but would return 2? I think we should make sure unnamed expressions can only be referenced by index, or with * and not by a generated name. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#280 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADXZMJ5SN5I2FDE3TBHR2TTMRON7ANCNFSM44H4TNUA> .

[houqp]

I think we should make sure unnamed expressions can only be referenced by index, or with * and not by a generated name.

+1, I did some research on this as well, postgresql even name most of the unnamed expression ?column? in their output. Users should either use index or manually create aliases.