ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Pavel Tupitsyn <ptupit...@apache.org>
Subject Re: IGNITE-3196 - ready for review
Date Fri, 10 Feb 2017 14:42:28 GMT
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 <ptupitsyn@apache.org>:
>
> > Merged to ignite-2.0.
> >
> > Thank you for the contribution, Vyacheslav!
> >
> > On Tue, Feb 7, 2017 at 10:15 PM, Denis Magda <dmagda@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
> > > <mailto:isapego@gridgain.com>> wrote:
> > > >
> > > > > Looks good to me.
> > > > >
> > > > > Best Regards,
> > > > > Igor
> > > > >
> > > > > On Tue, Feb 7, 2017 at 2:55 PM, Vyacheslav Daradur <
> > > daradurvs@gmail.com <mailto: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
> > > <mailto: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
> > > <mailto: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 <mailto: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
> > > <mailto: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 <mailto: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
> <
> > > http://ci.ignite.apache.org/viewQueued.html?itemId=445824>>
> > > > > >>>>>>
> > > > > >>>>>>
> > > > > >>>>>>
> > > > > >>>>>> 2017-02-06 13:20 GMT+03:00 Pavel Tupitsyn
<
> > ptupitsyn@apache.org
> > > <mailto:ptupitsyn@apache.org>>:
> > > > > >>>>>>
> > > > > >>>>>>> .NET changes look good to me.
> > > > > >>>>>>>
> > > > > >>>>>>> Pavel
> > > > > >>>>>>>
> > > > > >>>>>>> On Mon, Feb 6, 2017 at 1:10 PM, Igor
Sapego <
> > > isapego@gridgain.com <mailto: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
> > > <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 <mailto:daradurvs@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 <mailto: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 <mailto: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
<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 <mailto: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=
> > > <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 <mailto: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
<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
> > > > > >>>>>>> >>>> >>>> >
> > > > > >>>>>>> >>>> >>>>
> > > > > >>>>>>> >>>> >>>
> > > > > >>>>>>> >>>> >>>
> > > > > >>>>>>> >>>> >>
> > > > > >>>>>>> >>>> >
> > > > > >>>>>>> >>>>
> > > > > >>>>>>> >>>
> > > > > >>>>>>> >>>
> > > > > >>>>>>> >>
> > > > > >>>>>>> >
> > > > > >>>>>>>
> > > > > >>>>>>
> > > > > >>>>>>
> > > > > >>>>>
> > > > > >>>>
> > > > > >>>
> > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> > >
> >
>

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