Thanks for your explanation. 2017-02-10 18:18 GMT+03:00 Pavel Tupitsyn <ptupit...@apache.org>:
> You can close it. > > On Fri, Feb 10, 2017 at 6:16 PM, Vyacheslav Daradur <daradu...@gmail.com> > wrote: > > > I meant what I need to do with opened PR? > > I need to close it or to leave it open for future merge? > > > > 2017-02-10 17:42 GMT+03:00 Pavel Tupitsyn <ptupit...@apache.org>: > > > > > The ticket is targeted for 2.0 because this change may affect existing > > > code. > > > 1.9 is planned in the near future, and minor versions should not break > > > existing code. > > > > > > Pavel > > > > > > On Fri, Feb 10, 2017 at 5:24 PM, Vyacheslav Daradur < > daradu...@gmail.com > > > > > > wrote: > > > > > > > Pavel, thanks. > > > > > > > > What about PR to master-branch? > > > > > > > > 2017-02-10 16:55 GMT+03:00 Pavel Tupitsyn <ptupit...@apache.org>: > > > > > > > > > Merged to ignite-2.0. > > > > > > > > > > Thank you for the contribution, Vyacheslav! > > > > > > > > > > On Tue, Feb 7, 2017 at 10:15 PM, Denis Magda <dma...@apache.org> > > > wrote: > > > > > > > > > > > + Vladimir Ozerov > > > > > > > > > > > > It would be better if Vladimir Ozerov does the final review > > > considering > > > > > > all the changes in .NET, C++ and Java. > > > > > > > > > > > > Vladimir, could you do that? > > > > > > > > > > > > — > > > > > > Denis > > > > > > > > > > > > > On Feb 7, 2017, at 5:04 AM, Vyacheslav Daradur < > > > daradu...@gmail.com> > > > > > > wrote: > > > > > > > > > > > > > > +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 > > > > > <mailto: > > > > > > 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 > > > > > > <mailto: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 <mailto: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 > > > > > > <mailto: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 > > > > > > <mailto: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 <mailto: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 > > > > > > <mailto: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 <mailto: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 > > > > < > > > > > > http://ci.ignite.apache.org/viewQueued.html?itemId=445824>> > > > > > > > > >>>>>> > > > > > > > > >>>>>> > > > > > > > > >>>>>> > > > > > > > > >>>>>> 2017-02-06 13:20 GMT+03:00 Pavel Tupitsyn < > > > > > ptupit...@apache.org > > > > > > <mailto:ptupit...@apache.org>>: > > > > > > > > >>>>>> > > > > > > > > >>>>>>> .NET changes look good to me. > > > > > > > > >>>>>>> > > > > > > > > >>>>>>> Pavel > > > > > > > > >>>>>>> > > > > > > > > >>>>>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego < > > > > > > isap...@gridgain.com <mailto: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 > > > > > > <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 > > > > > > <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 <mailto:daradu...@gmail.com>> > > > > > > > > >>>>>>> > wrote: > > > > > > > > >>>>>>> > > > > > > > > > >>>>>>> >> Pavel, Igor > > > > > > > > >>>>>>> >> > > > > > > > > >>>>>>> >> Please, review it again. > > > > > > > > >>>>>>> >> > > > > > > > > >>>>>>> >> https://github.com/apache/ignite/pull/1473/files > < > > > > > > https://github.com/apache/ignite/pull/1473/files> > > > > > > > > >>>>>>> >> > > > > > > > > >>>>>>> >> All tests > > > > > > > > >>>>>>> >> <http://ci.ignite.apache.org/ > > > > viewLog.html?buildId=444231& > > > > > > tab <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 <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 <mailto: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 <mailto: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 <mailto: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 <mailto: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= > > > > > > <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 <mailto: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 <mailto: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 <mailto: > > > daradu...@gmail.com > > > > >> > > > > > > > > >>>>>>> >>>> >>>> wrote: > > > > > > > > >>>>>>> >>>> >>>> > > > > > > > > >>>>>>> >>>> >>>> > Hello. I fixed it. Please, review. > > > > > > > > >>>>>>> >>>> >>>> > > > > > > > > > >>>>>>> >>>> >>>> > https://issues.apache.org/ > > > > > jira/browse/IGNITE-3196 > > > > > > <https://issues.apache.org/jira/browse/IGNITE-3196> - > > > > > > > > >>>>>>> Marshaling > > > > > > > > >>>>>>> >>>> works > > > > > > > > >>>>>>> >>>> >>>> wrong > > > > > > > > >>>>>>> >>>> >>>> > for the BigDecimals that have negative > > scale > > > > > > > > >>>>>>> >>>> >>>> > > > > > > > > > >>>>>>> >>>> >>>> > > > > > > > > >>>>>>> >>>> >>> > > > > > > > > >>>>>>> >>>> >>> > > > > > > > > >>>>>>> >>>> >> > > > > > > > > >>>>>>> >>>> > > > > > > > > > >>>>>>> >>>> > > > > > > > > >>>>>>> >>> > > > > > > > > >>>>>>> >>> > > > > > > > > >>>>>>> >> > > > > > > > > >>>>>>> > > > > > > > > > >>>>>>> > > > > > > > > >>>>>> > > > > > > > > >>>>>> > > > > > > > > >>>>> > > > > > > > > >>>> > > > > > > > > >>> > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
