[Flink] fix data overflow while convert Integer to Long (#3738)#3740
Conversation
There was a problem hiding this comment.
Thanks for this patch @chenzihao5!
I checked (with a Scala shell but using java.lang.Integer) and using the explicit float does indeed seem to prevent overflow.
scala> import java.lang.{Integer => JInt}
import java.lang.{Integer=>JInt}
scala> JInt.MAX_VALUE
res5: Int = 2147483647
scala> JInt.MAX_VALUE * 1000
res6: Int = -1000
scala> JInt.MAX_VALUE * 1000L
res7: Long = 2147483647000
Could you add test cases somewhere for writing a value that would have overflowed otherwise?
@kbendick Thanks for your review. I will add some unit tests. |
|
Great finding, @chenzihao5 . It's indeed an overflow bug ! |
|
I'm marking this for inclusion in 0.13.0. We should wait for tests, but I want to make sure we include this in the release. |
Thanks for reminding, I will complete the unit test today. |
|
@rdblue @kbendick @openinx Hi, All. I have added unit tests in version 1.14, please help to review it, Thanks. About these unit tests: Thanks for reviewing! |
|
|
||
| @RunWith(Parameterized.class) | ||
| public class TestFlinkIcebergSinkNumData extends TableTestBase { | ||
| @ClassRule |
There was a problem hiding this comment.
Iceberg uses 2 spaces for an indent. Can you please reformat this file?
| new Object[]{"parquet", 1, true}, | ||
| new Object[]{"parquet", 1, false}, | ||
| new Object[]{"parquet", 2, true}, | ||
| new Object[]{"parquet", 2, false} |
There was a problem hiding this comment.
This many cases seems overkill to validate one method. Can you update an existing test suite for Avro writes?
There was a problem hiding this comment.
Thanks. I will update it.
|
@rdblue I have updated test into existing test suite for Avro writer and reformat code style. Please help to review it. |
|
Thanks, @chenzihao5! Looks good to me now. I'm running CI and will merge when it passes. |
|
Thanks, @chenzihao5! I merged this. |
Fix data overflow while writing data whose type is java.sql.Time.
Flink version: 1.12, 1.13, 1.14
function: org.apache.iceberg.flink.data.FlinkValueWriters.TimeMicrosWriter#write
@openinx @JingsongLi Please help to confirm this Problem. Thanks very much.