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 14:24:18 GMT
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