ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vyacheslav Daradur <daradu...@gmail.com>
Subject Re: IGNITE-3196 - ready for review
Date Mon, 06 Feb 2017 14:36:35 GMT
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 <ptupitsyn@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
> >>>> >>>> >
> >>>> >>>>
> >>>> >>>
> >>>> >>>
> >>>> >>
> >>>> >
> >>>>
> >>>
> >>>
> >>
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message