ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Igor Sapego <isap...@gridgain.com>
Subject Re: IGNITE-3196 - ready for review
Date Tue, 07 Feb 2017 12:23:02 GMT
Looks good to me.

Best Regards,
Igor

On Tue, Feb 7, 2017 at 2:55 PM, Vyacheslav Daradur <daradurvs@gmail.com>
wrote:

> Ok, thanks for explanations.
>
> What about this task?
>
> 2017-02-07 13:57 GMT+03:00 Igor Sapego <isapego@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> 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 <isapego@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 <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