[DISCUSS] Make AppendingState#add refuse to add null element
Locked
[DISCUSS] Make AppendingState#add refuse to add null element
 
|
Dear All
Currently, we found the implementations of AppendingState#add are not the same, taking some as example: - HeapReducingState will clear state if add null element - RocksDBReducingState will add null element if serializer can serialize null - Both HeapListState and RocksDBListState refuse to add null element — will throw NullPointException we think this need to be fixed, and possible solutions include: 1. Respect the current java doc, which said “If null is passed in, the state value will remain unchanged” 2. Make all AppendingState#add refuse to add null element We propose to apply the second solution, following the recommendation in Guava[1]. Would love to hear your thoughts. Thanks. Regards, Congxian [1] https://github.com/google/guava/wiki/UsingAndAvoidingNullExplained |
Re: [DISCUSS] Make AppendingState#add refuse to add null element
|
|
Hi,
As I said in the discussion on the Jira issue, I’m in favour of this change! This is the Jira Issue, for reference: https://issues.apache.org/jira/browse/FLINK-15424 Best, Aljoscha > On 8. Jan 2020, at 15:16, Congxian Qiu <[hidden email]> wrote: > > Dear All > > > Currently, we found the implementations of AppendingState#add are not the > same, taking some as example: > > - HeapReducingState will clear state if add null element > - RocksDBReducingState will add null element if serializer can serialize > null > - Both HeapListState and RocksDBListState refuse to add null element — > will throw NullPointException > > > we think this need to be fixed, and possible solutions include: > > 1. Respect the current java doc, which said “If null is passed in, the > state value will remain unchanged” > 2. Make all AppendingState#add refuse to add null element > > > We propose to apply the second solution, following the recommendation in > Guava[1]. > > > Would love to hear your thoughts. Thanks. > > > Regards, > > Congxian > > > [1] https://github.com/google/guava/wiki/UsingAndAvoidingNullExplained |
|
|
+1 for unifying the behavior to refusing adding null element. Nice catch
and thanks for bringing up the discussion! Best Regards, Yu On Wed, 8 Jan 2020 at 22:50, Aljoscha Krettek <[hidden email]> wrote: > Hi, > > As I said in the discussion on the Jira issue, I’m in favour of this > change! > > This is the Jira Issue, for reference: > https://issues.apache.org/jira/browse/FLINK-15424 > > Best, > Aljoscha > > > On 8. Jan 2020, at 15:16, Congxian Qiu <[hidden email]> wrote: > > > > Dear All > > > > > > Currently, we found the implementations of AppendingState#add are not the > > same, taking some as example: > > > > - HeapReducingState will clear state if add null element > > - RocksDBReducingState will add null element if serializer can > serialize > > null > > - Both HeapListState and RocksDBListState refuse to add null element — > > will throw NullPointException > > > > > > we think this need to be fixed, and possible solutions include: > > > > 1. Respect the current java doc, which said “If null is passed in, the > > state value will remain unchanged” > > 2. Make all AppendingState#add refuse to add null element > > > > > > We propose to apply the second solution, following the recommendation in > > Guava[1]. > > > > > > Would love to hear your thoughts. Thanks. > > > > > > Regards, > > > > Congxian > > > > > > [1] https://github.com/google/guava/wiki/UsingAndAvoidingNullExplained > > |
|
|
+1 for unifying the behavior of AppendingState#add .
However, I have concern for the usage of window reducing function [1], I'm not sure whether user would rely on processing StreamRecord(null) to clear state. As you can see, user could not see the reducing window state directly, and the only way to communicate with state is via processing records. I'm not sure whether this is by design, @Aljoscha Krettek<mailto:[hidden email]> would you please share the initial idea when introducing this for the first time? [1] https://ci.apache.org/projects/flink/flink-docs-stable/dev/stream/operators/windows.html#reducefunction Best Yun Tang ________________________________ From: Yu Li <[hidden email]> Sent: Thursday, January 9, 2020 14:09 To: dev <[hidden email]> Subject: Re: [DISCUSS] Make AppendingState#add refuse to add null element +1 for unifying the behavior to refusing adding null element. Nice catch and thanks for bringing up the discussion! Best Regards, Yu On Wed, 8 Jan 2020 at 22:50, Aljoscha Krettek <[hidden email]> wrote: > Hi, > > As I said in the discussion on the Jira issue, I’m in favour of this > change! > > This is the Jira Issue, for reference: > https://issues.apache.org/jira/browse/FLINK-15424 > > Best, > Aljoscha > > > On 8. Jan 2020, at 15:16, Congxian Qiu <[hidden email]> wrote: > > > > Dear All > > > > > > Currently, we found the implementations of AppendingState#add are not the > > same, taking some as example: > > > > - HeapReducingState will clear state if add null element > > - RocksDBReducingState will add null element if serializer can > serialize > > null > > - Both HeapListState and RocksDBListState refuse to add null element — > > will throw NullPointException > > > > > > we think this need to be fixed, and possible solutions include: > > > > 1. Respect the current java doc, which said “If null is passed in, the > > state value will remain unchanged” > > 2. Make all AppendingState#add refuse to add null element > > > > > > We propose to apply the second solution, following the recommendation in > > Guava[1]. > > > > > > Would love to hear your thoughts. Thanks. > > > > > > Regards, > > > > Congxian > > > > > > [1] https://github.com/google/guava/wiki/UsingAndAvoidingNullExplained > > |
Re: [DISCUSS] Make AppendingState#add refuse to add null element
|
|
This is mostly a remnant from the previous state API, see [1] for
reference. The behaviour was basically copied to the new state implementations, which was a mistake, in hindsight. Also see [2] where I added AppendingState, here I also didn't document any special null behaviour. Best, Aljoscha [1] https://github.com/apache/flink/commit/caf46728045c0b886e6d4ec0aa429a830740a391 [2] https://github.com/apache/flink/commit/6cd8ceb10c841827cf89b74ecf5a0495a6933d53 On 16.01.20 04:13, Yun Tang wrote: > +1 for unifying the behavior of AppendingState#add . > > However, I have concern for the usage of window reducing function [1], I'm not sure whether user would rely on processing StreamRecord(null) to clear state. As you can see, user could not see the reducing window state directly, and the only way to communicate with state is via processing records. > > I'm not sure whether this is by design, @Aljoscha Krettek<mailto:[hidden email]> would you please share the initial idea when introducing this for the first time? > > > [1] https://ci.apache.org/projects/flink/flink-docs-stable/dev/stream/operators/windows.html#reducefunction > > Best > Yun Tang > > ________________________________ > From: Yu Li <[hidden email]> > Sent: Thursday, January 9, 2020 14:09 > To: dev <[hidden email]> > Subject: Re: [DISCUSS] Make AppendingState#add refuse to add null element > > +1 for unifying the behavior to refusing adding null element. Nice catch > and thanks for bringing up the discussion! > > Best Regards, > Yu > > > On Wed, 8 Jan 2020 at 22:50, Aljoscha Krettek <[hidden email]> wrote: > >> Hi, >> >> As I said in the discussion on the Jira issue, I’m in favour of this >> change! >> >> This is the Jira Issue, for reference: >> https://issues.apache.org/jira/browse/FLINK-15424 >> >> Best, >> Aljoscha >> >>> On 8. Jan 2020, at 15:16, Congxian Qiu <[hidden email]> wrote: >>> >>> Dear All >>> >>> >>> Currently, we found the implementations of AppendingState#add are not the >>> same, taking some as example: >>> >>> - HeapReducingState will clear state if add null element >>> - RocksDBReducingState will add null element if serializer can >> serialize >>> null >>> - Both HeapListState and RocksDBListState refuse to add null element — >>> will throw NullPointException >>> >>> >>> we think this need to be fixed, and possible solutions include: >>> >>> 1. Respect the current java doc, which said “If null is passed in, the >>> state value will remain unchanged” >>> 2. Make all AppendingState#add refuse to add null element >>> >>> >>> We propose to apply the second solution, following the recommendation in >>> Guava[1]. >>> >>> >>> Would love to hear your thoughts. Thanks. >>> >>> >>> Regards, >>> >>> Congxian >>> >>> >>> [1] https://github.com/google/guava/wiki/UsingAndAvoidingNullExplained >> >> > |
|
|
@Aljoscha Krettek<mailto:[hidden email]> , got it and thanks for your clarification.
Best Yun Tang 获取 Outlook for Android<https://aka.ms/ghei36> ________________________________ From: Aljoscha Krettek <[hidden email]> Sent: Thursday, January 16, 2020 8:40:57 PM To: [hidden email] <[hidden email]> Subject: Re: [DISCUSS] Make AppendingState#add refuse to add null element This is mostly a remnant from the previous state API, see [1] for reference. The behaviour was basically copied to the new state implementations, which was a mistake, in hindsight. Also see [2] where I added AppendingState, here I also didn't document any special null behaviour. Best, Aljoscha [1] https://github.com/apache/flink/commit/caf46728045c0b886e6d4ec0aa429a830740a391 [2] https://github.com/apache/flink/commit/6cd8ceb10c841827cf89b74ecf5a0495a6933d53 On 16.01.20 04:13, Yun Tang wrote: > +1 for unifying the behavior of AppendingState#add . > > However, I have concern for the usage of window reducing function [1], I'm not sure whether user would rely on processing StreamRecord(null) to clear state. As you can see, user could not see the reducing window state directly, and the only way to communicate with state is via processing records. > > I'm not sure whether this is by design, @Aljoscha Krettek<mailto:[hidden email]> would you please share the initial idea when introducing this for the first time? > > > [1] https://ci.apache.org/projects/flink/flink-docs-stable/dev/stream/operators/windows.html#reducefunction > > Best > Yun Tang > > ________________________________ > From: Yu Li <[hidden email]> > Sent: Thursday, January 9, 2020 14:09 > To: dev <[hidden email]> > Subject: Re: [DISCUSS] Make AppendingState#add refuse to add null element > > +1 for unifying the behavior to refusing adding null element. Nice catch > and thanks for bringing up the discussion! > > Best Regards, > Yu > > > On Wed, 8 Jan 2020 at 22:50, Aljoscha Krettek <[hidden email]> wrote: > >> Hi, >> >> As I said in the discussion on the Jira issue, I’m in favour of this >> change! >> >> This is the Jira Issue, for reference: >> https://issues.apache.org/jira/browse/FLINK-15424 >> >> Best, >> Aljoscha >> >>> On 8. Jan 2020, at 15:16, Congxian Qiu <[hidden email]> wrote: >>> >>> Dear All >>> >>> >>> Currently, we found the implementations of AppendingState#add are not the >>> same, taking some as example: >>> >>> - HeapReducingState will clear state if add null element >>> - RocksDBReducingState will add null element if serializer can >> serialize >>> null >>> - Both HeapListState and RocksDBListState refuse to add null element ― >>> will throw NullPointException >>> >>> >>> we think this need to be fixed, and possible solutions include: >>> >>> 1. Respect the current java doc, which said “If null is passed in, the >>> state value will remain unchanged” >>> 2. Make all AppendingState#add refuse to add null element >>> >>> >>> We propose to apply the second solution, following the recommendation in >>> Guava[1]. >>> >>> >>> Would love to hear your thoughts. Thanks. >>> >>> >>> Regards, >>> >>> Congxian >>> >>> >>> [1] https://github.com/google/guava/wiki/UsingAndAvoidingNullExplained >> >> > |
«
Return to (DEPRECATED) Apache Flink Mailing List archive.
|
1 view|%1 views
| Free forum by Nabble | Edit this page |