Add support for dfdl:lengthKind="endOfParent"#1652
Add support for dfdl:lengthKind="endOfParent"#1652olabusayoT wants to merge 1 commit intoapache:mainfrom
dfdl:lengthKind="endOfParent"#1652Conversation
- Implemented logic to handle elements with `dfdl:lengthKind="endOfParent"`. - Added validations to enforce schema/spec constraints and error conditions specific to `endOfParent` lengthKind. - Introduced determination of effective length units for parent elements and related error checks. - removed NYI Errors - add tests for endOfParent elements with different LengthKinds incl nested EndOfParent DAFFODIL-238
332415b to
7e7bd04
Compare
dfdl:lengthKind="endOfParent"dfdl:lengthKind="endOfParent"
stevedlawrence
left a comment
There was a problem hiding this comment.
Looks great, surprise how much we kindof already have support for this with some tweaks.
| notYetImplemented("lengthKind='endOfParent' for complex type") | ||
| case LengthKind.EndOfParent => | ||
| notYetImplemented("lengthKind='endOfParent' for simple type") | ||
| // per DFDL Spec 9.3.2, endOfParent is already positioned at parent's end so length is zero |
There was a problem hiding this comment.
This comment is a bit confusing to me. This function is supposed to return whether or not we statically know if this element must have non-zero length. I imagine we can rarely statically know that for endOfParent elements, so I think returning false here is correct. But the comment kindof makes it sound like the length is always zero, which kindof contradicts that.
Reading this portion of the spec (which this comment copies), I think the spec is talking about the runtime evaluation of whether or not a field is zero length. I believe the spec is just saying that that an endOfParent element has zero length representation if it is already at the parents end (i.e. bitLimit == bitPosition). Since this is more about runtime, I'm not sure this comment belongs here and might avoid that confusion.
| } | ||
|
|
||
| final lazy val immediatelyEnclosingElementParent: Option[ElementBase] = { | ||
| val p = optLexicalParent.flatMap { |
There was a problem hiding this comment.
Note that I think lexical parents does not extend past global decls, so if a global decl has endOfParent then I'm not sure we will correctly check EOP restrictions for anything that references that decl. I'm wondering if the checks need to go down instead up?
For example, maybe an element needs to check if it has properties that would disallow children with lengthKind EOP and if so check if any children have are EOP? Or check if any immediate children have EOP, and if so then check if they are compatible?
| case e: ElementBase => Some(e) | ||
| case ge: GlobalElementDecl => Some(ge.asRoot) | ||
| case s: SequenceTermBase => s.immediatelyEnclosingElementParent | ||
| case c: ChoiceTermBase => c.immediatelyEnclosingElementParent |
There was a problem hiding this comment.
Do we need to return the choice in some cases? It looks like the logic for EOP sometimes cares about the choice so I'm not sure we can bypass this?
| </tdml:dfdlInfoset> | ||
| </tdml:infoset> | ||
| </tdml:parserTestCase> | ||
|
|
There was a problem hiding this comment.
Some lines from the spec that I'm not sure I saw tests for:
The dfdl:lengthKind 'endOfParent' can also be used on the document root to allow the last element to consume the data up to the end of the data stream.
I assume this means something like:
<element name="root" dfdl:lengthKind="endOfParent">
<complexType>
<sequence>
<element name="field1" ... />
<element name="rest" dfdl:lenghtKind="endOfParent" ... />
</sequence>
</complexType>
</element>So rest should contain everything up until the end of the data. That doesn't really work will with the assuming that we'll always have a bit limit, since the root element won't every set bitLimit in this case.
A simple element must have one of [...] dfdl:representation 'binary' and a packed decimal representation
I don't think there are any tests for packed binary formats, and it looks like there aren't any modifications to the code to support packed types
A simple element must have one of [...] dfdl:representation 'text'
I think this implies that you can have simple types with bool/numbers/dates/times/etc as long as representation is text. I think these should all work because they just add a converter to a specified length string parser, but we should have tests for them.
The dfdl:lengthKind 'endOfParent' means that the element is terminated [...] or the end of an enclosing choice with dfdl:choiceLengthKind ‘explicit’.
choiceLengthKind="explicit" isn't used very often, but we should probably have a couple tests to make sure this works with EOP children.
| notYetImplemented("lengthKind='endOfParent' for complex type") | ||
| case LengthKind.EndOfParent => | ||
| notYetImplemented("lengthKind='endOfParent' for simple type") | ||
| case LengthKind.EndOfParent => new SpecifiedLengthEndOfParent(this, body) |
There was a problem hiding this comment.
I think this is is only needed for complex types? My thinking is that simple types with lengthKind EOP should have parent parser (ether complex or explicity length choice) that already set the bit limit via one of these specified length parsers. So the bit limit has already been set correctly and we don't need another parser to do that.
But this is needed for complex types with EOP since they need to skip any bits up to their parents bit limit that thier children might not have consumed.
So I think this wants to be:
case LengthKind.EndOfParent if isComplexType => new SpecifiedLengthEndOfParent(this, body)And then bodyRequiresSpecifiedLength wants to be modified to make it so it evaluates to false if this is a simple type with lengthKind EOP.
| } | ||
| case None if this.isInstanceOf[Root] => LengthUnits.Characters | ||
| case _ => | ||
| Assert.invariantFailed( |
There was a problem hiding this comment.
I feel like this invariant might break if we have something like a global element decl with a child with EOP. That EOP will want to reach up to find where it's used but wont' be able to find a parent because it only looks lexically.
| nextSibling.isDefined && nextSibling.get.isInstanceOf[ModelGroup], | ||
| "%s is specified as dfdl:lengthKind=\"endOfParent\", but a model group is defined between this element and the end of the enclosing component", | ||
| context | ||
| ) |
There was a problem hiding this comment.
I think nextSibling is lexical, so I don't think it will detect errors if this is a global element decl that is referenced in a manner where it has siblings? Or maybe the context will be the element reference and the that global decl so it will work? Do we ahve tests forthis?
| case _ => // do nothing | ||
| } | ||
| schemaDefinitionWhen( | ||
| representation == Representation.Text && knownEncodingWidthInBits != 8 && parentEffectiveLengthUnits != LengthUnits.Characters, |
There was a problem hiding this comment.
I did some testing and change the CVS schema to this:
<element name="file" dfdl:lengthKind="explicit" dfdl:length="10" dfdl:terminator="%NL;">
<complexType>
<sequence>
<element name="field" type="xs:string" dfdl:lengthKind="endOfParent" />
</sequence>
</complexType>
</element>
And a got this stack trace:
org.apache.daffodil.lib.exceptions.Abort: Invariant broken: KnownEncodingMixin.this.isKnownEncoding
org.apache.daffodil.lib.exceptions.Assert$.abort(Assert.scala:153)
org.apache.daffodil.runtime1.processors.KnownEncodingMixin.knownEncodingName(EncodingRuntimeData.scala:56)
org.apache.daffodil.runtime1.processors.KnownEncodingMixin.knownEncodingName$(EncodingRuntimeData.scala:46)
org.apache.daffodil.core.dsom.LocalElementDeclBase.knownEncodingName$lzyINIT1(LocalElementDecl.scala:25)
at org.apache.daffodil.lib.exceptions.Assert$.abort(Assert.scala:153)
at org.apache.daffodil.runtime1.processors.KnownEncodingMixin.knownEncodingName(EncodingRuntimeData.scala:56)
at org.apache.daffodil.runtime1.processors.KnownEncodingMixin.knownEncodingName$(EncodingRuntimeData.scala:46)
at org.apache.daffodil.core.dsom.LocalElementDeclBase.knownEncodingName$lzyINIT1(LocalElementDecl.scala:25)
at org.apache.daffodil.core.dsom.LocalElementDeclBase.knownEncodingName(LocalElementDecl.scala:25)
at org.apache.daffodil.runtime1.processors.KnownEncodingMixin.knownEncodingCharset(EncodingRuntimeData.scala:62)
at org.apache.daffodil.runtime1.processors.KnownEncodingMixin.knownEncodingCharset$(EncodingRuntimeData.scala:46)
at org.apache.daffodil.core.dsom.LocalElementDeclBase.knownEncodingCharset$lzyINIT1(LocalElementDecl.scala:25)
at org.apache.daffodil.core.dsom.LocalElementDeclBase.knownEncodingCharset(LocalElementDecl.scala:25)
at org.apache.daffodil.runtime1.processors.KnownEncodingMixin.knownEncodingWidthInBits(EncodingRuntimeData.scala:81)
at org.apache.daffodil.runtime1.processors.KnownEncodingMixin.knownEncodingWidthInBits$(EncodingRuntimeData.scala:46)
at org.apache.daffodil.core.dsom.LocalElementDeclBase.knownEncodingWidthInBits$lzyINIT1(LocalElementDecl.scala:25)
at org.apache.daffodil.core.dsom.LocalElementDeclBase.knownEncodingWidthInBits(LocalElementDecl.scala:25)
at org.apache.daffodil.core.grammar.ElementBaseGrammarMixin.checkEndOfParentElem(ElementBaseGrammarMixin.scala:325)
at org.apache.daffodil.core.grammar.ElementBaseGrammarMixin.checkEndOfParentElem$(ElementBaseGrammarMixin.scala:49)
at org.apache.daffodil.core.dsom.LocalElementDeclBase.checkEndOfParentElem$lzyINIT1(LocalElementDecl.scala:25)
at org.apache.daffodil.core.dsom.LocalElementDeclBase.checkEndOfParentElem(LocalElementDecl.scala:25)
I think the issue is that the default encoding used by csv is UTF-8, which seems to cause problems here.
| schemaDefinitionWhen( | ||
| hasTerminator, | ||
| "%s is specified as dfdl:lengthKind=\"endOfParent\", but specifies a dfdl:terminator.", | ||
| context |
There was a problem hiding this comment.
I don't think we need to include context in the error string. I believe the error context is capture and output as part of the SDE.
| case LengthKind.EndOfParent => LengthMultipleOf(1) // NYI | ||
| case LengthKind.EndOfParent => | ||
| eb.immediatelyEnclosingElementParent match { | ||
| case Some(parent) => parent.elementSpecifiedLengthApprox |
There was a problem hiding this comment.
I don't think this is quite right. The length of this element isn't the same as the parent length, it's whatever is left over of the parent after the previous siblings.
So this elements length is kindof parent.elementSpecifiedLenghtApprox - priorAlignmentApprox (i.e the length of the parent minus wherever we are starting) but we can't just subtract approx things since they are potentially multiples.
That said, I wonder if we don't really need to get this elements approx length perfect, because no elements come after it, and the endingAlignApprox of the parent won't need this specific is going to be known since it has an explicit length? Maybe this just becomes LengthMultipleOf 1 or 8 depending on length units? This might need some more thought...
dfdl:lengthKind="endOfParent".endOfParentlengthKind.DAFFODIL-238