Hi Ivan,
Thanks for chiming in. My comments are below.
-Val
On Sat, May 1, 2021 at 12:20 AM Ivan Daschinsky <ivanda...@gmail.com> wrote:
> Hi!
> First of all, when array is serialized, marshaller actually DO
> PRESERVE type of element (seel
> org.apache.ignite.internal.binary.BinaryUtils#doReadObjectArray and
> org.apache.ignite.internal.binary.BinaryWriterExImpl#doWriteObjectArray).
> AFAIK, the motivation of Nickolay proposal, is the fact, that during
> call of PlatformService we do additional marshal/unmarshall step and
> during this step we loose array type info
> See org.apache.ignite.internal.binary.BinaryReaderExImpl#readObjectDetached
> and org.apache.ignite.internal.binary.BinaryUtils#doReadObjectArray)
>
> In order to handle this problem, we can just add some wrapper that
> contains element type info and use it in our INTERNAL API.
> I just don't understand why we should change IgniteBinary API.
>
Makes sense to me. I would also avoid changing the public API.
>
> >> It was designed as a data storage format, and the fact
> that Ignite uses it elsewhere is a fundamental design flaw. BTW, this is
> something we're trying to fix in 3.0.
>
> Please, do not. There are many cases when this can really improve
> performance. Reflection is always slower than low level api and many
> users are happy with low level API.
>
Low-level APIs are not being removed. If anything, they are likely to
become more low-level :) Either way, that's off-topic. I would recommend
reviewing the related IEPs and starting a separate discussion if you have
any questions or concerns.
>
> сб, 1 мая 2021 г. в 00:51, Valentin Kulichenko <
> valentin.kuliche...@gmail.com>:
> >
> > Hi Nikolay,
> >
> > Is there a specific motivation behind your proposal? I do acknowledge
> that
> > the semantics of the toBinary method is a little weird, but my concern is
> > that the way it works with arrays is just an example. Are you suggesting
> > changing collections, primitives, and other "first citizen" data types as
> > well? To me, that looks like a huge risky change without a clear value.
> >
> > I actually believe that binary format *should not* be used as a universal
> > serde mechanism. It was designed as a data storage format, and the fact
> > that Ignite uses it elsewhere is a fundamental design flaw. BTW, this is
> > something we're trying to fix in 3.0.
> >
> > That said, I'm not necessarily against the change (especially if we do
> not
> > change the default behavior). But I would like to better understand its
> > scope (e.g., arrays only or beyond?), as well as get some examples of use
> > cases that would benefit from the change.
> >
> > -Val
> >
> >
> > On Fri, Apr 30, 2021 at 7:35 AM Nikolay Izhikov <nizhi...@apache.org>
> wrote:
> >
> > > Hello, Ilya.
> > >
> > > Thanks for the feedback!
> > >
> > > > For me it sounds like something we would like to do in 3.0
> > >
> > > Ignite 3 is a very long way to go, so I prefer to target this fix in
> > > Ignite 2.x.
> > >
> > > > I think making it default "true" is a breaking change and is not
> > > possible in a minor release
> > >
> > > Yes, you are correct it is a breaking change.
> > > It seems for me, we all agreed that breaking changes are possible in
> minor
> > > releases.
> > >
> > > Anyway, if we will decide do not to enable this feature by default
> it’s OK
> > > for me.
> > > We still can implement it and improve the binary SerDe mechanism.
> > >
> > >
> > > > 30 апр. 2021 г., в 17:23, Ilya Kasnacheev <ilya.kasnach...@gmail.com
> >
> > > написал(а):
> > > >
> > > > Hello!
> > > >
> > > > For me it sounds like something we would like to do in 3.0 (if
> indeed it
> > > > will have arrays as possible value (or key) type), but doing it in
> 2.x
> > > > raises concerns whether it has enough time left to stabilize.
> > > >
> > > > Also, I think making it default "true" is a breaking change and is
> not
> > > > possible in a minor release, case in point,
> > > > IGNITE_BINARY_SORT_OBJECT_FIELDS is still waiting for 3.0 and it is
> less
> > > > destructive.
> > > >
> > > > Of course I would also like to hear what other community members
> think.
> > > >
> > > > Regards,
> > > > --
> > > > Ilya Kasnacheev
> > > >
> > > >
> > > > пт, 30 апр. 2021 г. в 17:16, Nikolay Izhikov <nizhi...@apache.org>:
> > > >
> > > >> Igniters,
> > > >>
> > > >> Want to clarify my proposal about new array store format.
> > > >> I think we should store array in special binary wrapper that will
> keep
> > > >> original component type
> > > >>
> > > >> ```
> > > >> public class BinaryArrayWrapper implements BinaryObjectEx,
> > > Externalizable {
> > > >> /** Type ID. */
> > > >> private int compTypeId;
> > > >>
> > > >> /** Raw data. */
> > > >> private String compClsName;
> > > >>
> > > >> /** Value. */
> > > >> private Object[] arr;
> > > >>
> > > >> // Further implementation.
> > > >> }
> > > >> ```
> > > >>
> > > >>
> > > >>> 30 апр. 2021 г., в 16:31, Nikolay Izhikov <nizhikov....@gmail.com>
> > > >> написал(а):
> > > >>>
> > > >>> Hello, Igniters.
> > > >>>
> > > >>> Currently, binary marshaller works as follows(Say, we have a class
> > > >> `User` then):
> > > >>>
> > > >>> IgniteBinary#toBinary(User)` -> BinaryObject
> > > >>> IgniteBinary#toBinary(User[])` -> Object[]
> > > >>> IgniteBinary#toBinary(Object[])` -> Object[]
> > > >>>
> > > >>> This means, that we lose array component type information during
> binary
> > > >> serialization.
> > > >>> AFAIK, it’s a design choice made during binary infrastructure
> > > >> development.
> > > >>>
> > > >>> This lead to the following disadvantages:
> > > >>>
> > > >>> 1. `IgniteBinary` can’t be used as a universal SerDe mechanism.
> > > >>> 2. Ignite internals(service grid, .Net calls) contains many tweaks
> and
> > > >> hacks to deal with custom user array and still has many issues [1]
> > > >>>
> > > >>> I propose to make breaking changes and fix the custom user array
> SeDe
> > > as
> > > >> follows:
> > > >>>
> > > >>> 1. Implement binary serialization that correctly Ser and Deser
> > > >> array using some kind of the wrapper (BinaryArrayWrapper).
> > > >>>
> > > >>> IgniteBinary#toBinary(User)` -> BinaryObject
> > > >>> IgniteBinary#toBinary(User[])` -> BinaryObject
> > > >>> IgniteBinary#toBinary(Object[])` -> BinaryObject
> > > >>>
> > > >>> 2. Introduce system flag `IGNITE_USE_BINARY_ARRAY` that
> enables
> > > >> correct SerDe of arrays. The default value is false to keep backward
> > > >> compatibility in the next Ignite release(2.11).
> > > >>>
> > > >>> 3. Set `IGNITE_USE_BINARY_ARRAY` to `true` in the ongoing
> Ignite
> > > >> releases (2.12).
> > > >>>
> > > >>> WDYT?
> > > >>>
> > > >>> [1] https://issues.apache.org/jira/browse/IGNITE-14299
> > > >>
> > > >>
> > >
> > >
>
>
>
> --
> Sincerely yours, Ivan Daschinskiy
>
