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 > >>>>>>> >>>> >>>> > > >>>>>>> >>>> >>>> > >>>>>>> >>>> >>> > >>>>>>> >>>> >>> > >>>>>>> >>>> >> > >>>>>>> >>>> > > >>>>>>> >>>> > >>>>>>> >>> > >>>>>>> >>> > >>>>>>> >> > >>>>>>> > > >>>>>>> > >>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > >
