[DISCUSS] FLIP-51: Rework of the Expression Design

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[DISCUSS] FLIP-51: Rework of the Expression Design

JingsongLee-2
Hi everyone,

We would like to start a discussion thread on "FLIP-51: Rework of the
Expression Design"(Design doc: [1], FLIP: [2]), where we describe how
 to improve the new java Expressions to work with type inference and
 convert expression to the calcite RexNode. This is a follow-up plan
for FLIP-32[3] and FLIP-37[4]. This FLIP is mostly based on FLIP-37.

This FLIP addresses several shortcomings of current:
   - New Expressions still use PlannerExpressions to type inference and
 to RexNode. Flnk-planner and blink-planner have a lot of repetitive code
 and logic.
   - Let TableApi and Cacite definitions consistent.
   - Reduce the complexity of Function development.
   - Powerful Function for user.

Key changes can be summarized as follows:
   - Improve the interface of FunctionDefinition.
   - Introduce type inference for built-in functions.
   - Introduce ExpressionConverter to convert Expression to calcite
 RexNode.
   - Remove repetitive code and logic in planners.

I also listed type inference and behavior of all built-in functions [5], to
verify that the interface is satisfied. After introduce type inference to
table-common module, planners should have a unified function behavior.
And this gives the community also the chance to quickly discuss types
 and behavior of functions a last time before they are declared stable.

Looking forward to your feedbacks. Thank you.

[1] https://docs.google.com/document/d/1yFDyquMo_-VZ59vyhaMshpPtg7p87b9IYdAtMXv5XmM/edit?usp=sharing
[2] https://cwiki.apache.org/confluence/display/FLINK/FLIP-51%3A+Rework+of+the+Expression+Design
[3] https://cwiki.apache.org/confluence/display/FLINK/FLIP-32%3A+Restructure+flink-table+for+future+contributions
[4] https://cwiki.apache.org/confluence/display/FLINK/FLIP-37%3A+Rework+of+the+Table+API+Type+System
[5] https://docs.google.com/document/d/1fyVmdGgbO1XmIyQ1BaoG_h5BcNcF3q9UJ1Bj1euO240/edit?usp=sharing

Best,
Jingsong Lee
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] FLIP-51: Rework of the Expression Design

Timo Walther-2
Hi Jingsong,

thanks for writing down this FLIP. Big +1 from my side to finally get
rid of PlannerExpressions and have consistent and well-defined behavior
for Table API and SQL updated to FLIP-37.

We might need to discuss some of the behavior of particular functions
but this should not affect the actual FLIP-51.

Regards,
Timo


Am 13.08.19 um 12:55 schrieb JingsongLee:

> Hi everyone,
>
> We would like to start a discussion thread on "FLIP-51: Rework of the
> Expression Design"(Design doc: [1], FLIP: [2]), where we describe how
>   to improve the new java Expressions to work with type inference and
>   convert expression to the calcite RexNode. This is a follow-up plan
> for FLIP-32[3] and FLIP-37[4]. This FLIP is mostly based on FLIP-37.
>
> This FLIP addresses several shortcomings of current:
>     - New Expressions still use PlannerExpressions to type inference and
>   to RexNode. Flnk-planner and blink-planner have a lot of repetitive code
>   and logic.
>     - Let TableApi and Cacite definitions consistent.
>     - Reduce the complexity of Function development.
>     - Powerful Function for user.
>
> Key changes can be summarized as follows:
>     - Improve the interface of FunctionDefinition.
>     - Introduce type inference for built-in functions.
>     - Introduce ExpressionConverter to convert Expression to calcite
>   RexNode.
>     - Remove repetitive code and logic in planners.
>
> I also listed type inference and behavior of all built-in functions [5], to
> verify that the interface is satisfied. After introduce type inference to
> table-common module, planners should have a unified function behavior.
> And this gives the community also the chance to quickly discuss types
>   and behavior of functions a last time before they are declared stable.
>
> Looking forward to your feedbacks. Thank you.
>
> [1] https://docs.google.com/document/d/1yFDyquMo_-VZ59vyhaMshpPtg7p87b9IYdAtMXv5XmM/edit?usp=sharing
> [2] https://cwiki.apache.org/confluence/display/FLINK/FLIP-51%3A+Rework+of+the+Expression+Design
> [3] https://cwiki.apache.org/confluence/display/FLINK/FLIP-32%3A+Restructure+flink-table+for+future+contributions
> [4] https://cwiki.apache.org/confluence/display/FLINK/FLIP-37%3A+Rework+of+the+Table+API+Type+System
> [5] https://docs.google.com/document/d/1fyVmdGgbO1XmIyQ1BaoG_h5BcNcF3q9UJ1Bj1euO240/edit?usp=sharing
>
> Best,
> Jingsong Lee


Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] FLIP-51: Rework of the Expression Design

Jark Wu-2
Thanks Jingsong for starting the discussion.

The general design of the FLIP looks good to me. +1 for the FLIP. It's time
to get rid of the old Expression!

Regarding to the function behavior, shall we also include new functions
from blink planner (e.g. LISTAGG, REGEXP, TO_DATE, etc..) ?


Best,
Jark





On Wed, 14 Aug 2019 at 23:34, Timo Walther <[hidden email]> wrote:

> Hi Jingsong,
>
> thanks for writing down this FLIP. Big +1 from my side to finally get
> rid of PlannerExpressions and have consistent and well-defined behavior
> for Table API and SQL updated to FLIP-37.
>
> We might need to discuss some of the behavior of particular functions
> but this should not affect the actual FLIP-51.
>
> Regards,
> Timo
>
>
> Am 13.08.19 um 12:55 schrieb JingsongLee:
> > Hi everyone,
> >
> > We would like to start a discussion thread on "FLIP-51: Rework of the
> > Expression Design"(Design doc: [1], FLIP: [2]), where we describe how
> >   to improve the new java Expressions to work with type inference and
> >   convert expression to the calcite RexNode. This is a follow-up plan
> > for FLIP-32[3] and FLIP-37[4]. This FLIP is mostly based on FLIP-37.
> >
> > This FLIP addresses several shortcomings of current:
> >     - New Expressions still use PlannerExpressions to type inference and
> >   to RexNode. Flnk-planner and blink-planner have a lot of repetitive
> code
> >   and logic.
> >     - Let TableApi and Cacite definitions consistent.
> >     - Reduce the complexity of Function development.
> >     - Powerful Function for user.
> >
> > Key changes can be summarized as follows:
> >     - Improve the interface of FunctionDefinition.
> >     - Introduce type inference for built-in functions.
> >     - Introduce ExpressionConverter to convert Expression to calcite
> >   RexNode.
> >     - Remove repetitive code and logic in planners.
> >
> > I also listed type inference and behavior of all built-in functions [5],
> to
> > verify that the interface is satisfied. After introduce type inference to
> > table-common module, planners should have a unified function behavior.
> > And this gives the community also the chance to quickly discuss types
> >   and behavior of functions a last time before they are declared stable.
> >
> > Looking forward to your feedbacks. Thank you.
> >
> > [1]
> https://docs.google.com/document/d/1yFDyquMo_-VZ59vyhaMshpPtg7p87b9IYdAtMXv5XmM/edit?usp=sharing
> > [2]
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-51%3A+Rework+of+the+Expression+Design
> > [3]
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-32%3A+Restructure+flink-table+for+future+contributions
> > [4]
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-37%3A+Rework+of+the+Table+API+Type+System
> > [5]
> https://docs.google.com/document/d/1fyVmdGgbO1XmIyQ1BaoG_h5BcNcF3q9UJ1Bj1euO240/edit?usp=sharing
> >
> > Best,
> > Jingsong Lee
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] FLIP-51: Rework of the Expression Design

JingsongLee-2
Hi jark:

I'll add a chapter to list blink planner extended functions.

Best,
 Jingsong Lee


------------------------------------------------------------------
From:Jark Wu <[hidden email]>
Send Time:2019年8月15日(星期四) 05:12
To:dev <[hidden email]>
Subject:Re: [DISCUSS] FLIP-51: Rework of the Expression Design

Thanks Jingsong for starting the discussion.

The general design of the FLIP looks good to me. +1 for the FLIP. It's time
to get rid of the old Expression!

Regarding to the function behavior, shall we also include new functions
from blink planner (e.g. LISTAGG, REGEXP, TO_DATE, etc..) ?


Best,
Jark





On Wed, 14 Aug 2019 at 23:34, Timo Walther <[hidden email]> wrote:

> Hi Jingsong,
>
> thanks for writing down this FLIP. Big +1 from my side to finally get
> rid of PlannerExpressions and have consistent and well-defined behavior
> for Table API and SQL updated to FLIP-37.
>
> We might need to discuss some of the behavior of particular functions
> but this should not affect the actual FLIP-51.
>
> Regards,
> Timo
>
>
> Am 13.08.19 um 12:55 schrieb JingsongLee:
> > Hi everyone,
> >
> > We would like to start a discussion thread on "FLIP-51: Rework of the
> > Expression Design"(Design doc: [1], FLIP: [2]), where we describe how
> >   to improve the new java Expressions to work with type inference and
> >   convert expression to the calcite RexNode. This is a follow-up plan
> > for FLIP-32[3] and FLIP-37[4]. This FLIP is mostly based on FLIP-37.
> >
> > This FLIP addresses several shortcomings of current:
> >     - New Expressions still use PlannerExpressions to type inference and
> >   to RexNode. Flnk-planner and blink-planner have a lot of repetitive
> code
> >   and logic.
> >     - Let TableApi and Cacite definitions consistent.
> >     - Reduce the complexity of Function development.
> >     - Powerful Function for user.
> >
> > Key changes can be summarized as follows:
> >     - Improve the interface of FunctionDefinition.
> >     - Introduce type inference for built-in functions.
> >     - Introduce ExpressionConverter to convert Expression to calcite
> >   RexNode.
> >     - Remove repetitive code and logic in planners.
> >
> > I also listed type inference and behavior of all built-in functions [5],
> to
> > verify that the interface is satisfied. After introduce type inference to
> > table-common module, planners should have a unified function behavior.
> > And this gives the community also the chance to quickly discuss types
> >   and behavior of functions a last time before they are declared stable.
> >
> > Looking forward to your feedbacks. Thank you.
> >
> > [1]
> https://docs.google.com/document/d/1yFDyquMo_-VZ59vyhaMshpPtg7p87b9IYdAtMXv5XmM/edit?usp=sharing
> > [2]
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-51%3A+Rework+of+the+Expression+Design
> > [3]
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-32%3A+Restructure+flink-table+for+future+contributions
> > [4]
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-37%3A+Rework+of+the+Table+API+Type+System
> > [5]
> https://docs.google.com/document/d/1fyVmdGgbO1XmIyQ1BaoG_h5BcNcF3q9UJ1Bj1euO240/edit?usp=sharing
> >
> > Best,
> > Jingsong Lee
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] FLIP-51: Rework of the Expression Design

JingsongLee-2
Hi @Timo Walther @Dawid Wysakowicz:

Now, flink-planner have some legacy DataTypes:
like: legacy decimal, legacy basic array type info...
And If the new type inference infer a Decimal/VarChar with precision, there should will fail in TypeConversions.

The better we do on DataType, the more problems we will have with TypeInformation conversion, and the new TypeInference is a lot of precision related.
What do you think?
1. Should TypeConversions support all data types and flink-planner support new types?2. Or do a special conversion between flink-planner and type inference.

(There are many problems with the conversion between TypeInformation and DataType, and I think we should solve them completely in 1.10.)

Best,
Jingsong Lee


------------------------------------------------------------------
From:JingsongLee <[hidden email]>
Send Time:2019年8月15日(星期四) 10:31
To:dev <[hidden email]>
Subject:Re: [DISCUSS] FLIP-51: Rework of the Expression Design

Hi jark:

I'll add a chapter to list blink planner extended functions.

Best,
 Jingsong Lee


------------------------------------------------------------------
From:Jark Wu <[hidden email]>
Send Time:2019年8月15日(星期四) 05:12
To:dev <[hidden email]>
Subject:Re: [DISCUSS] FLIP-51: Rework of the Expression Design

Thanks Jingsong for starting the discussion.

The general design of the FLIP looks good to me. +1 for the FLIP. It's time
to get rid of the old Expression!

Regarding to the function behavior, shall we also include new functions
from blink planner (e.g. LISTAGG, REGEXP, TO_DATE, etc..) ?


Best,
Jark





On Wed, 14 Aug 2019 at 23:34, Timo Walther <[hidden email]> wrote:

> Hi Jingsong,
>
> thanks for writing down this FLIP. Big +1 from my side to finally get
> rid of PlannerExpressions and have consistent and well-defined behavior
> for Table API and SQL updated to FLIP-37.
>
> We might need to discuss some of the behavior of particular functions
> but this should not affect the actual FLIP-51.
>
> Regards,
> Timo
>
>
> Am 13.08.19 um 12:55 schrieb JingsongLee:
> > Hi everyone,
> >
> > We would like to start a discussion thread on "FLIP-51: Rework of the
> > Expression Design"(Design doc: [1], FLIP: [2]), where we describe how
> >   to improve the new java Expressions to work with type inference and
> >   convert expression to the calcite RexNode. This is a follow-up plan
> > for FLIP-32[3] and FLIP-37[4]. This FLIP is mostly based on FLIP-37.
> >
> > This FLIP addresses several shortcomings of current:
> >     - New Expressions still use PlannerExpressions to type inference and
> >   to RexNode. Flnk-planner and blink-planner have a lot of repetitive
> code
> >   and logic.
> >     - Let TableApi and Cacite definitions consistent.
> >     - Reduce the complexity of Function development.
> >     - Powerful Function for user.
> >
> > Key changes can be summarized as follows:
> >     - Improve the interface of FunctionDefinition.
> >     - Introduce type inference for built-in functions.
> >     - Introduce ExpressionConverter to convert Expression to calcite
> >   RexNode.
> >     - Remove repetitive code and logic in planners.
> >
> > I also listed type inference and behavior of all built-in functions [5],
> to
> > verify that the interface is satisfied. After introduce type inference to
> > table-common module, planners should have a unified function behavior.
> > And this gives the community also the chance to quickly discuss types
> >   and behavior of functions a last time before they are declared stable.
> >
> > Looking forward to your feedbacks. Thank you.
> >
> > [1]
> https://docs.google.com/document/d/1yFDyquMo_-VZ59vyhaMshpPtg7p87b9IYdAtMXv5XmM/edit?usp=sharing
> > [2]
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-51%3A+Rework+of+the+Expression+Design
> > [3]
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-32%3A+Restructure+flink-table+for+future+contributions
> > [4]
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-37%3A+Rework+of+the+Table+API+Type+System
> > [5]
> https://docs.google.com/document/d/1fyVmdGgbO1XmIyQ1BaoG_h5BcNcF3q9UJ1Bj1euO240/edit?usp=sharing
> >
> > Best,
> > Jingsong Lee
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] FLIP-51: Rework of the Expression Design

Timo Walther-2
Hi,

regarding the LegacyTypeInformation esp. for decimals. I don't have a
clear answer yet, but I think it should not limit us. If possible it
should travel through the type inference and we only need some special
cases at some locations e.g. when computing the leastRestrictive. E.g.
the logical type root is set correctly which is required for family
checking.

I'm wondering when a decimal type with precision can occur. Usually, it
should come from literals or defined column. But I think it might also
be ok that the flink-planner just receives a decimal with precision and
treats it with Java semantics.

Doing it on the planner side is the easiest option but also the one that
could cause side-effects if the back-and-forth conversion of
LegacyTypeConverter is not a 1:1 mapping anymore. I guess we will see
the implications during implementation. All old Flink tests should pass
still.

Regards,
Timo

Am 15.08.19 um 10:43 schrieb JingsongLee:

> Hi @Timo Walther @Dawid Wysakowicz:
>
> Now, flink-planner have some legacy DataTypes:
> like: legacy decimal, legacy basic array type info...
> And If the new type inference infer a Decimal/VarChar with precision, there should will fail in TypeConversions.
>
> The better we do on DataType, the more problems we will have with TypeInformation conversion, and the new TypeInference is a lot of precision related.
> What do you think?
> 1. Should TypeConversions support all data types and flink-planner support new types?2. Or do a special conversion between flink-planner and type inference.
>
> (There are many problems with the conversion between TypeInformation and DataType, and I think we should solve them completely in 1.10.)
>
> Best,
> Jingsong Lee
>
>
> ------------------------------------------------------------------
> From:JingsongLee <[hidden email]>
> Send Time:2019年8月15日(星期四) 10:31
> To:dev <[hidden email]>
> Subject:Re: [DISCUSS] FLIP-51: Rework of the Expression Design
>
> Hi jark:
>
> I'll add a chapter to list blink planner extended functions.
>
> Best,
>   Jingsong Lee
>
>
> ------------------------------------------------------------------
> From:Jark Wu <[hidden email]>
> Send Time:2019年8月15日(星期四) 05:12
> To:dev <[hidden email]>
> Subject:Re: [DISCUSS] FLIP-51: Rework of the Expression Design
>
> Thanks Jingsong for starting the discussion.
>
> The general design of the FLIP looks good to me. +1 for the FLIP. It's time
> to get rid of the old Expression!
>
> Regarding to the function behavior, shall we also include new functions
> from blink planner (e.g. LISTAGG, REGEXP, TO_DATE, etc..) ?
>
>
> Best,
> Jark
>
>
>
>
>
> On Wed, 14 Aug 2019 at 23:34, Timo Walther <[hidden email]> wrote:
>
>> Hi Jingsong,
>>
>> thanks for writing down this FLIP. Big +1 from my side to finally get
>> rid of PlannerExpressions and have consistent and well-defined behavior
>> for Table API and SQL updated to FLIP-37.
>>
>> We might need to discuss some of the behavior of particular functions
>> but this should not affect the actual FLIP-51.
>>
>> Regards,
>> Timo
>>
>>
>> Am 13.08.19 um 12:55 schrieb JingsongLee:
>>> Hi everyone,
>>>
>>> We would like to start a discussion thread on "FLIP-51: Rework of the
>>> Expression Design"(Design doc: [1], FLIP: [2]), where we describe how
>>>    to improve the new java Expressions to work with type inference and
>>>    convert expression to the calcite RexNode. This is a follow-up plan
>>> for FLIP-32[3] and FLIP-37[4]. This FLIP is mostly based on FLIP-37.
>>>
>>> This FLIP addresses several shortcomings of current:
>>>      - New Expressions still use PlannerExpressions to type inference and
>>>    to RexNode. Flnk-planner and blink-planner have a lot of repetitive
>> code
>>>    and logic.
>>>      - Let TableApi and Cacite definitions consistent.
>>>      - Reduce the complexity of Function development.
>>>      - Powerful Function for user.
>>>
>>> Key changes can be summarized as follows:
>>>      - Improve the interface of FunctionDefinition.
>>>      - Introduce type inference for built-in functions.
>>>      - Introduce ExpressionConverter to convert Expression to calcite
>>>    RexNode.
>>>      - Remove repetitive code and logic in planners.
>>>
>>> I also listed type inference and behavior of all built-in functions [5],
>> to
>>> verify that the interface is satisfied. After introduce type inference to
>>> table-common module, planners should have a unified function behavior.
>>> And this gives the community also the chance to quickly discuss types
>>>    and behavior of functions a last time before they are declared stable.
>>>
>>> Looking forward to your feedbacks. Thank you.
>>>
>>> [1]
>> https://docs.google.com/document/d/1yFDyquMo_-VZ59vyhaMshpPtg7p87b9IYdAtMXv5XmM/edit?usp=sharing
>>> [2]
>> https://cwiki.apache.org/confluence/display/FLINK/FLIP-51%3A+Rework+of+the+Expression+Design
>>> [3]
>> https://cwiki.apache.org/confluence/display/FLINK/FLIP-32%3A+Restructure+flink-table+for+future+contributions
>>> [4]
>> https://cwiki.apache.org/confluence/display/FLINK/FLIP-37%3A+Rework+of+the+Table+API+Type+System
>>> [5]
>> https://docs.google.com/document/d/1fyVmdGgbO1XmIyQ1BaoG_h5BcNcF3q9UJ1Bj1euO240/edit?usp=sharing
>>> Best,
>>> Jingsong Lee
>>
>>