Enforcing procedure name args only

[aczajkowski]

Enforcing using named arguments when calling procedure

[martint]

I believe this is a departure from the SQL spec, but I'd need to check the document.

[aczajkowski]

Ok Thx, let me know can we proceed with that. This is limiting spec, but i think in resonable way within its boundries.

[martint]

What's the motivation for this? Why would we ever need it?

[aczajkowski]

In case when we have all arguments optional and logic is dependent on set non-required arguments.
This would not cause backward compatibility issues if we will ever decide some new argument is added or required. Only documentation would need to be update.

Perfect example is current flush_metadata_cache procedure.
For now we allow below use cases

USE example_catalog.some_schema;

# Clear all caches in catalog 'example_catalog' for all schema
system.flush_metadata_cache(); 

# Clear all caches for specific partition 'partition_column=partition_value'
# in table 'example_catalog.example_schema.example_table'
system.flush_metadata_cache(
  schema_name => 'example_schema',
  table_name => 'example_table',
  partition_columns => ARRAY['partition_column'],
  partition_values => ARRAY['partition_value']
);

But at some point we might extend scope to

USE example_catalog.some_schema;

# Clear all caches for specific schema 'example_catalog.example_schema'
system.flush_metadata_cache(
  schema_name => 'example_schema'
);

# Clear all caches for specific table 'example_catalog.example_schema.example_table'
system.flush_metadata_cache(
  schema_name => 'example_schema',
  table_name => 'example_table'
);

Maybe even other use cases which are hard to predict at the moment.

CC: @findepi

[aczajkowski]

@martint @electrum applied requested changes + added background on this change motivation.
Let me know if this is ok to proceed ?

[aczajkowski]

@martint @electrum Rebased on top of master due to conflicts.

[aczajkowski]

@martint is there any update on this. Should i close this PR ?

[losipiuk]

@martint is there any update on this. Should i close this PR ?

@martint bump

[aczajkowski]

@martint is there any update on this. Should i close this PR ?

@martint bump

@martint any update around your concern here #10545 (comment)

[aczajkowski]

@martint @electrum @losipiuk bump

[ksobolew]

I like it. As @martint says it may be deviating from the SQL spec, but it makes sense in certain situations. In fact, there are entire languages which don't have positional arguments, only named ones.

[ksobolew]

Wow, that was a clever workaround. Glad that it's gone :D

[aczajkowski]

I like it. As @martint says it may be deviating from the SQL spec, but it makes sense in certain situations. In fact, there are entire languages which don't have positional arguments, only named ones.

Also we are not disallowing using positional arguments at all. It's just one procedure definition that disallows it's usage.
So our spec is compatible but in some cases we just limit it for a specific use case.

[kokosing]

It's just one procedure definition that disallows it's usage.

From what I see the given procedure was already like that, it already required named arguments. We just remove hack and have dedicated support for such procedures.

@martint let me know if you have any more comments, otherwise I would like to merge it

[martint]

One minor comment. Otherwise it looks good, and I'm ok with the change.

[martint]

Rename to requiresNamedArguments

[aczajkowski]

Applied.

[kokosing]

Merged, thanks!

[colebow]

Hey, do we think this needs a release note, or is it just better and more clear error handling?

cc @kokosing

[aczajkowski]

@colebow my opinion is that is just internal mechanics.