[GitHub] flink issue #6231: [FLINK-9694] Potentially NPE in CompositeTypeSerializerCo...
Github user pnowojski commented on the issue: https://github.com/apache/flink/pull/6231 Thanks for verification and reporting issue! ---
[GitHub] flink issue #6231: [FLINK-9694] Potentially NPE in CompositeTypeSerializerCo...
Github user yanghua commented on the issue: https://github.com/apache/flink/pull/6231 PR #6392 fixed this issue. ---
[GitHub] flink issue #6231: [FLINK-9694] Potentially NPE in CompositeTypeSerializerCo...
Github user yanghua commented on the issue: https://github.com/apache/flink/pull/6231 @pnowojski thanks for giving a solution, I will try to verify it in our inner Flink version. ---
[GitHub] flink issue #6231: [FLINK-9694] Potentially NPE in CompositeTypeSerializerCo...
Github user pnowojski commented on the issue: https://github.com/apache/flink/pull/6231 True, `checkNotNull(nestedSerializers);` is useless. Allowing nulls without enabled compile errors on violated `@Nullable` checks (this we can not enable in Flink) always leads to some null pointer exceptions and makes developing code unnecessary complicated - you need to manually walk through whole call stack up and down to confirm if null can/should be handled/supported/provided. This is annoying especially that in 99.9% cases completely avoidable - in this case, just call different constructor. I was looking for a way to call this other constructor in Scala and I couldn't (Scala seems to be annoying), however I found this workaround: https://github.com/apache/flink/pull/6392 ---
[GitHub] flink issue #6231: [FLINK-9694] Potentially NPE in CompositeTypeSerializerCo...
Github user yanghua commented on the issue: https://github.com/apache/flink/pull/6231 @pnowojski I have said it is because of the constructor : ``` CompositeTypeSerializerConfigSnapshot(TypeSerializer... nestedSerializers) ``` used [`varargs ` in JIRA description](https://issues.apache.org/jira/browse/FLINK-9694), the last comment in this PR, I just explain it looks like this style. We added null check and it works fine in our Flink env. So if we do not process it, in this case, this code is useless: ``` Preconditions.checkNotNull(nestedSerializers); ``` Why we do not check null in the potential nullable context? So what's the way you think is not ugly and dangerous? ---
[GitHub] flink issue #6231: [FLINK-9694] Potentially NPE in CompositeTypeSerializerCo...
Github user pnowojski commented on the issue: https://github.com/apache/flink/pull/6231 Nope, you are not quite correct. This: ``` def this() = this(null)//scala ``` translates to ``` CompositeTypeSerializerConfigSnapshot(null); ``` But because of varargs usages in `public CompositeTypeSerializerConfigSnapshot(TypeSerializer... nestedSerializers)`, you get in this constructor one element array containing single null value. If there was ``` def this() = this(null, null, null)//in scala CompositeTypeSerializerConfigSnapshot(null, null, null); // or in java ``` `nestedSerializers` array would have three null elements. Never the less I still do not think this is a good fix. Instead of supporting and handling nulls in `CompositeTypeSerializerConfigSnapshot` constructor (ugly and dangerous), default constructor of `CRowSerializerConfigSnapshot` should invoke default constructor of `CompositeTypeSerializerConfigSnapshot`. ---
[GitHub] flink issue #6231: [FLINK-9694] Potentially NPE in CompositeTypeSerializerCo...
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/6231
hi @pnowojski I did not call the
`CompositeTypeSerializerConfigSnapshot(TypeSerializer... nestedSerializers)`
constructor explicitly, the caller is Flink itself, see
[here](https://github.com/apache/flink/blob/master/flink-libraries/flink-table/src/main/scala/org/apache/flink/table/runtime/types/CRowSerializer.scala#L123).
And I just fix the NPE in this case :
```scala
def this() = this(null)//scala
```
but it does not means :
```
CompositeTypeSerializerConfigSnapshot(null);//java
```
it seems means :
```
CompositeTypeSerializerConfigSnapshot(new TypeSerializer[] {null})
//java
```
so it jumps the preconditions not null check :
```
Preconditions.checkNotNull(nestedSerializers);//java
```
then coursed NPE in the `for` loop
[here](https://github.com/apache/flink/blob/master/flink-core/src/main/java/org/apache/flink/api/common/typeutils/CompositeTypeSerializerConfigSnapshot.java#L53).
I think it is a defensive check, then it's OK in our inner Flink version
(in the previous comment, I said we customized table to provide stream and
dimension table join).
---
[GitHub] flink issue #6231: [FLINK-9694] Potentially NPE in CompositeTypeSerializerCo...
Github user pnowojski commented on the issue: https://github.com/apache/flink/pull/6231 I agree with @zentol and I do not see reason for supporting nulls here. This fix looks like hiding underlying implementation problem. Default constructor of `CRowSerializerConfigSnapshot` could use `CompositeTypeSerializerConfigSnapshot#CompositeTypeSerializerConfigSnapshot()` (without parameters). I don't know much about scala, but shouldn't it use this pattern: https://stackoverflow.com/questions/3299776/in-scala-how-can-i-subclass-a-java-class-with-multiple-constructors ? ---
[GitHub] flink issue #6231: [FLINK-9694] Potentially NPE in CompositeTypeSerializerCo...
Github user yanghua commented on the issue: https://github.com/apache/flink/pull/6231 @zentol we really caused this exception, in our inner Flink version, we customized flink-table and implemented stream and dim table join. I think the default constructor is needed by deserialization. Whatever it takes, the author who wrote this code misunderstood the variable `nestedSerializers ` could be null(in current case), but it did not happens. The truth is : the elements in `nestedSerializers` could be null. We add a judgement and fixed this NPE, now it works OK. ---
[GitHub] flink issue #6231: [FLINK-9694] Potentially NPE in CompositeTypeSerializerCo...
Github user zentol commented on the issue: https://github.com/apache/flink/pull/6231 This doesn't look correct to me, the serializer should always be non-null, and silently dropping null serializers most likely just causes other problems. Did you actually run into this NPE when using Flink, or did you explicitly call the constructor for demonstration purposes? As far as I can tell the code in `CRowSerializer` is either broken or a not adequately documented hack. It is at the very least untested as this NPE _always_ occur when this constructor is used. @tzulitai Why exactly is this [constructor](https://github.com/apache/flink/blob/master/flink-libraries/flink-table/src/main/scala/org/apache/flink/table/runtime/types/CRowSerializer.scala#L123) necessary? ---
[GitHub] flink issue #6231: [FLINK-9694] Potentially NPE in CompositeTypeSerializerCo...
Github user yanghua commented on the issue: https://github.com/apache/flink/pull/6231 @pnowojski can you review this? ---
[GitHub] flink issue #6231: [FLINK-9694] Potentially NPE in CompositeTypeSerializerCo...
Github user yanghua commented on the issue: https://github.com/apache/flink/pull/6231 cc @twalthr and @fhueske ---
[GitHub] flink issue #6231: [FLINK-9694] Potentially NPE in CompositeTypeSerializerCo...
Github user zhangminglei commented on the issue: https://github.com/apache/flink/pull/6231 Looks good from my side @yanghua :) ---
