You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@uima.apache.org by tw...@apache.org on 2007/01/09 15:00:13 UTC
svn commit: r494413 -
/incubator/uima/uimaj/trunk/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java
Author: twgoetz
Date: Tue Jan 9 06:00:11 2007
New Revision: 494413
URL: http://svn.apache.org/viewvc?view=rev&rev=494413
Log:
Jira UIMA-25: remove strange array type subsumption code.
https://issues.apache.org/jira/browse/UIMA-25
Modified:
incubator/uima/uimaj/trunk/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java
Modified: incubator/uima/uimaj/trunk/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java
URL: http://svn.apache.org/viewvc/incubator/uima/uimaj/trunk/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java?view=diff&rev=494413&r1=494412&r2=494413
==============================================================================
--- incubator/uima/uimaj/trunk/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java (original)
+++ incubator/uima/uimaj/trunk/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java Tue Jan 9 06:00:11 2007
@@ -823,21 +823,14 @@
if (superType == type)
return true;
- // Yes, the code below is intentional. Until we actually support real
- // arrays of some
- // particular fs,
- // we have FSArray is the supertype of xxxx[] AND
- // xxx[] is the supertype of FSArray
- // (this second relation because all we can generate are instances of
- // FSArray
- // and we must be able to assign them to xxx[] )
+ // Special check for subtypes of FSArray.
if (superType == this.fsArrayTypeCode) {
return !ll_isPrimitiveArrayType(type) && ll_isArrayType(type);
}
+ // Special check for supertypes of FSArray. Why do we need this?
if (type == this.fsArrayTypeCode) {
- return superType == this.top || superType == this.arrayBaseTypeCode
- || (!ll_isPrimitiveArrayType(superType) && ll_isArrayType(superType));
+ return superType == this.top || superType == this.arrayBaseTypeCode;
}
// at this point, we could have arrays of other primitive types, or
Re: svn commit: r494413 - /incubator/uima/uimaj/trunk/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java
Posted by Adam Lally <al...@alum.rpi.edu>.
On 1/9/07, Thilo Goetz <tw...@gmx.de> wrote:
> I guess not, actually. The current approach is really not too nice,
> though. If I revert my change, subsumes() is broken for array types.
> But then, we don't properly support them, anyway.
Since it's not possible to create new instances of these types, the
fact that subsumes is broken doesn't really affect users.
> So should I just back
> out the change and defer the issue to post-2.1? I'd be fine with that.
>
Yes, I think so.
-Adam
Re: svn commit: r494413 - /incubator/uima/uimaj/trunk/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java
Posted by Thilo Goetz <tw...@gmx.de>.
Adam Lally wrote:
> On 1/9/07, Thilo Goetz <tw...@gmx.de> wrote:
>> Ok, no problem. I tried to start a discussion about this a few weeks
>> back, but nobody responded. Now I guess I have your attention ;-)
>>
>
> Sorry, that must have slipped through without my noticing.
>
>> >
>> > As I remember it, the issue is that type-restricted subtypes of
>> > FSArray were sort of half-implemented. So in the type system
>> > descriptor one can declare that a feature has a range type of FSArray
>> > whose element type is "MyType". This resulted in the dynamic creation
>> > of a type FSArray[MyType] in the type system. But, there's no method
>> > by which you can actually create an instance of this type.
>>
>> Why don't we add CAS.createArrayFS(Type arrayType, int length)?
>>
>
> Are there other issues with dynamic creation of new types after the
> type system has been "committed"? I recall that this broke some other
> code that Marshall had to work around, although maybe this has been
> sufficiently addressed at this point - I'm not sure. Also I think
> this may impact the C++. Eddie?
>
>> A proper implementation of parametrized arrays has been on the agenda
>> for a very long time. Why not do it now? I'm not sure what you mean by
>> using this in JCasGen, but checked setters and getters will only work if
>> the underlying CAS knows about parametrized arrays as well (since you
>> can freely mix CAS and JCas use on the same data).
>>
>
> I guess in general I'm not opposed to the idea of parameterized
> arrays. It may cause incompatibility, however, for those who are
> currently declaring the parameterized arrays in their type systems.
>
> One idea, if we're going to do a proper implementation, is to change
> the descriptor syntax to be like
> this:<rangeTypeName>FSArray[MyAnnotation]</rangeTypeName>. Then we
> could deprecate the <elementType> descriptor element and have our
> implementation ignore it, and that way wouldn't break assignment
> compatibility for those who were using it.
>
> I'm not sure exactly how JCasGen would use this, just that Marshall
> though that it could. It's true that if we only enforce this in JCas,
> someone using the CAS interface could "corrupt" the data such that it
> wouldn't work through the JCas, which I agree is not good. Of course
> the converse is also true - if we implement parameterized arrays in
> CAS we need to also do it in JCAS.
>
> Do we have enough runway to sufficiently explore all the issues and
> implement this in 2.1?
I guess not, actually. The current approach is really not too nice,
though. If I revert my change, subsumes() is broken for array types.
But then, we don't properly support them, anyway. So should I just back
out the change and defer the issue to post-2.1? I'd be fine with that.
>
> -Adam
Re: svn commit: r494413 - /incubator/uima/uimaj/trunk/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java
Posted by Adam Lally <al...@alum.rpi.edu>.
On 1/9/07, Thilo Goetz <tw...@gmx.de> wrote:
> Ok, no problem. I tried to start a discussion about this a few weeks
> back, but nobody responded. Now I guess I have your attention ;-)
>
Sorry, that must have slipped through without my noticing.
> >
> > As I remember it, the issue is that type-restricted subtypes of
> > FSArray were sort of half-implemented. So in the type system
> > descriptor one can declare that a feature has a range type of FSArray
> > whose element type is "MyType". This resulted in the dynamic creation
> > of a type FSArray[MyType] in the type system. But, there's no method
> > by which you can actually create an instance of this type.
>
> Why don't we add CAS.createArrayFS(Type arrayType, int length)?
>
Are there other issues with dynamic creation of new types after the
type system has been "committed"? I recall that this broke some other
code that Marshall had to work around, although maybe this has been
sufficiently addressed at this point - I'm not sure. Also I think
this may impact the C++. Eddie?
> A proper implementation of parametrized arrays has been on the agenda
> for a very long time. Why not do it now? I'm not sure what you mean by
> using this in JCasGen, but checked setters and getters will only work if
> the underlying CAS knows about parametrized arrays as well (since you
> can freely mix CAS and JCas use on the same data).
>
I guess in general I'm not opposed to the idea of parameterized
arrays. It may cause incompatibility, however, for those who are
currently declaring the parameterized arrays in their type systems.
One idea, if we're going to do a proper implementation, is to change
the descriptor syntax to be like
this:<rangeTypeName>FSArray[MyAnnotation]</rangeTypeName>. Then we
could deprecate the <elementType> descriptor element and have our
implementation ignore it, and that way wouldn't break assignment
compatibility for those who were using it.
I'm not sure exactly how JCasGen would use this, just that Marshall
though that it could. It's true that if we only enforce this in JCas,
someone using the CAS interface could "corrupt" the data such that it
wouldn't work through the JCas, which I agree is not good. Of course
the converse is also true - if we implement parameterized arrays in
CAS we need to also do it in JCAS.
Do we have enough runway to sufficiently explore all the issues and
implement this in 2.1?
-Adam
Re: svn commit: r494413 - /incubator/uima/uimaj/trunk/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java
Posted by Thilo Goetz <tw...@gmx.de>.
Adam Lally wrote:
> On 1/9/07, twgoetz@apache.org <tw...@apache.org> wrote:
>> Author: twgoetz
>> Date: Tue Jan 9 06:00:11 2007
>> New Revision: 494413
>>
>> URL: http://svn.apache.org/viewvc?view=rev&rev=494413
>> Log:
>> Jira UIMA-25: remove strange array type subsumption code.
>>
>> https://issues.apache.org/jira/browse/UIMA-25
>> <snip/>
>
>
> -1, sorry. We at least need to discuss this first. There's a reason
> why that code was put there (by Marhsall, I think). I'm unpleasantly
> surprised that we had no test case for this though. :-(
Ok, no problem. I tried to start a discussion about this a few weeks
back, but nobody responded. Now I guess I have your attention ;-)
>
> As I remember it, the issue is that type-restricted subtypes of
> FSArray were sort of half-implemented. So in the type system
> descriptor one can declare that a feature has a range type of FSArray
> whose element type is "MyType". This resulted in the dynamic creation
> of a type FSArray[MyType] in the type system. But, there's no method
> by which you can actually create an instance of this type.
Why don't we add CAS.createArrayFS(Type arrayType, int length)?
>
> If you try to define a "plain old" FSArray to a feature whose range
> type is FSArray[MyType], it fails on the subsumes check. So there's
> no way you could ever assign anything to this feature in our current
> implementation.
>
> I think we actually had a release in this state, and when things
> started breaking, tried to patch it in a maintenance release. This
> was done by hacking subsumes() to return true for any FSArrays (e.g.
> FSArray[MyType] subsumes FSArray *and* vice-versa), essentially making
> them equivalent from an assignment perspective.
>
> My original proposal for adding "elementType" to our type system
> descriptor was that we would not enforce this restriction in the CAS
> API, anyway. A slot to hold this information was needed for Ecore
> compatibility, but enforcement wasn't a strict requirement. I thought
> maybe we'd end up using in this JCasGen, though.
>
> -Adam
A proper implementation of parametrized arrays has been on the agenda
for a very long time. Why not do it now? I'm not sure what you mean by
using this in JCasGen, but checked setters and getters will only work if
the underlying CAS knows about parametrized arrays as well (since you
can freely mix CAS and JCas use on the same data).
--Thilo
Re: svn commit: r494413 - /incubator/uima/uimaj/trunk/uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java
Posted by Adam Lally <al...@alum.rpi.edu>.
On 1/9/07, twgoetz@apache.org <tw...@apache.org> wrote:
> Author: twgoetz
> Date: Tue Jan 9 06:00:11 2007
> New Revision: 494413
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=494413
> Log:
> Jira UIMA-25: remove strange array type subsumption code.
>
> https://issues.apache.org/jira/browse/UIMA-25
> <snip/>
-1, sorry. We at least need to discuss this first. There's a reason
why that code was put there (by Marhsall, I think). I'm unpleasantly
surprised that we had no test case for this though. :-(
As I remember it, the issue is that type-restricted subtypes of
FSArray were sort of half-implemented. So in the type system
descriptor one can declare that a feature has a range type of FSArray
whose element type is "MyType". This resulted in the dynamic creation
of a type FSArray[MyType] in the type system. But, there's no method
by which you can actually create an instance of this type.
If you try to define a "plain old" FSArray to a feature whose range
type is FSArray[MyType], it fails on the subsumes check. So there's
no way you could ever assign anything to this feature in our current
implementation.
I think we actually had a release in this state, and when things
started breaking, tried to patch it in a maintenance release. This
was done by hacking subsumes() to return true for any FSArrays (e.g.
FSArray[MyType] subsumes FSArray *and* vice-versa), essentially making
them equivalent from an assignment perspective.
My original proposal for adding "elementType" to our type system
descriptor was that we would not enforce this restriction in the CAS
API, anyway. A slot to hold this information was needed for Ecore
compatibility, but enforcement wasn't a strict requirement. I thought
maybe we'd end up using in this JCasGen, though.
-Adam