+Denis >>Ok, so we agree on .NET and C++ parts, only Java part is to be reviewed. >>Denis, can you have a look?
2017-02-07 15:27 GMT+03:00 Pavel Tupitsyn <ptupit...@apache.org>: > Ok, so we agree on .NET and C++ parts, only Java part is to be reviewed. > > Denis, can you have a look? > > Pavel > > On Tue, Feb 7, 2017 at 3:23 PM, Igor Sapego <isap...@gridgain.com> wrote: > > > Looks good to me. > > > > Best Regards, > > Igor > > > > On Tue, Feb 7, 2017 at 2:55 PM, Vyacheslav Daradur <daradu...@gmail.com> > > wrote: > > > > > Ok, thanks for explanations. > > > > > > What about this task? > > > > > > 2017-02-07 13:57 GMT+03:00 Igor Sapego <isap...@gridgain.com>: > > > > > >> But that's Ok. Since we use int8_t for bytes in C++ as well I guess > > >> your -0x80 may have more sense than 0x80. > > >> > > >> Best Regards, > > >> Igor > > >> > > >> On Tue, Feb 7, 2017 at 1:54 PM, Igor Sapego <isap...@gridgain.com> > > wrote: > > >> > > >>> I was just curious. > > >>> > > >>> In C++ both constants 0x80 and -0x80 are of type 'int' and have the > > same > > >>> lower byte, so they give the same result. Though first number is > > actually > > >>> 0x00000080 when the second one is 0xFFFFFF80. > > >>> > > >>> So it's just made a minus sign look a little redundant and pointless > to > > >>> me > > >>> in C++ code. > > >>> > > >>> Best Regards, > > >>> Igor > > >>> > > >>> On Mon, Feb 6, 2017 at 10:15 PM, Vyacheslav Daradur < > > daradu...@gmail.com > > >>> > wrote: > > >>> > > >>>> Byte.MIN_VALUE = -128 = -0x80 > > >>>> Byte.MAX_VALUE = 127 = 0x7F > > >>>> > > >>>> It is just more evident for me. > > >>>> > > >>>> Maybe, I just have the Java programming style. > > >>>> > > >>>> In Java: > > >>>> byte a = 100 | -0x80; // compiled > > >>>> byte b = 100 | 0x80; // doesn't compile, explicit type casting is > > >>>> neccessary (byte)(100 | 0x80) > > >>>> System.out.println(a | -0x80); // -28 > > >>>> System.out.println(a | 0x80); // 228 - cast to int > > >>>> > > >>>> Is it bad style? > > >>>> > > >>>> 2017-02-06 20:04 GMT+03:00 Igor Sapego <isap...@gridgain.com>: > > >>>> > > >>>>> Vyacheslav, > > >>>>> > > >>>>> Overall looks good. But why do you use -0x80 instead of 0x80? > > >>>>> > > >>>>> Best Regards, > > >>>>> Igor > > >>>>> > > >>>>> On Mon, Feb 6, 2017 at 5:36 PM, Vyacheslav Daradur < > > >>>>> daradu...@gmail.com> wrote: > > >>>>> > > >>>>>> Igor, > > >>>>>> > > >>>>>> I didn't change the CPP code before approval approach. > > >>>>>> I shall write directly, sorry. > > >>>>>> > > >>>>>> But I made CPP changes already. > > >>>>>> > > >>>>>> > TestEscConvertFunctionFloat > > >>>>>> > TestEscConvertFunctionDouble. > > >>>>>> These tests were passed > > >>>>>> <http://ci.ignite.apache.org/viewQueued.html?itemId=445824> > > >>>>>> > > >>>>>> > > >>>>>> > > >>>>>> 2017-02-06 13:20 GMT+03:00 Pavel Tupitsyn <ptupit...@apache.org>: > > >>>>>> > > >>>>>>> .NET changes look good to me. > > >>>>>>> > > >>>>>>> Pavel > > >>>>>>> > > >>>>>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego < > isap...@gridgain.com> > > >>>>>>> wrote: > > >>>>>>> > > >>>>>>> > Vyacheslav, I can see two ODBC tests fail in C++ test suits > that > > >>>>>>> should > > >>>>>>> > not: > > >>>>>>> > - TestEscConvertFunctionFloat > > >>>>>>> > <http://ci.ignite.apache.org/viewLog.html?buildId=444207&tab > > >>>>>>> =buildResultsDiv&buildTypeId=IgniteTests_IgnitePlatformCppLi > > >>>>>>> nux#testNameId-9178617718508801660> > > >>>>>>> > - TestEscConvertFunctionDouble > > >>>>>>> > <http://ci.ignite.apache.org/viewLog.html?buildId=444207&tab > > >>>>>>> =buildResultsDiv&buildTypeId=IgniteTests_IgnitePlatformCppLi > > >>>>>>> nux#testNameId5432107083822590090> > > >>>>>>> > . > > >>>>>>> > > > >>>>>>> > I believe, this is because I can't see any changes in C++ > Decimal > > >>>>>>> > marshaling code. > > >>>>>>> > Please, pay attention to file ignite\modules\platforms\cpp\ > > >>>>>>> > odbc\src\utility.cpp, > > >>>>>>> > functions ReadDecimal and WriteDecimal. > > >>>>>>> > > > >>>>>>> > Best Regards, > > >>>>>>> > Igor > > >>>>>>> > > > >>>>>>> > On Mon, Feb 6, 2017 at 11:21 AM, Vyacheslav Daradur < > > >>>>>>> daradu...@gmail.com> > > >>>>>>> > wrote: > > >>>>>>> > > > >>>>>>> >> Pavel, Igor > > >>>>>>> >> > > >>>>>>> >> Please, review it again. > > >>>>>>> >> > > >>>>>>> >> https://github.com/apache/ignite/pull/1473/files > > >>>>>>> >> > > >>>>>>> >> All tests > > >>>>>>> >> <http://ci.ignite.apache.org/viewLog.html?buildId=444231&tab > > >>>>>>> =buildResultsDiv&buildTypeId=IgniteTests_RunAll> > > >>>>>>> >> .NET tests > > >>>>>>> >> <http://ci.ignite.apache.org/viewLog.html?buildId=443439&tab > > >>>>>>> =buildResultsDiv&buildTypeId=IgniteTests_IgnitePlatformNet> > > >>>>>>> >> > > >>>>>>> >> How about this solution? > > >>>>>>> >> > > >>>>>>> >> 2017-02-03 13:59 GMT+03:00 Vyacheslav Daradur < > > >>>>>>> daradu...@gmail.com>: > > >>>>>>> >> > > >>>>>>> >>> 1. On my first question > > >>>>>>> >>> I think up, if we serialize only positive numbers, we can > write > > >>>>>>> sign in > > >>>>>>> >>> first byte, because it is positive always. > > >>>>>>> >>> I will try to make this decision > > >>>>>>> >>> > > >>>>>>> >>> 2017-02-03 12:48 GMT+03:00 Pavel Tupitsyn < > > ptupit...@apache.org > > >>>>>>> >: > > >>>>>>> >>> > > >>>>>>> >>>> Vyacheslav, > > >>>>>>> >>>> > > >>>>>>> >>>> I see the problem now. Yes, negative scale is not supported > in > > >>>>>>> .NET. > > >>>>>>> >>>> > > >>>>>>> >>>> I don't think we should do the multiplication. As you > > >>>>>>> described, this > > >>>>>>> >>>> will > > >>>>>>> >>>> break equality on Java side. SQL queries might be broken, > etc. > > >>>>>>> >>>> I think we should throw an exception in .NET when > encountering > > >>>>>>> negative > > >>>>>>> >>>> decimal scale. > > >>>>>>> >>>> > > >>>>>>> >>>> Vladimir O, any thoughts? > > >>>>>>> >>>> > > >>>>>>> >>>> Pavel > > >>>>>>> >>>> > > >>>>>>> >>>> On Fri, Feb 3, 2017 at 12:01 PM, Vyacheslav Daradur < > > >>>>>>> >>>> daradu...@gmail.com> > > >>>>>>> >>>> wrote: > > >>>>>>> >>>> > > >>>>>>> >>>> > Hello. > > >>>>>>> >>>> > > > >>>>>>> >>>> > I looked and understood the code of methods ReadDecimal > and > > >>>>>>> >>>> WriteDecimal > > >>>>>>> >>>> > on .NET platform. > > >>>>>>> >>>> > > > >>>>>>> >>>> > 1. At the moment remaking of this methods for my > > >>>>>>> Java-decimal-fix is > > >>>>>>> >>>> very > > >>>>>>> >>>> > difficult, it needs to write new methods for > > >>>>>>> >>>> serialization/deserialization > > >>>>>>> >>>> > of negative decimals. > > >>>>>>> >>>> > > > >>>>>>> >>>> > I can make it, but there is simpler decision: to add > > >>>>>>> additional byte > > >>>>>>> >>>> for > > >>>>>>> >>>> > sign. > > >>>>>>> >>>> > > > >>>>>>> >>>> > I need advice: difficult solution (new methods .net) > Versus > > : > > >>>>>>> simple > > >>>>>>> >>>> > solutions (additional byte)? > > >>>>>>> >>>> > > > >>>>>>> >>>> > *I don't know yet, what changes are necessary on ะก++ > > platform. > > >>>>>>> >>>> > > > >>>>>>> >>>> > 2. I see a problem with the negative scale on .NET > platform. > > >>>>>>> >>>> > > > >>>>>>> >>>> > Now negative scale is forbidden. > > >>>>>>> >>>> > > > >>>>>>> >>>> > We can make: > > >>>>>>> >>>> > if (scale < 0) return Decimal.Multiply(new decimal(lo, > mid, > > >>>>>>> hi, neg, > > >>>>>>> >>>> 0), > > >>>>>>> >>>> > new decimal(Math.Pow(10, -scale))); > > >>>>>>> >>>> > > > >>>>>>> >>>> > But there is the problem: > > >>>>>>> >>>> > * 1 Serialize in Java; number=123456789, scale=-4 > > >>>>>>> >>>> > * 2 Deserialize in .NET; number=1234567890000, scale=0 > > >>>>>>> >>>> > * 3 Serialize in .NET; number=1234567890000, scale=0 > > >>>>>>> >>>> > * 4 Deserialize in Java; number=1234567890000, scale=0 > > >>>>>>> >>>> > > > >>>>>>> >>>> > Logically: (1) 123456789 * 10^4 == (2) 1234567890000 > > >>>>>>> >>>> > > > >>>>>>> >>>> > In Java (1) not equal (2), because scales are different. > > >>>>>>> >>>> > > > >>>>>>> >>>> > Any thougths? > > >>>>>>> >>>> > > > >>>>>>> >>>> > 2017-01-31 14:08 GMT+03:00 Pavel Tupitsyn < > > >>>>>>> ptupit...@apache.org>: > > >>>>>>> >>>> > > > >>>>>>> >>>> >> Vyacheslav, > > >>>>>>> >>>> >> > > >>>>>>> >>>> >> I'm not sure I understand the code you attached. > > >>>>>>> >>>> >> > > >>>>>>> >>>> >> If you know how to fix the .NET part, can you just do it > in > > >>>>>>> your PR > > >>>>>>> >>>> and > > >>>>>>> >>>> >> run "Platform .NET" on TeamCity to verify? > > >>>>>>> >>>> >> http://ci.ignite.apache.org/viewType.html?buildTypeId= > > Ignite > > >>>>>>> >>>> >> Tests_IgnitePlatformNet > > >>>>>>> >>>> >> > > >>>>>>> >>>> >> Thanks, > > >>>>>>> >>>> >> > > >>>>>>> >>>> >> Pavel > > >>>>>>> >>>> >> > > >>>>>>> >>>> >> On Tue, Jan 31, 2017 at 1:35 PM, Vyacheslav Daradur < > > >>>>>>> >>>> daradu...@gmail.com> > > >>>>>>> >>>> >> wrote: > > >>>>>>> >>>> >> > > >>>>>>> >>>> >>> Pavel, I see that you are the main contributor of > > >>>>>>> Ignite.NET. > > >>>>>>> >>>> >>> > > >>>>>>> >>>> >>> We should repair BinaryUtils#ReadDecimal and > > >>>>>>> >>>> BinaryUtils#WriteDecimal. > > >>>>>>> >>>> >>> > > >>>>>>> >>>> >>> *ReadDecimal: > > >>>>>>> >>>> >>> byte[] mag = ReadByteArray(stream); // including at > least > > >>>>>>> one sign > > >>>>>>> >>>> bit, > > >>>>>>> >>>> >>> which is (ceil((this.bitLength() + 1)/8)) > > >>>>>>> >>>> >>> bool neg = (mag[0] < 0); > > >>>>>>> >>>> >>> if (scale < 0) > > >>>>>>> >>>> >>> // TODO: a scale of -3 means the unscaled value is > > >>>>>>> multiplied by > > >>>>>>> >>>> 1000 > > >>>>>>> >>>> >>> > > >>>>>>> >>>> >>> *WriteDecimal: > > >>>>>>> >>>> >>> int sign = vals[3] < 0 ? -1 : 0; > > >>>>>>> >>>> >>> stream.WriteInt(sign); > > >>>>>>> >>>> >>> > > >>>>>>> >>>> >>> Can you help with this task? > > >>>>>>> >>>> >>> > > >>>>>>> >>>> >>> > > >>>>>>> >>>> >>> 2017-01-31 12:46 GMT+03:00 Igor Sapego < > > >>>>>>> isap...@gridgain.com>: > > >>>>>>> >>>> >>> > > >>>>>>> >>>> >>>> Vyacheslav, > > >>>>>>> >>>> >>>> > > >>>>>>> >>>> >>>> I had a look at your PR and left some comments in Jira. > > >>>>>>> >>>> >>>> > > >>>>>>> >>>> >>>> Best Regards, > > >>>>>>> >>>> >>>> Igor > > >>>>>>> >>>> >>>> > > >>>>>>> >>>> >>>> On Mon, Jan 30, 2017 at 12:52 PM, Vyacheslav Daradur < > > >>>>>>> >>>> >>>> daradu...@gmail.com> > > >>>>>>> >>>> >>>> wrote: > > >>>>>>> >>>> >>>> > > >>>>>>> >>>> >>>> > Hello. I fixed it. Please, review. > > >>>>>>> >>>> >>>> > > > >>>>>>> >>>> >>>> > https://issues.apache.org/jira/browse/IGNITE-3196 - > > >>>>>>> Marshaling > > >>>>>>> >>>> works > > >>>>>>> >>>> >>>> wrong > > >>>>>>> >>>> >>>> > for the BigDecimals that have negative scale > > >>>>>>> >>>> >>>> > > > >>>>>>> >>>> >>>> > > >>>>>>> >>>> >>> > > >>>>>>> >>>> >>> > > >>>>>>> >>>> >> > > >>>>>>> >>>> > > > >>>>>>> >>>> > > >>>>>>> >>> > > >>>>>>> >>> > > >>>>>>> >> > > >>>>>>> > > > >>>>>>> > > >>>>>> > > >>>>>> > > >>>>> > > >>>> > > >>> > > >> > > > > > >
