You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ignite.apache.org by Vyacheslav Daradur <da...@gmail.com> on 2017/01/30 09:52:02 UTC
IGNITE-3196 - ready for review
Hello. I fixed it. Please, review.
https://issues.apache.org/jira/browse/IGNITE-3196 - Marshaling works wrong
for the BigDecimals that have negative scale
Re: IGNITE-3196 - ready for review
Posted by Vyacheslav Daradur <da...@gmail.com>.
Thanks for your explanation.
2017-02-10 18:18 GMT+03:00 Pavel Tupitsyn <pt...@apache.org>:
> You can close it.
>
> On Fri, Feb 10, 2017 at 6:16 PM, Vyacheslav Daradur <da...@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 <pt...@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 <
> daradurvs@gmail.com
> > >
> > > wrote:
> > >
> > > > Pavel, thanks.
> > > >
> > > > What about PR to master-branch?
> > > >
> > > > 2017-02-10 16:55 GMT+03:00 Pavel Tupitsyn <pt...@apache.org>:
> > > >
> > > > > Merged to ignite-2.0.
> > > > >
> > > > > Thank you for the contribution, Vyacheslav!
> > > > >
> > > > > On Tue, Feb 7, 2017 at 10:15 PM, Denis Magda <dm...@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 <
> > > daradurvs@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 <
> ptupitsyn@apache.org
> > > > > <mailto:
> > > > > > ptupitsyn@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 <
> > isapego@gridgain.com
> > > > > > <ma...@gridgain.com>> wrote:
> > > > > > >
> > > > > > > > Looks good to me.
> > > > > > > >
> > > > > > > > Best Regards,
> > > > > > > > Igor
> > > > > > > >
> > > > > > > > On Tue, Feb 7, 2017 at 2:55 PM, Vyacheslav Daradur <
> > > > > > daradurvs@gmail.com <ma...@gmail.com>>
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Ok, thanks for explanations.
> > > > > > > > >
> > > > > > > > > What about this task?
> > > > > > > > >
> > > > > > > > > 2017-02-07 13:57 GMT+03:00 Igor Sapego <
> isapego@gridgain.com
> > > > > > <ma...@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 <
> > > > isapego@gridgain.com
> > > > > > <ma...@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 <
> > > > > > > > daradurvs@gmail.com <ma...@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 <
> > > isapego@gridgain.com
> > > > > > <ma...@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 <
> > > > > > > > >>>>> daradurvs@gmail.com <ma...@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 <
> > > > > ptupitsyn@apache.org
> > > > > > <ma...@apache.org>>:
> > > > > > > > >>>>>>
> > > > > > > > >>>>>>> .NET changes look good to me.
> > > > > > > > >>>>>>>
> > > > > > > > >>>>>>> Pavel
> > > > > > > > >>>>>>>
> > > > > > > > >>>>>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <
> > > > > > isapego@gridgain.com <ma...@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 <
> > > > > > > > >>>>>>> daradurvs@gmail.com <ma...@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 <
> > > > > > > > >>>>>>> daradurvs@gmail.com <ma...@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 <
> > > > > > > > ptupitsyn@apache.org <ma...@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
> > > <
> > > > > > > > >>>>>>> >>>> daradurvs@gmail.com <mailto:daradurvs@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 <
> > > > > > > > >>>>>>> ptupitsyn@apache.org <ma...@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 <
> > > > > > > > >>>>>>> >>>> daradurvs@gmail.com <mailto:daradurvs@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 <
> > > > > > > > >>>>>>> isapego@gridgain.com <ma...@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
> > > > > > <
> > > > > > > > >>>>>>> >>>> >>>> daradurvs@gmail.com <mailto:
> > > daradurvs@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
> > > > > > > > >>>>>>> >>>> >>>> >
> > > > > > > > >>>>>>> >>>> >>>>
> > > > > > > > >>>>>>> >>>> >>>
> > > > > > > > >>>>>>> >>>> >>>
> > > > > > > > >>>>>>> >>>> >>
> > > > > > > > >>>>>>> >>>> >
> > > > > > > > >>>>>>> >>>>
> > > > > > > > >>>>>>> >>>
> > > > > > > > >>>>>>> >>>
> > > > > > > > >>>>>>> >>
> > > > > > > > >>>>>>> >
> > > > > > > > >>>>>>>
> > > > > > > > >>>>>>
> > > > > > > > >>>>>>
> > > > > > > > >>>>>
> > > > > > > > >>>>
> > > > > > > > >>>
> > > > > > > > >>
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: IGNITE-3196 - ready for review
Posted by Pavel Tupitsyn <pt...@apache.org>.
You can close it.
On Fri, Feb 10, 2017 at 6:16 PM, Vyacheslav Daradur <da...@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 <pt...@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 <daradurvs@gmail.com
> >
> > wrote:
> >
> > > Pavel, thanks.
> > >
> > > What about PR to master-branch?
> > >
> > > 2017-02-10 16:55 GMT+03:00 Pavel Tupitsyn <pt...@apache.org>:
> > >
> > > > Merged to ignite-2.0.
> > > >
> > > > Thank you for the contribution, Vyacheslav!
> > > >
> > > > On Tue, Feb 7, 2017 at 10:15 PM, Denis Magda <dm...@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 <
> > daradurvs@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 <ptupitsyn@apache.org
> > > > <mailto:
> > > > > ptupitsyn@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 <
> isapego@gridgain.com
> > > > > <ma...@gridgain.com>> wrote:
> > > > > >
> > > > > > > Looks good to me.
> > > > > > >
> > > > > > > Best Regards,
> > > > > > > Igor
> > > > > > >
> > > > > > > On Tue, Feb 7, 2017 at 2:55 PM, Vyacheslav Daradur <
> > > > > daradurvs@gmail.com <ma...@gmail.com>>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Ok, thanks for explanations.
> > > > > > > >
> > > > > > > > What about this task?
> > > > > > > >
> > > > > > > > 2017-02-07 13:57 GMT+03:00 Igor Sapego <isapego@gridgain.com
> > > > > <ma...@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 <
> > > isapego@gridgain.com
> > > > > <ma...@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 <
> > > > > > > daradurvs@gmail.com <ma...@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 <
> > isapego@gridgain.com
> > > > > <ma...@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 <
> > > > > > > >>>>> daradurvs@gmail.com <ma...@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 <
> > > > ptupitsyn@apache.org
> > > > > <ma...@apache.org>>:
> > > > > > > >>>>>>
> > > > > > > >>>>>>> .NET changes look good to me.
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> Pavel
> > > > > > > >>>>>>>
> > > > > > > >>>>>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <
> > > > > isapego@gridgain.com <ma...@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 <
> > > > > > > >>>>>>> daradurvs@gmail.com <ma...@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 <
> > > > > > > >>>>>>> daradurvs@gmail.com <ma...@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 <
> > > > > > > ptupitsyn@apache.org <ma...@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
> > <
> > > > > > > >>>>>>> >>>> daradurvs@gmail.com <ma...@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 <
> > > > > > > >>>>>>> ptupitsyn@apache.org <ma...@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 <
> > > > > > > >>>>>>> >>>> daradurvs@gmail.com <ma...@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 <
> > > > > > > >>>>>>> isapego@gridgain.com <ma...@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
> > > > > <
> > > > > > > >>>>>>> >>>> >>>> daradurvs@gmail.com <mailto:
> > daradurvs@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
> > > > > > > >>>>>>> >>>> >>>> >
> > > > > > > >>>>>>> >>>> >>>>
> > > > > > > >>>>>>> >>>> >>>
> > > > > > > >>>>>>> >>>> >>>
> > > > > > > >>>>>>> >>>> >>
> > > > > > > >>>>>>> >>>> >
> > > > > > > >>>>>>> >>>>
> > > > > > > >>>>>>> >>>
> > > > > > > >>>>>>> >>>
> > > > > > > >>>>>>> >>
> > > > > > > >>>>>>> >
> > > > > > > >>>>>>>
> > > > > > > >>>>>>
> > > > > > > >>>>>>
> > > > > > > >>>>>
> > > > > > > >>>>
> > > > > > > >>>
> > > > > > > >>
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > >
> > >
> >
>
Re: IGNITE-3196 - ready for review
Posted by Vyacheslav Daradur <da...@gmail.com>.
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 <pt...@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 <da...@gmail.com>
> wrote:
>
> > Pavel, thanks.
> >
> > What about PR to master-branch?
> >
> > 2017-02-10 16:55 GMT+03:00 Pavel Tupitsyn <pt...@apache.org>:
> >
> > > Merged to ignite-2.0.
> > >
> > > Thank you for the contribution, Vyacheslav!
> > >
> > > On Tue, Feb 7, 2017 at 10:15 PM, Denis Magda <dm...@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 <
> daradurvs@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 <ptupitsyn@apache.org
> > > <mailto:
> > > > ptupitsyn@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 <isapego@gridgain.com
> > > > <ma...@gridgain.com>> wrote:
> > > > >
> > > > > > Looks good to me.
> > > > > >
> > > > > > Best Regards,
> > > > > > Igor
> > > > > >
> > > > > > On Tue, Feb 7, 2017 at 2:55 PM, Vyacheslav Daradur <
> > > > daradurvs@gmail.com <ma...@gmail.com>>
> > > > > > wrote:
> > > > > >
> > > > > > > Ok, thanks for explanations.
> > > > > > >
> > > > > > > What about this task?
> > > > > > >
> > > > > > > 2017-02-07 13:57 GMT+03:00 Igor Sapego <isapego@gridgain.com
> > > > <ma...@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 <
> > isapego@gridgain.com
> > > > <ma...@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 <
> > > > > > daradurvs@gmail.com <ma...@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 <
> isapego@gridgain.com
> > > > <ma...@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 <
> > > > > > >>>>> daradurvs@gmail.com <ma...@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 <
> > > ptupitsyn@apache.org
> > > > <ma...@apache.org>>:
> > > > > > >>>>>>
> > > > > > >>>>>>> .NET changes look good to me.
> > > > > > >>>>>>>
> > > > > > >>>>>>> Pavel
> > > > > > >>>>>>>
> > > > > > >>>>>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <
> > > > isapego@gridgain.com <ma...@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 <
> > > > > > >>>>>>> daradurvs@gmail.com <ma...@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 <
> > > > > > >>>>>>> daradurvs@gmail.com <ma...@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 <
> > > > > > ptupitsyn@apache.org <ma...@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
> <
> > > > > > >>>>>>> >>>> daradurvs@gmail.com <ma...@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 <
> > > > > > >>>>>>> ptupitsyn@apache.org <ma...@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 <
> > > > > > >>>>>>> >>>> daradurvs@gmail.com <ma...@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 <
> > > > > > >>>>>>> isapego@gridgain.com <ma...@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
> > > > <
> > > > > > >>>>>>> >>>> >>>> daradurvs@gmail.com <mailto:
> daradurvs@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
> > > > > > >>>>>>> >>>> >>>> >
> > > > > > >>>>>>> >>>> >>>>
> > > > > > >>>>>>> >>>> >>>
> > > > > > >>>>>>> >>>> >>>
> > > > > > >>>>>>> >>>> >>
> > > > > > >>>>>>> >>>> >
> > > > > > >>>>>>> >>>>
> > > > > > >>>>>>> >>>
> > > > > > >>>>>>> >>>
> > > > > > >>>>>>> >>
> > > > > > >>>>>>> >
> > > > > > >>>>>>>
> > > > > > >>>>>>
> > > > > > >>>>>>
> > > > > > >>>>>
> > > > > > >>>>
> > > > > > >>>
> > > > > > >>
> > > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > >
> >
>
Re: IGNITE-3196 - ready for review
Posted by Pavel Tupitsyn <pt...@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 <da...@gmail.com>
wrote:
> Pavel, thanks.
>
> What about PR to master-branch?
>
> 2017-02-10 16:55 GMT+03:00 Pavel Tupitsyn <pt...@apache.org>:
>
> > Merged to ignite-2.0.
> >
> > Thank you for the contribution, Vyacheslav!
> >
> > On Tue, Feb 7, 2017 at 10:15 PM, Denis Magda <dm...@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 <da...@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 <ptupitsyn@apache.org
> > <mailto:
> > > ptupitsyn@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 <isapego@gridgain.com
> > > <ma...@gridgain.com>> wrote:
> > > >
> > > > > Looks good to me.
> > > > >
> > > > > Best Regards,
> > > > > Igor
> > > > >
> > > > > On Tue, Feb 7, 2017 at 2:55 PM, Vyacheslav Daradur <
> > > daradurvs@gmail.com <ma...@gmail.com>>
> > > > > wrote:
> > > > >
> > > > > > Ok, thanks for explanations.
> > > > > >
> > > > > > What about this task?
> > > > > >
> > > > > > 2017-02-07 13:57 GMT+03:00 Igor Sapego <isapego@gridgain.com
> > > <ma...@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 <
> isapego@gridgain.com
> > > <ma...@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 <
> > > > > daradurvs@gmail.com <ma...@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 <isapego@gridgain.com
> > > <ma...@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 <
> > > > > >>>>> daradurvs@gmail.com <ma...@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 <
> > ptupitsyn@apache.org
> > > <ma...@apache.org>>:
> > > > > >>>>>>
> > > > > >>>>>>> .NET changes look good to me.
> > > > > >>>>>>>
> > > > > >>>>>>> Pavel
> > > > > >>>>>>>
> > > > > >>>>>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <
> > > isapego@gridgain.com <ma...@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 <
> > > > > >>>>>>> daradurvs@gmail.com <ma...@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 <
> > > > > >>>>>>> daradurvs@gmail.com <ma...@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 <
> > > > > ptupitsyn@apache.org <ma...@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 <
> > > > > >>>>>>> >>>> daradurvs@gmail.com <ma...@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 <
> > > > > >>>>>>> ptupitsyn@apache.org <ma...@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 <
> > > > > >>>>>>> >>>> daradurvs@gmail.com <ma...@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 <
> > > > > >>>>>>> isapego@gridgain.com <ma...@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
> > > <
> > > > > >>>>>>> >>>> >>>> daradurvs@gmail.com <mailto:daradurvs@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
> > > > > >>>>>>> >>>> >>>> >
> > > > > >>>>>>> >>>> >>>>
> > > > > >>>>>>> >>>> >>>
> > > > > >>>>>>> >>>> >>>
> > > > > >>>>>>> >>>> >>
> > > > > >>>>>>> >>>> >
> > > > > >>>>>>> >>>>
> > > > > >>>>>>> >>>
> > > > > >>>>>>> >>>
> > > > > >>>>>>> >>
> > > > > >>>>>>> >
> > > > > >>>>>>>
> > > > > >>>>>>
> > > > > >>>>>>
> > > > > >>>>>
> > > > > >>>>
> > > > > >>>
> > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> > >
> >
>
Re: IGNITE-3196 - ready for review
Posted by Vyacheslav Daradur <da...@gmail.com>.
Pavel, thanks.
What about PR to master-branch?
2017-02-10 16:55 GMT+03:00 Pavel Tupitsyn <pt...@apache.org>:
> Merged to ignite-2.0.
>
> Thank you for the contribution, Vyacheslav!
>
> On Tue, Feb 7, 2017 at 10:15 PM, Denis Magda <dm...@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 <da...@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 <ptupitsyn@apache.org
> <mailto:
> > ptupitsyn@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 <isapego@gridgain.com
> > <ma...@gridgain.com>> wrote:
> > >
> > > > Looks good to me.
> > > >
> > > > Best Regards,
> > > > Igor
> > > >
> > > > On Tue, Feb 7, 2017 at 2:55 PM, Vyacheslav Daradur <
> > daradurvs@gmail.com <ma...@gmail.com>>
> > > > wrote:
> > > >
> > > > > Ok, thanks for explanations.
> > > > >
> > > > > What about this task?
> > > > >
> > > > > 2017-02-07 13:57 GMT+03:00 Igor Sapego <isapego@gridgain.com
> > <ma...@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 <isapego@gridgain.com
> > <ma...@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 <
> > > > daradurvs@gmail.com <ma...@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 <isapego@gridgain.com
> > <ma...@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 <
> > > > >>>>> daradurvs@gmail.com <ma...@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 <
> ptupitsyn@apache.org
> > <ma...@apache.org>>:
> > > > >>>>>>
> > > > >>>>>>> .NET changes look good to me.
> > > > >>>>>>>
> > > > >>>>>>> Pavel
> > > > >>>>>>>
> > > > >>>>>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <
> > isapego@gridgain.com <ma...@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 <
> > > > >>>>>>> daradurvs@gmail.com <ma...@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 <
> > > > >>>>>>> daradurvs@gmail.com <ma...@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 <
> > > > ptupitsyn@apache.org <ma...@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 <
> > > > >>>>>>> >>>> daradurvs@gmail.com <ma...@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 <
> > > > >>>>>>> ptupitsyn@apache.org <ma...@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 <
> > > > >>>>>>> >>>> daradurvs@gmail.com <ma...@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 <
> > > > >>>>>>> isapego@gridgain.com <ma...@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
> > <
> > > > >>>>>>> >>>> >>>> daradurvs@gmail.com <ma...@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
> > > > >>>>>>> >>>> >>>> >
> > > > >>>>>>> >>>> >>>>
> > > > >>>>>>> >>>> >>>
> > > > >>>>>>> >>>> >>>
> > > > >>>>>>> >>>> >>
> > > > >>>>>>> >>>> >
> > > > >>>>>>> >>>>
> > > > >>>>>>> >>>
> > > > >>>>>>> >>>
> > > > >>>>>>> >>
> > > > >>>>>>> >
> > > > >>>>>>>
> > > > >>>>>>
> > > > >>>>>>
> > > > >>>>>
> > > > >>>>
> > > > >>>
> > > > >>
> > > > >
> > > >
> > >
> >
> >
>
Re: IGNITE-3196 - ready for review
Posted by Pavel Tupitsyn <pt...@apache.org>.
Merged to ignite-2.0.
Thank you for the contribution, Vyacheslav!
On Tue, Feb 7, 2017 at 10:15 PM, Denis Magda <dm...@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 <da...@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 <ptupitsyn@apache.org <mailto:
> ptupitsyn@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 <isapego@gridgain.com
> <ma...@gridgain.com>> wrote:
> >
> > > Looks good to me.
> > >
> > > Best Regards,
> > > Igor
> > >
> > > On Tue, Feb 7, 2017 at 2:55 PM, Vyacheslav Daradur <
> daradurvs@gmail.com <ma...@gmail.com>>
> > > wrote:
> > >
> > > > Ok, thanks for explanations.
> > > >
> > > > What about this task?
> > > >
> > > > 2017-02-07 13:57 GMT+03:00 Igor Sapego <isapego@gridgain.com
> <ma...@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 <isapego@gridgain.com
> <ma...@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 <
> > > daradurvs@gmail.com <ma...@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 <isapego@gridgain.com
> <ma...@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 <
> > > >>>>> daradurvs@gmail.com <ma...@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 <ptupitsyn@apache.org
> <ma...@apache.org>>:
> > > >>>>>>
> > > >>>>>>> .NET changes look good to me.
> > > >>>>>>>
> > > >>>>>>> Pavel
> > > >>>>>>>
> > > >>>>>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <
> isapego@gridgain.com <ma...@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 <
> > > >>>>>>> daradurvs@gmail.com <ma...@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 <
> > > >>>>>>> daradurvs@gmail.com <ma...@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 <
> > > ptupitsyn@apache.org <ma...@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 <
> > > >>>>>>> >>>> daradurvs@gmail.com <ma...@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 <
> > > >>>>>>> ptupitsyn@apache.org <ma...@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 <
> > > >>>>>>> >>>> daradurvs@gmail.com <ma...@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 <
> > > >>>>>>> isapego@gridgain.com <ma...@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
> <
> > > >>>>>>> >>>> >>>> daradurvs@gmail.com <ma...@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
> > > >>>>>>> >>>> >>>> >
> > > >>>>>>> >>>> >>>>
> > > >>>>>>> >>>> >>>
> > > >>>>>>> >>>> >>>
> > > >>>>>>> >>>> >>
> > > >>>>>>> >>>> >
> > > >>>>>>> >>>>
> > > >>>>>>> >>>
> > > >>>>>>> >>>
> > > >>>>>>> >>
> > > >>>>>>> >
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>
> > > >>>
> > > >>
> > > >
> > >
> >
>
>
Re: IGNITE-3196 - ready for review
Posted by Denis Magda <dm...@apache.org>.
+ 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 <da...@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 <ptupitsyn@apache.org <ma...@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 <isapego@gridgain.com <ma...@gridgain.com>> wrote:
>
> > Looks good to me.
> >
> > Best Regards,
> > Igor
> >
> > On Tue, Feb 7, 2017 at 2:55 PM, Vyacheslav Daradur <daradurvs@gmail.com <ma...@gmail.com>>
> > wrote:
> >
> > > Ok, thanks for explanations.
> > >
> > > What about this task?
> > >
> > > 2017-02-07 13:57 GMT+03:00 Igor Sapego <isapego@gridgain.com <ma...@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 <isapego@gridgain.com <ma...@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 <
> > daradurvs@gmail.com <ma...@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 <isapego@gridgain.com <ma...@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 <
> > >>>>> daradurvs@gmail.com <ma...@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 <ptupitsyn@apache.org <ma...@apache.org>>:
> > >>>>>>
> > >>>>>>> .NET changes look good to me.
> > >>>>>>>
> > >>>>>>> Pavel
> > >>>>>>>
> > >>>>>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <isapego@gridgain.com <ma...@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 <
> > >>>>>>> daradurvs@gmail.com <ma...@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 <
> > >>>>>>> daradurvs@gmail.com <ma...@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 <
> > ptupitsyn@apache.org <ma...@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 <
> > >>>>>>> >>>> daradurvs@gmail.com <ma...@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 <
> > >>>>>>> ptupitsyn@apache.org <ma...@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 <
> > >>>>>>> >>>> daradurvs@gmail.com <ma...@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 <
> > >>>>>>> isapego@gridgain.com <ma...@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 <
> > >>>>>>> >>>> >>>> daradurvs@gmail.com <ma...@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
> > >>>>>>> >>>> >>>> >
> > >>>>>>> >>>> >>>>
> > >>>>>>> >>>> >>>
> > >>>>>>> >>>> >>>
> > >>>>>>> >>>> >>
> > >>>>>>> >>>> >
> > >>>>>>> >>>>
> > >>>>>>> >>>
> > >>>>>>> >>>
> > >>>>>>> >>
> > >>>>>>> >
> > >>>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>
> > >
> >
>
Re: IGNITE-3196 - ready for review
Posted by Vyacheslav Daradur <da...@gmail.com>.
+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 <pt...@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 <is...@gridgain.com> wrote:
>
> > Looks good to me.
> >
> > Best Regards,
> > Igor
> >
> > On Tue, Feb 7, 2017 at 2:55 PM, Vyacheslav Daradur <da...@gmail.com>
> > wrote:
> >
> > > Ok, thanks for explanations.
> > >
> > > What about this task?
> > >
> > > 2017-02-07 13:57 GMT+03:00 Igor Sapego <is...@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 <is...@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 <
> > daradurvs@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 <is...@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 <
> > >>>>> daradurvs@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 <pt...@apache.org>:
> > >>>>>>
> > >>>>>>> .NET changes look good to me.
> > >>>>>>>
> > >>>>>>> Pavel
> > >>>>>>>
> > >>>>>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <
> isapego@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 <
> > >>>>>>> daradurvs@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 <
> > >>>>>>> daradurvs@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 <
> > ptupitsyn@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 <
> > >>>>>>> >>>> daradurvs@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 <
> > >>>>>>> ptupitsyn@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 <
> > >>>>>>> >>>> daradurvs@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 <
> > >>>>>>> isapego@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 <
> > >>>>>>> >>>> >>>> daradurvs@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
> > >>>>>>> >>>> >>>> >
> > >>>>>>> >>>> >>>>
> > >>>>>>> >>>> >>>
> > >>>>>>> >>>> >>>
> > >>>>>>> >>>> >>
> > >>>>>>> >>>> >
> > >>>>>>> >>>>
> > >>>>>>> >>>
> > >>>>>>> >>>
> > >>>>>>> >>
> > >>>>>>> >
> > >>>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>
> > >
> >
>
Re: IGNITE-3196 - ready for review
Posted by Pavel Tupitsyn <pt...@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 <is...@gridgain.com> wrote:
> Looks good to me.
>
> Best Regards,
> Igor
>
> On Tue, Feb 7, 2017 at 2:55 PM, Vyacheslav Daradur <da...@gmail.com>
> wrote:
>
> > Ok, thanks for explanations.
> >
> > What about this task?
> >
> > 2017-02-07 13:57 GMT+03:00 Igor Sapego <is...@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 <is...@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 <
> daradurvs@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 <is...@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 <
> >>>>> daradurvs@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 <pt...@apache.org>:
> >>>>>>
> >>>>>>> .NET changes look good to me.
> >>>>>>>
> >>>>>>> Pavel
> >>>>>>>
> >>>>>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <is...@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 <
> >>>>>>> daradurvs@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 <
> >>>>>>> daradurvs@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 <
> ptupitsyn@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 <
> >>>>>>> >>>> daradurvs@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 <
> >>>>>>> ptupitsyn@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 <
> >>>>>>> >>>> daradurvs@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 <
> >>>>>>> isapego@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 <
> >>>>>>> >>>> >>>> daradurvs@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
> >>>>>>> >>>> >>>> >
> >>>>>>> >>>> >>>>
> >>>>>>> >>>> >>>
> >>>>>>> >>>> >>>
> >>>>>>> >>>> >>
> >>>>>>> >>>> >
> >>>>>>> >>>>
> >>>>>>> >>>
> >>>>>>> >>>
> >>>>>>> >>
> >>>>>>> >
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> >
>
Re: IGNITE-3196 - ready for review
Posted by Igor Sapego <is...@gridgain.com>.
Looks good to me.
Best Regards,
Igor
On Tue, Feb 7, 2017 at 2:55 PM, Vyacheslav Daradur <da...@gmail.com>
wrote:
> Ok, thanks for explanations.
>
> What about this task?
>
> 2017-02-07 13:57 GMT+03:00 Igor Sapego <is...@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 <is...@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 <daradurvs@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 <is...@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 <
>>>>> daradurvs@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 <pt...@apache.org>:
>>>>>>
>>>>>>> .NET changes look good to me.
>>>>>>>
>>>>>>> Pavel
>>>>>>>
>>>>>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <is...@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 <
>>>>>>> daradurvs@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 <
>>>>>>> daradurvs@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 <ptupitsyn@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 <
>>>>>>> >>>> daradurvs@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 <
>>>>>>> ptupitsyn@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 <
>>>>>>> >>>> daradurvs@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 <
>>>>>>> isapego@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 <
>>>>>>> >>>> >>>> daradurvs@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
>>>>>>> >>>> >>>> >
>>>>>>> >>>> >>>>
>>>>>>> >>>> >>>
>>>>>>> >>>> >>>
>>>>>>> >>>> >>
>>>>>>> >>>> >
>>>>>>> >>>>
>>>>>>> >>>
>>>>>>> >>>
>>>>>>> >>
>>>>>>> >
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
Re: IGNITE-3196 - ready for review
Posted by Vyacheslav Daradur <da...@gmail.com>.
Ok, thanks for explanations.
What about this task?
2017-02-07 13:57 GMT+03:00 Igor Sapego <is...@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 <is...@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 <da...@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 <is...@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 <daradurvs@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 <pt...@apache.org>:
>>>>>
>>>>>> .NET changes look good to me.
>>>>>>
>>>>>> Pavel
>>>>>>
>>>>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <is...@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 <
>>>>>> daradurvs@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 <daradurvs@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 <pt...@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 <
>>>>>> >>>> daradurvs@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 <
>>>>>> ptupitsyn@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 <
>>>>>> >>>> daradurvs@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 <isapego@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 <
>>>>>> >>>> >>>> daradurvs@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
>>>>>> >>>> >>>> >
>>>>>> >>>> >>>>
>>>>>> >>>> >>>
>>>>>> >>>> >>>
>>>>>> >>>> >>
>>>>>> >>>> >
>>>>>> >>>>
>>>>>> >>>
>>>>>> >>>
>>>>>> >>
>>>>>> >
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>
Re: IGNITE-3196 - ready for review
Posted by Igor Sapego <is...@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 <is...@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 <da...@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 <is...@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 <da...@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 <pt...@apache.org>:
>>>>
>>>>> .NET changes look good to me.
>>>>>
>>>>> Pavel
>>>>>
>>>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <is...@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 <
>>>>> daradurvs@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 <daradurvs@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 <pt...@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 <
>>>>> >>>> daradurvs@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 <ptupitsyn@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 <
>>>>> >>>> daradurvs@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 <isapego@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 <
>>>>> >>>> >>>> daradurvs@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
>>>>> >>>> >>>> >
>>>>> >>>> >>>>
>>>>> >>>> >>>
>>>>> >>>> >>>
>>>>> >>>> >>
>>>>> >>>> >
>>>>> >>>>
>>>>> >>>
>>>>> >>>
>>>>> >>
>>>>> >
>>>>>
>>>>
>>>>
>>>
>>
>
Re: IGNITE-3196 - ready for review
Posted by Igor Sapego <is...@gridgain.com>.
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 <da...@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 <is...@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 <da...@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 <pt...@apache.org>:
>>>
>>>> .NET changes look good to me.
>>>>
>>>> Pavel
>>>>
>>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <is...@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 <
>>>> daradurvs@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 <da...@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 <pt...@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 <
>>>> >>>> daradurvs@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 <ptupitsyn@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 <
>>>> >>>> daradurvs@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 <is...@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 <
>>>> >>>> >>>> daradurvs@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
>>>> >>>> >>>> >
>>>> >>>> >>>>
>>>> >>>> >>>
>>>> >>>> >>>
>>>> >>>> >>
>>>> >>>> >
>>>> >>>>
>>>> >>>
>>>> >>>
>>>> >>
>>>> >
>>>>
>>>
>>>
>>
>
Re: IGNITE-3196 - ready for review
Posted by Vyacheslav Daradur <da...@gmail.com>.
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 <is...@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 <da...@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 <pt...@apache.org>:
>>
>>> .NET changes look good to me.
>>>
>>> Pavel
>>>
>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <is...@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 <
>>> daradurvs@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 <da...@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 <pt...@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 <
>>> >>>> daradurvs@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 <pt...@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 <
>>> >>>> daradurvs@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 <is...@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 <
>>> >>>> >>>> daradurvs@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
>>> >>>> >>>> >
>>> >>>> >>>>
>>> >>>> >>>
>>> >>>> >>>
>>> >>>> >>
>>> >>>> >
>>> >>>>
>>> >>>
>>> >>>
>>> >>
>>> >
>>>
>>
>>
>
Re: IGNITE-3196 - ready for review
Posted by Igor Sapego <is...@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 <da...@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 <pt...@apache.org>:
>
>> .NET changes look good to me.
>>
>> Pavel
>>
>> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <is...@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 <
>> daradurvs@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 <da...@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 <pt...@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 <
>> >>>> daradurvs@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 <pt...@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 <
>> >>>> daradurvs@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 <is...@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 <
>> >>>> >>>> daradurvs@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
>> >>>> >>>> >
>> >>>> >>>>
>> >>>> >>>
>> >>>> >>>
>> >>>> >>
>> >>>> >
>> >>>>
>> >>>
>> >>>
>> >>
>> >
>>
>
>
Re: IGNITE-3196 - ready for review
Posted by Vyacheslav Daradur <da...@gmail.com>.
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 <pt...@apache.org>:
> .NET changes look good to me.
>
> Pavel
>
> On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <is...@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 <daradurvs@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 <da...@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 <pt...@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 <
> >>>> daradurvs@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 <pt...@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 <
> >>>> daradurvs@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 <is...@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 <
> >>>> >>>> daradurvs@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
> >>>> >>>> >
> >>>> >>>>
> >>>> >>>
> >>>> >>>
> >>>> >>
> >>>> >
> >>>>
> >>>
> >>>
> >>
> >
>
Re: IGNITE-3196 - ready for review
Posted by Pavel Tupitsyn <pt...@apache.org>.
.NET changes look good to me.
Pavel
On Mon, Feb 6, 2017 at 1:10 PM, Igor Sapego <is...@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_IgnitePlatformCppLinux#testNameId-9178617718508801660>
> - TestEscConvertFunctionDouble
> <http://ci.ignite.apache.org/viewLog.html?buildId=444207&tab=buildResultsDiv&buildTypeId=IgniteTests_IgnitePlatformCppLinux#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 <da...@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 <da...@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 <pt...@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 <
>>>> daradurvs@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 <pt...@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 <
>>>> daradurvs@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 <is...@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 <
>>>> >>>> daradurvs@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
>>>> >>>> >
>>>> >>>>
>>>> >>>
>>>> >>>
>>>> >>
>>>> >
>>>>
>>>
>>>
>>
>
Re: IGNITE-3196 - ready for review
Posted by Igor Sapego <is...@gridgain.com>.
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_IgnitePlatformCppLinux#testNameId-9178617718508801660>
- TestEscConvertFunctionDouble
<http://ci.ignite.apache.org/viewLog.html?buildId=444207&tab=buildResultsDiv&buildTypeId=IgniteTests_IgnitePlatformCppLinux#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 <da...@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 <da...@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 <pt...@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 <daradurvs@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 <pt...@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 <
>>> daradurvs@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 <is...@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 <
>>> >>>> daradurvs@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
>>> >>>> >
>>> >>>>
>>> >>>
>>> >>>
>>> >>
>>> >
>>>
>>
>>
>
Re: IGNITE-3196 - ready for review
Posted by Vyacheslav Daradur <da...@gmail.com>.
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 <da...@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 <pt...@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 <da...@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 <pt...@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 <
>> daradurvs@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 <is...@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 <
>> >>>> daradurvs@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
>> >>>> >
>> >>>>
>> >>>
>> >>>
>> >>
>> >
>>
>
>
Re: IGNITE-3196 - ready for review
Posted by Vyacheslav Daradur <da...@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 <pt...@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 <da...@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 <pt...@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 <
> daradurvs@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 <is...@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 <
> >>>> daradurvs@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
> >>>> >
> >>>>
> >>>
> >>>
> >>
> >
>
Re: IGNITE-3196 - ready for review
Posted by Pavel Tupitsyn <pt...@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 <da...@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 <pt...@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 <da...@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 <is...@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 <
>>>> daradurvs@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
>>>> >
>>>>
>>>
>>>
>>
>
Re: IGNITE-3196 - ready for review
Posted by Vyacheslav Daradur <da...@gmail.com>.
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 <pt...@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=
> IgniteTests_IgnitePlatformNet
>
> Thanks,
>
> Pavel
>
> On Tue, Jan 31, 2017 at 1:35 PM, Vyacheslav Daradur <da...@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 <is...@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 <
>>> daradurvs@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
>>> >
>>>
>>
>>
>
Re: IGNITE-3196 - ready for review
Posted by Pavel Tupitsyn <pt...@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=IgniteTests_IgnitePlatformNet
Thanks,
Pavel
On Tue, Jan 31, 2017 at 1:35 PM, Vyacheslav Daradur <da...@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 <is...@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 <daradurvs@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
>> >
>>
>
>
Re: IGNITE-3196 - ready for review
Posted by Vyacheslav Daradur <da...@gmail.com>.
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 <is...@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 <da...@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
> >
>
Re: IGNITE-3196 - ready for review
Posted by Igor Sapego <is...@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 <da...@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
>