Hi Yash,
thanks for updating the FLIP.
Here is more feedback from my side:
Add `option(ConfigOption<T> configOption, T value)` similar to
`TableDescriptor`.
Use `comment()` instead of `setComment` to be in sync with
`TableDescriptor`.
In `TableDescriptor`, the schema is not part of the `newBuilder` but
offered as optional method `schema()`, this allows for omitting the
schema and automatically derive it from the input. Do we want to offer
the same functionality? We might want to offer a CREATE MODEL AS syntax
in Table API?
In `TableDescriptor.forConnector()` we make the connector option
mandatory, is there no similar mandatory option for models?
Similar to Leonard, I will not directly -1 to cancel existing voting
process, but I hope to continue voting after the addressed above. For
the next time please make sure that the DISCUSS thread has settled
before voting.
Thanks,
Timo
On 11.03.25 09:25, Leonard Xu wrote:
Thanks @Anand for the quick response and update, the updated FLIP looks clear
enough to me.
+1(binding)
Best,
Leonard
2025年3月11日 12:46，Yash Anand <yan...@confluent.io.INVALID> 写道：
Hi Leonard,
Thank you for your input. I will update the FLIP accordingly to make it
more clear and standardized enough.
Thanks,
Yash Anand
On Mon, Mar 10, 2025 at 11:08 PM Leonard Xu <xbjt...@gmail.com> wrote:
Sorry for jumping the thread late, but I think current status of this FLIP
is not ready, at least for me
(1) Could you finish your proposed API according Flink API bylaws? For
example the code piece should be:
Builder<SELF> {
SELF option(String key, String value
SELF setComment(@Nullable String comment);
=>
/** Builder for {@link ModelDescriptor}. **/
@PublicEvolving
Builder<SELF> {
/** Defines the option of {@link ModelDescriptor}. **/
SELF option(String key, String value);
/** Defines the comment of {@link ModelDescriptor}. **/
SELF setComment(@Nullable String comment);
(2) TableEnvironment is a public API, so any changes (such as adding
public methods in this case) must be clearly documented. You may refer to
[1] as an example. In the [Public Interfaces] section of this FLIP, only
TableEnvironment is listed. However, the subsequent [Proposed Changes]
section appears to conflate TableEnvironment with ModelDescriptor.
Clarifications are needed:
Which package should ModelDescriptor belong to?
Is ModelDescriptor intended to be an inner class of TableEnvironment?
At last, this is a useful FLIP and I generally agree with the motivation
and design, but it is not clear and standardized enough.
I will not directly -1 to cancel existing voting process, but I hope to
continue voting after the addressed above(1)(2) comments. WDYT?
Best,
Leonard
[1]
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=334760466
2025年3月11日 01:19，Yash Anand <yan...@confluent.io.INVALID> 写道：
Hi Timo,
Thanks for pointing that out. I have added the full API of
the ModelDescriptor in the FLIP.
Thanks,
Yash Anand
On Mon, Mar 10, 2025 at 11:27 AM Timo Walther <twal...@apache.org>
wrote:
Hi Yash,
could you provide the full API of the ModelDescriptor in the FLIP?
Thanks,
Timo
On 10.03.25 16:55, Mingge Deng wrote:
Thanks Yash!
+1 (binding)
Best,
Mingge
On Mon, Mar 10, 2025 at 8:51 AM Dawid Wysakowicz <
dwysakow...@apache.org
wrote:
+1 (binding)
Best,
Dawid
On Wed, 19 Feb 2025 at 18:37, Hao Li <h...@confluent.io.invalid>
wrote:
+1 (non-binding)
Thanks Yash,
Hao
On Tue, Feb 18, 2025 at 10:46 AM Yash Anand
<yan...@confluent.io.invalid
wrote:
Hi Everyone,
I'd like to start a vote on FLIP-507: Add Model DDL methods in TABLE
API
[1] which has been discussed in this thread [2].
The vote will be open for at least 72 hours unless there is an
objection
or
not enough votes.
[1]
https://cwiki.apache.org/confluence/display/FLINK/FLIP-507%3A+Add+Model+DDL+methods+in+TABLE+API
[2]
https://lists.apache.org/thread/w9dt6y1w0yns5j3g4685tstjdg5flvy9
