Disallow ORDER BY literal in window functions

[kaikalur]

Disallow ORDER BY literal in Window functions as this is currently treated differently for int literals compared to ORDER BY in SELECT where it's treated it as an ordinal.

Test plan - (Please fill in how you tested your changes)
Test included.

== RELEASE NOTES ==

General Changes
* Disallow ORDER BY literals used with Window functions as it's not useful, expensive and most often used wrongly. 

[rongrong]

While I agree with this change in principle, since this is backward incompatible, we should add a session property that's by default not enabled for the next release, add a warning to say that this feature is not what you think it is, and will be removed in future releases. And then maybe a few release later we would switch the default behavior. Then removing the session property.

[kaikalur]

While I agree with this change in principle, since this is backward incompatible, we should add a session property that's by default not enabled for the next release, add a warning to say that this feature is not what you think it is, and will be removed in future releases. And then maybe a few release later we would switch the default behavior. Then removing the session property.

I agree we should have a way to disable it but I think default should be fail as the fix is trivial or they can just turn on the session property.

[rongrong]

While I agree with this change in principle, since this is backward incompatible, we should add a session property that's by default not enabled for the next release, add a warning to say that this feature is not what you think it is, and will be removed in future releases. And then maybe a few release later we would switch the default behavior. Then removing the session property.

I agree we should have a way to disable it but I think default should be fail as the fix is trivial or they can just turn on the session property.

Overall any unannounced behavior change will never be associated with good user experience. But if you insist the default is fail, at least make it clear in error message how to fix / disable it. And make sure when we deploy it, the default is changed in our internal configuration, unless you already made sure that all affected pipelines are fixed beforehand.

[kaikalur]

So added a session param which by default allows this. And so it's a performance warning when enabled, error when not enabled.

[rongrong]

Do you want to call it ordinals or literals? Just pick one! :P

[kaikalur]

Oh yeah - I think literals is more appropriate as that's what we are doing. Fixed.

[rongrong]

Some more nits. Also I think the release note is also not accurate, please revise. Thanks!

[rongrong]

We normally use toImmutableList() rather than Collectors.toList()
We normally format this as

orderBy.getSortItems().stream()
        .filter(...)
        .collect(toImmutableList())

You can use

List<SortItem> constSortItems = window.getOrderBy()
        .ifPresent(orderBy -> orderBy.getSortItems()....)
        .orElse(ImmutableList.of());

[kaikalur]

Done

[rongrong]

instead of?

[kaikalur]

Well we already set the context in the first clause. Anyway I fixed it and also removed the unnecessary information.

[rongrong]

I think the description is not accurate.

[kaikalur]

Fixed.