[GitHub] flink pull request #6321: [FLINK-9829] fix the wrapper classes be compared b...

2018-07-16 Thread lamber-ken
Github user lamber-ken closed the pull request at:

https://github.com/apache/flink/pull/6321


---


[GitHub] flink pull request #6321: [FLINK-9829] fix the wrapper classes be compared b...

2018-07-15 Thread lamber-ken
Github user lamber-ken commented on a diff in the pull request:

https://github.com/apache/flink/pull/6321#discussion_r202566500
  
--- Diff: 
flink-core/src/main/java/org/apache/flink/api/common/typeutils/base/BigDecSerializer.java
 ---
@@ -69,17 +69,17 @@ public void serialize(BigDecimal record, DataOutputView 
target) throws IOExcepti
}
// fast paths for 0, 1, 10
// only reference equality is checked because equals would be 
too expensive
--- End diff --

right, the second section code was not exposed anywhere, and just modify 
the first section code now


---


[GitHub] flink pull request #6321: [FLINK-9829] fix the wrapper classes be compared b...

2018-07-13 Thread zentol
Github user zentol commented on a diff in the pull request:

https://github.com/apache/flink/pull/6321#discussion_r202335561
  
--- Diff: 
flink-core/src/main/java/org/apache/flink/api/common/typeutils/base/BigDecSerializer.java
 ---
@@ -69,17 +69,17 @@ public void serialize(BigDecimal record, DataOutputView 
target) throws IOExcepti
}
// fast paths for 0, 1, 10
// only reference equality is checked because equals would be 
too expensive
--- End diff --

The code adheres to the API provided by `BigInteger`, so this is fine as 
far as I'm concerned.
It would be a different story if, say, we would use reference equality for 
`2`, since that relies an implementation detail that isn't exposed anywhere.


---


[GitHub] flink pull request #6321: [FLINK-9829] fix the wrapper classes be compared b...

2018-07-12 Thread lamber-ken
Github user lamber-ken commented on a diff in the pull request:

https://github.com/apache/flink/pull/6321#discussion_r202075894
  
--- Diff: 
flink-core/src/main/java/org/apache/flink/api/common/typeutils/base/BigDecSerializer.java
 ---
@@ -69,17 +69,17 @@ public void serialize(BigDecimal record, DataOutputView 
target) throws IOExcepti
}
// fast paths for 0, 1, 10
// only reference equality is checked because equals would be 
too expensive
--- End diff --

No.
In terms of performance, this is OK, but from the point of view of language 
specification, may be it's better to use `equals` method.


---


[GitHub] flink pull request #6321: [FLINK-9829] fix the wrapper classes be compared b...

2018-07-12 Thread zentol
Github user zentol commented on a diff in the pull request:

https://github.com/apache/flink/pull/6321#discussion_r202056979
  
--- Diff: 
flink-core/src/main/java/org/apache/flink/api/common/typeutils/base/BigDecSerializer.java
 ---
@@ -69,17 +69,17 @@ public void serialize(BigDecimal record, DataOutputView 
target) throws IOExcepti
}
// fast paths for 0, 1, 10
// only reference equality is checked because equals would be 
too expensive
--- End diff --

did you see this comment?


---


[GitHub] flink pull request #6321: [FLINK-9829] fix the wrapper classes be compared b...

2018-07-12 Thread lamber-ken
GitHub user lamber-ken opened a pull request:

https://github.com/apache/flink/pull/6321

[FLINK-9829] fix the wrapper classes be compared by symbol of '==' directly 
in BigDecSerializer.java

## What is the purpose of the change
  - fix the wrapper classes be compared by symbol of '==' directly in 
BigDecSerializer.java

## Brief change log

  - The wrapper classes should be compared by equals method rather than by 
symbol of '==' directly
  - should use BigDecimal's equal method

## Does this pull request potentially affect one of the following parts:

  - Dependencies (does it add or upgrade a dependency): (no)
  - The public API, i.e., is any changed class annotated with 
`@Public(Evolving)`: (no)
  - The serializers: (no)
  - The runtime per-record code paths (performance sensitive): (no)
  - Anything that affects deployment or recovery: JobManager (and its 
components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
  - The S3 file system connector: (no)

## Documentation

  - Does this pull request introduce a new feature? (no)
  - If yes, how is the feature documented? (JavaDocs)


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/lamber-ken/flink FLINK-9829

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/flink/pull/6321.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #6321


commit cebe6b9d6f4c812c336581545623f430d5cb0b58
Author: lamber-ken 
Date:   2018-07-12T06:53:53Z

[FLINK-9829] fix the wrapper classes be compared by symbol of '==' directly 
in BigDecSerializer.java




---