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 Fri, 10 Feb 2017 15:16:40 GMT
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 <ptupitsyn@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 <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