Nikolay, I agree that our binary array handling has some limitations - Ignite loses array element type in many cases (cache.Put -> cache.Get, Binary Mode, etc).
However, for internal platform and services implementations we should fix the root cause: avoid extra deserialization->serialization pass completely. This will also improve performance. Thanks, Pavel On Sat, May 1, 2021 at 9:20 PM Valentin Kulichenko < valentin.kuliche...@gmail.com> wrote: > 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 > > >