fix 621, where unnamed window functions shall be differentiated by partition and order by clause

[jimexist]

Which issue does this PR close?

Closes #621

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

[houqp]

should we update physical column name as well for better readability in the query output? https://github.com/apache/arrow-datafusion/blob/master/datafusion/src/physical_plan/planner.rs#L137-L139

[jimexist]

should we update physical column name as well for better readability in the query output? https://github.com/apache/arrow-datafusion/blob/master/datafusion/src/physical_plan/planner.rs#L137-L139

i think this is more of the strategy on how we name unnamed columns in the schema. for window functions without an alias, it'll be created with a unique name, given the function type, arg name, partition by clause, order by clause, and window frames. any of these are equivalent then the columns can be thought of the same and thus deduplicated in the projection phase, otherwise they will be planned separately.

if any such columns are having a name conflict in the projection phase the schema validation logic will fail.

[jimexist]

Postgres is totally okay with dup names in the projection phase:

[postgres] # select max(c1) over (), max(c1) over (), max(c1) over (partition by c1) from test limit 3;
 max | max | max
-----+-----+-----
 e   | e   | a
 e   | e   | a
 e   | e   | a
(3 rows)

[jimexist]

if we are okay with dup names then i guess just keeping the function name would be enough

[houqp]

We do allow query outputs (i.e. record batch schema and physical plan schema) to contain columns with same names. This is quite common in join queries where more than one relations contain the same column name.

My previous comment was more around the inconsistency on how window function names are generated in logical plane v.s. physical plane. For example, in physical plane, we don't take partition by clause, order by clause, and window frames into account when creating the column name. If we don't want to maintain this consistency, I think we should update this invariant instead: https://github.com/apache/arrow-datafusion/blob/master/docs/specification/invariants.md#the-physical-schema-is-invariant-under-planning.

https://github.com/apache/arrow-datafusion/blob/master/docs/specification/output-field-name-semantic.md contains some rules on how we want to generate field names for physical schema. We don't have any rules for window functions at the moment, so maybe worth formalize it in that document as well.

[alamb]

Looks good to me

[jimexist]

We do allow query outputs (i.e. record batch schema and physical plan schema) to contain columns with same names. This is quite common in join queries where more than one relations contain the same column name.

My previous comment was more around the inconsistency on how window function names are generated in logical plane v.s. physical plane. For example, in physical plane, we don't take partition by clause, order by clause, and window frames into account when creating the column name. If we don't want to maintain this consistency, I think we should update this invariant instead: https://github.com/apache/arrow-datafusion/blob/master/docs/specification/invariants.md#the-physical-schema-is-invariant-under-planning.

https://github.com/apache/arrow-datafusion/blob/master/docs/specification/output-field-name-semantic.md contains some rules on how we want to generate field names for physical schema. We don't have any rules for window functions at the moment, so maybe worth formalize it in that document as well.

Thanks for the advice let me address in another pull request