Add builders for table function arguments #12350
Conversation
core/trino-spi/src/main/java/io/trino/spi/ptf/DescriptorArgument.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public DescriptorArgumentSpecification(String name, Descriptor defaultValue) | ||
| public static Builder descriptorArgumentBuilder(String name) |
There was a problem hiding this comment.
I think putting required fields in a builder() constructor kind of defeats of the purposes of a builder, which is readability ("named parameters"). I know we do so in some places, but i don't think we should, especially for code that's written very rarely and read much more often, as the one here.
There was a problem hiding this comment.
I saw that pattern in Trino and found it good because it keeps the usage concise. Also, it gives the notion that name is not optional, while the other field is.
There was a problem hiding this comment.
You can enforce optionality by throwing in build(). Yet I came ok with SINGLE argument passed to builder() method as we have here.
There was a problem hiding this comment.
It is enforced anyway by throwing in the DescriptorArgumentSpecification constructor. I wasn't trying here to enforce the semantics, but to convey it clearly to the user of the builder. If you still think that builder shouldn't take name in the constructor, I'll remove it.
There was a problem hiding this comment.
Up to me it is fine to have this arg (as stated above) :)
There was a problem hiding this comment.
@findepi is it acceptable for you to leave the argument?
There was a problem hiding this comment.
While rebasing, I decided to remove the argument name from the Builder constructor. The code didn't render good that way.
core/trino-spi/src/main/java/io/trino/spi/ptf/ScalarArgument.java
Outdated
Show resolved
Hide resolved
|
|
||
| public ScalarArgument build() | ||
| { | ||
| return new ScalarArgument(type, value); |
There was a problem hiding this comment.
That looks like a typed value. I wouldn't foresee additional parameters here, and so the builder looks like an overkill
There was a problem hiding this comment.
Not really. At some point, we will allow arbitrary expressions as scalar arguments.
core/trino-spi/src/main/java/io/trino/spi/ptf/TableArgument.java
Outdated
Show resolved
Hide resolved
| return this; | ||
| } | ||
|
|
||
| public Builder pruneWhenEmpty(boolean pruneWhenEmpty) |
There was a problem hiding this comment.
Generally I prefer boolean methods to have a good default and then a no-arg method to change tha behavior, so I would make this:
public Builder pruneWhenEmpty()
{
this.pruneWhenEmpty = true;
return this;
}
There was a problem hiding this comment.
It was the initial approach. I changed it according to #12350 (comment)
Adds builders for
Makes constructors of those classes private.
This is a SPI change. No release notes needed if this change is merged before the upcoming release (the main change has not been yet released)
This change has to be taken into account in the documentation (#12338)