[DISCUSS] FLIP-162 follow-up discussion

[Leonard Xu]

Hi, all

As the FLIP-162 discussed,  we agreed current time functions’ behavior is incorrect and plan to introduce the option table.exec.fallback-legacy-time-function to enable user fallback to incorrect behavior.

(1) The option is convenient for users who want to upgrade to 1.13 but don't want to change their sql job, user need to config the option value, this is the first time users influenced by these wrong functions.

(2) But we didn’t consider that the option will be deleted after one or two major versions, users have to change their sql job again at that time point, this the second time users influenced by these wrong functions.

(3) Besides, maintaining two sets of functions is prone to bugs.

I’ve discussed with some community developers offline, they tend to solve these functions at once i.e. Correct the wrong functions directly and do not introduce this option.

Considering that we will delete the configuration eventually,  comparing hurting users twice and bothering them for a long time, I would rather hurt users once.
Thus I also +1 that we should directly correct these wrong functions and remove the wrong functions at the same time.


If we can make a consensus in this thread, I think we can remove this option support in FLIP-162.
How do you think?

Best,
Leonard

[Kurt Young]

 Hi Leonard,

Thanks for this careful consideration. Given the fallback option will
eventually change the behavior twice, which means
potentially break user's job twice, I would also +1 to not introduce it.

Best,
Kurt

On Fri, Mar 5, 2021 at 3:00 PM Leonard Xu <[hidden email]> wrote:

> Hi, all
>
> As the FLIP-162 discussed,  we agreed current time functions’ behavior is
> incorrect and plan to introduce the option *t**able.exec.fallback-legacy-time-function
> *to enable user fallback to incorrect behavior.
>
> (1) The option is convenient for users who want to upgrade to 1.13 but
> don't want to change their sql job, user need to config the option value, *this
> is the first time users influenced by these wrong functions.*
>
> (2) But we didn’t consider that the option will be deleted after one or
> two major versions, users have to change their sql job again at that time
> point, *this the second time** users influenced by these wrong functions.*
>
> (3) Besides, maintaining two sets of functions is prone to bugs.
>
> I’ve discussed with some community developers offline, they tend to solve
> these functions at once i.e. Correct the wrong functions directly and do
> not introduce this option.
>
> Considering that we will delete the configuration eventually,  comparing
> hurting users twice and bothering them for a long time, I would rather hurt
> users once.
> *Thus I also +1* that we should directly correct these wrong functions
> and remove the wrong functions at the same time.
>
>
> If we can make a consensus in this thread, I think we can remove this
> option support in FLIP-162.
> How do you think?
>
> Best,
> Leonard
>
>
>
>
>

[Jark Wu-2]

Thanks Leonard,

I'm also +1 with not introducing this fallback option.
It's error-prone to mix the implementation of wrong behavior and correct
behavior.
And it's better to educate users the right way in one version instead of
spanning multiple versions.

Best,
Jark

On Tue, 9 Mar 2021 at 15:15, Kurt Young <[hidden email]> wrote:

> Hi Leonard,
>
> Thanks for this careful consideration. Given the fallback option will
> eventually change the behavior twice, which means
> potentially break user's job twice, I would also +1 to not introduce it.
>
> Best,
> Kurt
>
> On Fri, Mar 5, 2021 at 3:00 PM Leonard Xu <[hidden email]> wrote:
>
>> Hi, all
>>
>> As the FLIP-162 discussed,  we agreed current time functions’ behavior is
>> incorrect and plan to introduce the option *t**able.exec.fallback-legacy-time-function
>> *to enable user fallback to incorrect behavior.
>>
>> (1) The option is convenient for users who want to upgrade to 1.13 but
>> don't want to change their sql job, user need to config the option value, *this
>> is the first time users influenced by these wrong functions.*
>>
>> (2) But we didn’t consider that the option will be deleted after one or
>> two major versions, users have to change their sql job again at that time
>> point, *this the second time** users influenced by these wrong
>> functions.*
>>
>> (3) Besides, maintaining two sets of functions is prone to bugs.
>>
>> I’ve discussed with some community developers offline, they tend to solve
>> these functions at once i.e. Correct the wrong functions directly and do
>> not introduce this option.
>>
>> Considering that we will delete the configuration eventually,  comparing
>> hurting users twice and bothering them for a long time, I would rather hurt
>> users once.
>> *Thus I also +1* that we should directly correct these wrong functions
>> and remove the wrong functions at the same time.
>>
>>
>> If we can make a consensus in this thread, I think we can remove this
>> option support in FLIP-162.
>> How do you think?
>>
>> Best,
>> Leonard
>>
>>
>>
>>
>>

[Timo Walther-2]

Hi Leonard,

I'm fine with dropping the old buggy behavior immediatly. Users can
still implement a UDF with the old bavhior if needed. I hope the new
functions will be well-tested so that a fallback to the old functions is
not necessary as a workaround. It will definitely avoid confusion for
users and avoid spaghetti code in the planner module.

Regards,
Timo

On 09.03.21 08:14, Kurt Young wrote:

>   Hi Leonard,
>
> Thanks for this careful consideration. Given the fallback option will
> eventually change the behavior twice, which means
> potentially break user's job twice, I would also +1 to not introduce it.
>
> Best,
> Kurt
>
> On Fri, Mar 5, 2021 at 3:00 PM Leonard Xu <[hidden email]> wrote:
>
>> Hi, all
>>
>> As the FLIP-162 discussed,  we agreed current time functions’ behavior is
>> incorrect and plan to introduce the option *t**able.exec.fallback-legacy-time-function
>> *to enable user fallback to incorrect behavior.
>>
>> (1) The option is convenient for users who want to upgrade to 1.13 but
>> don't want to change their sql job, user need to config the option value, *this
>> is the first time users influenced by these wrong functions.*
>>
>> (2) But we didn’t consider that the option will be deleted after one or
>> two major versions, users have to change their sql job again at that time
>> point, *this the second time** users influenced by these wrong functions.*
>>
>> (3) Besides, maintaining two sets of functions is prone to bugs.
>>
>> I’ve discussed with some community developers offline, they tend to solve
>> these functions at once i.e. Correct the wrong functions directly and do
>> not introduce this option.
>>
>> Considering that we will delete the configuration eventually,  comparing
>> hurting users twice and bothering them for a long time, I would rather hurt
>> users once.
>> *Thus I also +1* that we should directly correct these wrong functions
>> and remove the wrong functions at the same time.
>>
>>
>> If we can make a consensus in this thread, I think we can remove this
>> option support in FLIP-162.
>> How do you think?
>>
>> Best,
>> Leonard
>>
>>
>>
>>
>>
>

[Jingsong Li]

+1

Let's go straight to the right behavior. Drop the option for the wrong
behavior.

Best,
Jingsong


On Tue, Mar 9, 2021 at 4:29 PM Timo Walther <[hidden email]> wrote:

> Hi Leonard,
>
> I'm fine with dropping the old buggy behavior immediatly. Users can
> still implement a UDF with the old bavhior if needed. I hope the new
> functions will be well-tested so that a fallback to the old functions is
> not necessary as a workaround. It will definitely avoid confusion for
> users and avoid spaghetti code in the planner module.
>
> Regards,
> Timo
>
> On 09.03.21 08:14, Kurt Young wrote:
> >   Hi Leonard,
> >
> > Thanks for this careful consideration. Given the fallback option will
> > eventually change the behavior twice, which means
> > potentially break user's job twice, I would also +1 to not introduce it.
> >
> > Best,
> > Kurt
> >
> > On Fri, Mar 5, 2021 at 3:00 PM Leonard Xu <[hidden email]> wrote:
> >
> >> Hi, all
> >>
> >> As the FLIP-162 discussed,  we agreed current time functions’ behavior
> is
> >> incorrect and plan to introduce the option
> *t**able.exec.fallback-legacy-time-function
> >> *to enable user fallback to incorrect behavior.
> >>
> >> (1) The option is convenient for users who want to upgrade to 1.13 but
> >> don't want to change their sql job, user need to config the option
> value, *this
> >> is the first time users influenced by these wrong functions.*
> >>
> >> (2) But we didn’t consider that the option will be deleted after one or
> >> two major versions, users have to change their sql job again at that
> time
> >> point, *this the second time** users influenced by these wrong
> functions.*
> >>
> >> (3) Besides, maintaining two sets of functions is prone to bugs.
> >>
> >> I’ve discussed with some community developers offline, they tend to
> solve
> >> these functions at once i.e. Correct the wrong functions directly and do
> >> not introduce this option.
> >>
> >> Considering that we will delete the configuration eventually,  comparing
> >> hurting users twice and bothering them for a long time, I would rather
> hurt
> >> users once.
> >> *Thus I also +1* that we should directly correct these wrong functions
> >> and remove the wrong functions at the same time.
> >>
> >>
> >> If we can make a consensus in this thread, I think we can remove this
> >> option support in FLIP-162.
> >> How do you think?
> >>
> >> Best,
> >> Leonard
> >>
> >>
> >>
> >>
> >>
> >
>
>

--
Best, Jingsong Lee

[Leonard Xu]

Thank you all for your positive feedbacks.

Considering that you’re binding votes for this FLIP and all of us agreed to remove this option,
Thus I update this section of FLIP-162[1], I’ll start work on it next .


Best,
Leonard
[1] https://cwiki.apache.org/confluence/display/FLINK/FLIP-162%3A+Consistent+Flink+SQL+time+function+behavior <https://cwiki.apache.org/confluence/display/FLINK/FLIP-162:+Consistent+Flink+SQL+time+function+behavior>

> 在 2021年3月9日，16:34，Jingsong Li <[hidden email]> 写道：
>
> +1
>
> Let's go straight to the right behavior. Drop the option for the wrong
> behavior.
>
> Best,
> Jingsong
>
>
> On Tue, Mar 9, 2021 at 4:29 PM Timo Walther <[hidden email]> wrote:
>
>> Hi Leonard,
>>
>> I'm fine with dropping the old buggy behavior immediatly. Users can
>> still implement a UDF with the old bavhior if needed. I hope the new
>> functions will be well-tested so that a fallback to the old functions is
>> not necessary as a workaround. It will definitely avoid confusion for
>> users and avoid spaghetti code in the planner module.
>>
>> Regards,
>> Timo
>>
>> On 09.03.21 08:14, Kurt Young wrote:
>>>  Hi Leonard,
>>>
>>> Thanks for this careful consideration. Given the fallback option will
>>> eventually change the behavior twice, which means
>>> potentially break user's job twice, I would also +1 to not introduce it.
>>>
>>> Best,
>>> Kurt
>>>
>>> On Fri, Mar 5, 2021 at 3:00 PM Leonard Xu <[hidden email]> wrote:
>>>
>>>> Hi, all
>>>>
>>>> As the FLIP-162 discussed,  we agreed current time functions’ behavior
>> is
>>>> incorrect and plan to introduce the option
>> *t**able.exec.fallback-legacy-time-function
>>>> *to enable user fallback to incorrect behavior.
>>>>
>>>> (1) The option is convenient for users who want to upgrade to 1.13 but
>>>> don't want to change their sql job, user need to config the option
>> value, *this
>>>> is the first time users influenced by these wrong functions.*
>>>>
>>>> (2) But we didn’t consider that the option will be deleted after one or
>>>> two major versions, users have to change their sql job again at that
>> time
>>>> point, *this the second time** users influenced by these wrong
>> functions.*
>>>>
>>>> (3) Besides, maintaining two sets of functions is prone to bugs.
>>>>
>>>> I’ve discussed with some community developers offline, they tend to
>> solve
>>>> these functions at once i.e. Correct the wrong functions directly and do
>>>> not introduce this option.
>>>>
>>>> Considering that we will delete the configuration eventually,  comparing
>>>> hurting users twice and bothering them for a long time, I would rather
>> hurt
>>>> users once.
>>>> *Thus I also +1* that we should directly correct these wrong functions
>>>> and remove the wrong functions at the same time.
>>>>
>>>>
>>>> If we can make a consensus in this thread, I think we can remove this
>>>> option support in FLIP-162.
>>>> How do you think?
>>>>
>>>> Best,
>>>> Leonard
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>
> --
> Best, Jingsong Lee