Why do we need 2 mappers vs 1? What is the difference between them? On Fri, Jan 22, 2016 at 9:46 AM, Artem Shutak <[email protected]> wrote:
> Dmitriy, > > I think I got your point. I still have BinaryNameMapper and BinaryIdMapper > interfaces, but I have now by one implementation for each mapper with > properties as you wrote before. > - BinaryBaseNameMapper - an implementation of BinaryNameMapper that has > *setSimpleName(boolean isSImpleName)* property. > - BinaryBaseIdMapper - an implementation of BinaryBaseIdMapper that has > *setLowerCase(boolean usLowerCase)* property. > > Thoughts? > > -- Artem -- > > On Thu, Jan 21, 2016 at 1:35 PM, Artem Shutak <[email protected]> > wrote: > > > Dmitriy, > > > > Out-of-the-box Ignite wil have 2 IdMappers: > > - BinaryLowerCaseIdMapper - converts all of the characters in given > > string to lower case and then calls hashCode() for it. > > - BinaryStraightIdMapper - just calls hashCode() for given string > > (without converting to lower case). > > > > May be BinaryStraightIdMapper is not the best name for this mapper. > > Please, suggest another one if you have. > > > > I agree, that current design with BinaryNameMapper and BinaryIdMapper > > looks a little bit complex, but I don't see a better option to support > all > > things described in my previous email. Dmitriy, If you have, please, > > describe your design in the ticket. > > > > Lets focus on names of mappers in this thread. > > > > Thanks, > > -- Artem -- > > > > On Thu, Jan 21, 2016 at 6:10 AM, Dmitriy Setrakyan < > [email protected]> > > wrote: > > > >> Artem, > >> > >> My suggestion was that instead of having 3 id mappers that do almost the > >> same thing, have 1 ID mapper with additional configuration properties. > >> > >> BTW, I still don’t get what a straight ID mapper means. > >> > >> D. > >> > >> On Wed, Jan 20, 2016 at 9:45 AM, Artem Shutak <[email protected]> > >> wrote: > >> > >> > Dmitriy, > >> > > >> > BinaryStraightIdMapper - calculate hash code for given strings (do not > >> > change given string and call hashCode() for it). It's simplest > >> IdMapper. It > >> > would be nice if we will have it out-of-the-box. > >> > > >> > Could you please describe your approach in more details? Keep in mind, > >> that > >> > BinaryIdMapper is already a part of public API and, theoretically, a > >> user > >> > can write your own mapper to map class names to IDs (if he will have > >> > collisions for own classes) and custom mapper should know nothing > about > >> > "simpleName" and "lowerCase" properties (for custom implementation). > >> > > >> > Lets me try to clarify current approach. At first, names classes and > >> fileds > >> > are processed by BinaryNameMapper and then these processed names pass > to > >> > BinaryIdMapper for id calculation. > >> > > >> > Keep in mind the following: > >> > 1. It have to work with .Net. > >> > 2. it have to work without .Net and have to work with full name of > >> classes > >> > (to fix IGNITE-2191). > >> > 3. By some reasons, Ignite write *typeName* (for some types or for > all, > >> not > >> > sure) at binary metadata. > >> > > >> > To support 1 and 2 we cannot always write full name or simple name at > >> > metadata. So, we have to have method like *String typeName(String > >> > fullClassName)* that returns processed class name (we have the one on > >> > BinaryNameMapper). > >> > > >> > I want to repeat that this design are consistent with .Net where we > >> already > >> > have NameMapper and IdMapper. > >> > > >> > Thnaks, > >> > -- Artem -- > >> > > >> > On Wed, Jan 20, 2016 at 7:58 PM, Dmitriy Setrakyan < > >> [email protected]> > >> > wrote: > >> > > >> > > Looks too busy. > >> > > > >> > > Why not just have one ID mapper with config properties, like > >> > > isSimpleName(), isLowerCase()? > >> > > > >> > > Also, what is a straight ID mapper? > >> > > > >> > > D. > >> > > > >> > > On Wed, Jan 20, 2016 at 4:01 AM, Artem Shutak <[email protected] > > > >> > > wrote: > >> > > > >> > > > Dmitriy, thanks for reminding me about this thread. > >> > > > > >> > > > According to found issues in process of working on IGNITE-2191, it > >> was > >> > > > decided to add new entity BinaryNameMapper (in addition to > >> > > BinaryIdMapper) > >> > > > as a property of BinaryConfiguration. So, we will have the same > >> > > > configuration as we already have for .Net. > >> > > > > >> > > > So, we need to confirm names for BinaryNameMapper and > >> BinaryIdMapper. I > >> > > > propose the following: > >> > > > > >> > > > - BinarySimpleNameMapper / SimpleClassNameBinaryNameMapper / > >> > > > BinarySimpleClassNameMapper - returns simple name of class. > >> > > > - BinaryFullNameMapper / BinaryNoopNameMapper / > >> NoopBinaryNameMapper / > >> > > > BinaryOriginalNameMapper / StraightBinaryNameMapper - returns name > >> > > without > >> > > > changes. > >> > > > - BinaryLowerCaseIdMapper - it was BinaryInternalIdMapper. > >> > > > - BinaryStraightIdMapper - calculate hash code for given strings. > >> > > > > >> > > > I would chose BinarySimpleClassNameMapper, > BinaryOriginalNameMapper, > >> > > > BinaryLowerCaseIdMapper and BinaryStraightIdMapper names. > >> > > > > >> > > > Thanks, > >> > > > -- Artem -- > >> > > > > >> > > > On Wed, Jan 20, 2016 at 1:46 AM, Dmitriy Setrakyan < > >> > > [email protected]> > >> > > > wrote: > >> > > > > >> > > > > Artem, what name do you plan to give to the > >> BinaryInternalIdMapper? > >> > > > > > >> > > > > On Fri, Jan 15, 2016 at 1:52 AM, Artem Shutak < > >> [email protected]> > >> > > > > wrote: > >> > > > > > >> > > > > > All Binary-related classes start from Binary. So, it's not > >> > > consistent. > >> > > > > > > >> > > > > > We should chose between *BinarySimpleNameIdMapper* and > >> > > > > > *BinarySimpleClassNameIdMapper*. > >> > > > > > > >> > > > > > Also, I'd like to move default *BinaryInternalIdMapper* to > >> public > >> > > > package > >> > > > > > (that uses full class name) and rename him accordingly. Any > >> > > objections? > >> > > > > > > >> > > > > > -- Artem -- > >> > > > > > > >> > > > > > On Thu, Jan 14, 2016 at 11:21 PM, Dmitriy Setrakyan < > >> > > > > [email protected] > >> > > > > > > > >> > > > > > wrote: > >> > > > > > > >> > > > > > > On Thu, Jan 14, 2016 at 3:39 AM, Yakov Zhdanov < > >> > > [email protected]> > >> > > > > > > wrote: > >> > > > > > > > >> > > > > > > > I would suggest "SimpleClassNameBinaryIdMapper" > >> > > > > > > > > >> > > > > > > > >> > > > > > > Is it consistent? Do we have other classes in the same > package > >> > that > >> > > > > start > >> > > > > > > with word Binary? If not, then I agree. > >> > > > > > > > >> > > > > > > > >> > > > > > > > > >> > > > > > > > --Yakov > >> > > > > > > > > >> > > > > > > > 2016-01-14 4:59 GMT+03:00 Dmitriy Setrakyan < > >> > > [email protected] > >> > > > >: > >> > > > > > > > > >> > > > > > > > > I like the last one too. > >> > > > > > > > > > >> > > > > > > > > On Wed, Jan 13, 2016 at 7:54 AM, Artem Shutak < > >> > > > > [email protected]> > >> > > > > > > > > wrote: > >> > > > > > > > > > >> > > > > > > > > > Igniters, > >> > > > > > > > > > > >> > > > > > > > > > I'm working on > >> > > > https://issues.apache.org/jira/browse/IGNITE-2191 > >> > > > > > > > (Binary > >> > > > > > > > > > marshaller: support user classes with the same simple > >> name) > >> > > > bug. > >> > > > > > > > > > > >> > > > > > > > > > The fix expects new BinaryIdMapper that uses a simple > >> name > >> > of > >> > > > > > > classes. > >> > > > > > > > It > >> > > > > > > > > > is required for supporting of platforms (.Net. C++). > >> > > > > > > > > > > >> > > > > > > > > > I would be glade to hear suggestions about good name > for > >> > this > >> > > > > > mapper. > >> > > > > > > > > > > >> > > > > > > > > > I propose the following: > >> > > > > > > > > > - BinaryPlatformIdMapper > >> > > > > > > > > > - BinaryInteropIdMapper > >> > > > > > > > > > - SimpleNameBinaryIdMapper > >> > > > > > > > > > - BinarySimpleNameIdMapper > >> > > > > > > > > > > >> > > > > > > > > > I like the last one, but it has a big length. > >> > > > > > > > > > > >> > > > > > > > > > Thanks, > >> > > > > > > > > > -- Artem -- > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > > > >
