@matriv @kneth rebased and pushed a fixup to the state that was on kneth/column_name_for_scalar_functions. I think this approach is viable. A few tests are failing (expectedly) that still need fixing.

Unclear to me is if we should also change the HTTP output (otherwise we have to deal with jdbc 0/1 diffs in the tests that assert column names and use functions)

Probably also needs a changes entry.

Happy to hand this back to you for finalization.

I've pushed a wrapup version, but now we have the problem that functions (e.g. aggregates), which don't collapse to a literal but need to be calculated based on the query and cols, are also returned with only the function name (e.g.: max instead of max(age)).

This conforms with postgres:

postgres=# select min(a), min(a+1) from t;
 min | min 
-----+-----
   1 |   2
(1 row)

But I'm thinking of 2 variations:

1. return the whole function call if there is at least one arg:

@Override
protected Void visitSingleColumn(SingleColumn node, SelectAnalysis context) {
    Expression expression = node.getExpression();
    Symbol symbol = context.toSymbol(expression);
    String alias = node.getAlias();
    if (alias == null) {
        String colName = OutputNameFormatter.format(expression);
        String name =  null;
        if (expression instanceof FunctionCall fn) {
            if (fn.getArguments().isEmpty()) {
                name = fn.getName().getSuffix();
            } else {
                name = colName;
            }
        }
        context.add(ColumnIdent.of(colName), symbol, name);
    } else {
        context.add(ColumnIdent.of(alias), new AliasSymbol(alias, symbol), alias);
    }
    return null;
}

2. Inspect the Symbol of the relevant output and return the outputName only if the output is a Literal?

What do you think?

1. Changes entry needs to be adjusted still.
2. Regarding arithmetic operators and comparison operators we return the whole expression:

https://github.com/crate/crate/pull/17858/files#diff-fbb4d9d2f81fbfe7deb26e9f27e3f100c3580b4d76cdd98108ecd8bd7da88230R181

where as postgres returns:

postgres=# select (1+2) * 3;
 ?column? 
----------
        9
(1 row)


postgres=# select (1 >23);
 ?column? 
----------
 f
(1 row)

The same situation holds for other expressions like IN/ANY/CASE/IS NULL, etc.

3. For casts we return:

https://github.com/crate/crate/pull/17858/files#diff-ed2a6369cdf047f097bfca45fa6b4367d8a969567bdbfdfc3fd744d55af1a516R782

and postgres returns the target data type:

postgres=# select cast(1 as varchar(10));
 varchar 
---------
 1
(1 row)

What should we do for 2 and 3?
For 2. I guess it's fine to leave as is (return the whole expression) and explain in the changes entry.

For 3? I'm not sure, maybe do what postgres does?

Another issue with the current approach is that if a FunctionCall is nested within let's say an arithmetic operator, then we still print out the whole function call. e.g.:

cr> select (1 + abs(a)) from tbl;
+--------------+
| (1 + abs(a)) |
+--------------+
|            6 |
|            2 |
|            3 |
|            4 |
|            5 |
+--------------+
SELECT 5 rows in set (1.017 sec)

Another option (regarding point 2.) would be to return ?column? as postgres does for anything else than a FunctionCall so the above issue is resolved.

Regarding arithmetic operators and comparison operators we return the whole expression:

That's okay for now.

Another issue with the current approach is that if a FunctionCall is nested within let's say an arithmetic operator, then we still print out the whole function call. e.g.:

Should also be fine.

Another option (regarding point 2.) would be to return ?column? as postgres

We can do that if we encounter a concrete compatibility issue. Until then I'd keep this limited to handling the FunctionCall in a special way.

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #17858 has been dequeued, merge conditions unmatch:

• label=ready-to-merge
• any of [🛡 GitHub branch protection]:
  • check-neutral = ci/jenkins/pr_tests
  • check-skipped = ci/jenkins/pr_tests
  • check-success = ci/jenkins/pr_tests
• #approved-reviews-by >= 1 [🛡 GitHub branch protection]
• #approved-reviews-by>=1
• #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

retest this please

That is an excellent improvement that will boost ecosystem compatibility on a very important detail. Thank you so much! 💯