Think it should be describes the functions input and not output. Please can you confirm.

Agreed. Descriptor arguments represent the column name/types of the input not the output. Ex.

exclude_columns(input => table, columns => descriptor) → table
We check to make sure that the columns are present in the table.

        if (!inputNames.containsAll(excludedNames)) {
            String missingColumns = Sets.difference(excludedNames, inputNames).stream()
                    .collect(joining(", ", "[", "]"));
            throw new TrinoException(INVALID_FUNCTION_ARGUMENT, format("Excluded columns: %s not present in the table", missingColumns));
        }

Updated comment.

Done

Can you write an introductory paragraph about the usage of this class ?

Done.

Why is field name optional ? What's the use-case for it ?

It's used in ReturnType to return anonymous columns.

Makes sense. Can you leave a comment about this in the class ?

Guess you'll are adding Builders for all classes, else having a one field Builder doesn't seem worth it.

Yes, for example, ScalarArgument, DescriptorArgument, TableArgument all have builders. So for consistency we have one here.

Can you remove this code if not needed ?

Done

What do you do with the IGNORED column ? Can we remove it if not required ?

We do nothing with the column. It is an optional column used in the test and will be ingored during table function analysis.

Remove this comment if not needed.

Done

Remove this code ?

Done

Why do we need such a special class in the SPI ? CatalogSchemaFunctionName can have all 3 as individual elements.

Makes sense. Can you leave a comment about this in the class ?

Would it make sense to move this check before we validate all the table functions.
Something simple like,

if (tableFunctions.containsKey(catalogName) {
     error out saying ("Table functions already registered for catalog:");
}

Addressed in new PR

Not a huge fan of returning null, is it possible to change the method to return an Optional instead?

This error code is too vague, i think the name needs to be tied to the table function registry for clarity

Updated to SESSION_CATALOG_NOT_SET

its not clear yo me but looks like only scalar arguments work and table within table won't so can we fail gracefully for unsupported cases?

Why do we need this rule and corresponding SPI calls? I think we can use Connector optimizers (https://prestodb.io/blog/2019/12/23/improve-presto-planner/) which are present in presto and not trino so we can potentially skip a lot of additions to metadata?

cc: @tdcmeehan

I guess i get the changes here, metadata might be the better interface to override in this case

Not sure we need this, by default it will throw unsupported

This code seems inefficient having 2 for loops doing same thing

`Set seenNames = new HashSet<>();
int tableArgumentsWithRowSemantics = 0;

for (ArgumentSpecification spec : tableFunction.getArguments()) {
    // Check for duplicate names
    if (!seenNames.add(spec.getName())) {
        throw new IllegalArgumentException("duplicate argument name: " + spec.getName());
    }
    
    // Count table arguments with row semantics
    if (spec instanceof TableArgumentSpecification && 
        ((TableArgumentSpecification) spec).isRowSemantics()) {
        tableArgumentsWithRowSemantics++;
    }
}

checkArgument(tableArgumentsWithRowSemantics <= 1, "more than one table argument with row semantics");

`

Good catch. We could also exit early if tableArgumentsWithRowSemantics > 1 but I think we should keep it where it throws all duplicate arguments first. So followed your example.