You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "stevedlawrence (via GitHub)" <gi...@apache.org> on 2023/05/25 17:17:26 UTC
[GitHub] [daffodil] stevedlawrence opened a new pull request, #1015: Fix separatorSuppressionPolicy="never"
stevedlawrence opened a new pull request, #1015:
URL: https://github.com/apache/daffodil/pull/1015
Currently, parsing with occursCountKind "implicit" and separator suppression policy "never" behaves similar to occursCountKind "fixed", in that it requires maxOccurs instances, but does not allow for absent representations. This is incorrect--SSP "never" only requires non-absent instance up to minOccurs, and afterwards allows absent occurrences as long as maxOccurs separators are found.
To fix this, this modifies parsing to use the same logic as trailingEmpty and trailingEmptyStrict, but adds additional logic at the end of parsing a repetition to ensure that there were no errors or missing separators. This is done by using the same Rep parser, but adding a new PositionalNever flag to differentiate the logic. A new arrayCompleteChecks function is used to check if this flag is set and create a parse error if errors or missing separators occurred. This also renames finalChecks to sequenceCompleteChecks to differentiate it from arrayCompleteChecks.
Currently, the unparsing logic is also broken for occursCountKind "implicit" and separator suppression policy "never" if there are no instances of an array/optional element. This is because the logic always looks for a "start" event, which may not exist if minOccurs is zero and all instances have an absent representation.
To fix this, this modifies unparseWithNoSuppression to not require "start" events and relies on the existing shouldDoUnparse function to determine if the current event is the right one to unparse. The existing logic will then output any missing separators as needed, with a slight tweak to handle an off-by-one error for infix separators.
DAFFODIL-2802, DAFFODIL-2499
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #1015: Fix separatorSuppressionPolicy="never"
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1015:
URL: https://github.com/apache/daffodil/pull/1015#discussion_r1207100835
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala:
##########
@@ -62,6 +62,7 @@ object SeparatedSequenceChildBehavior {
sealed abstract class PositionalLike extends Type
sealed abstract class PositionalTrailing extends PositionalLike
case object Positional extends PositionalLike
Review Comment:
Makes sense. I see know that the comment above these enums does say something to that effect.
What you describe sounds a lot like this new `PositionalNever` thing. I wonder if `PositionalNever` shouldn't be required and `Positional` should have the `arrayCompleteChecks`, but that there are some other bugs with `Positional`? I can't devise any tests that seem to work in correctly, so maybe we commit this as is, and eventually a bug will crop up that reveals if there is an actual issue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a diff in pull request #1015: Fix separatorSuppressionPolicy="never"
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1015:
URL: https://github.com/apache/daffodil/pull/1015#discussion_r1207022677
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala:
##########
@@ -62,6 +62,7 @@ object SeparatedSequenceChildBehavior {
sealed abstract class PositionalLike extends Type
sealed abstract class PositionalTrailing extends PositionalLike
case object Positional extends PositionalLike
Review Comment:
Positional means that the way you tell what element/child of a sequence you are dealing with is by counting which child it is. Only true when things are required, or have fixed number of children, excepting the last declared in the sequence, which can be optional, because we know it can't be anything else.
Non-Positional means the data has some way of identifying what child e.g., by initiators. In other words, it's ambiguous until you look at the data which it is. Hence, one ends up speculating, and trying to parse a child, and when it succeeds, then it "is one" and when it fails it isn't and you go on to the next declared child if it is optional.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence merged pull request #1015: Fix separatorSuppressionPolicy="never"
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence merged PR #1015:
URL: https://github.com/apache/daffodil/pull/1015
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a diff in pull request #1015: Fix separatorSuppressionPolicy="never"
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1015:
URL: https://github.com/apache/daffodil/pull/1015#discussion_r1206038854
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- // val isBounded = unparser.isBoundedMax // not needed for the no-suppression case
- //
- // The number of occurrances we unparse is always exactly driven
- // by the number of infoset events for the repeating/optional element.
- //
- // For RepUnparser - array/optional case - in all cases we should get a
- // startArray event. If we don't then
- // the element must be entirely optional, so we get no events for it
- // at all.
- //
+ Assert.invariant(state.inspect)
+ val ev = state.inspectAccessor
+ val isArr = erd.isArray
- if (state.inspect) {
- val ev = state.inspectAccessor
- val isArr = erd.isArray
- if (ev.isStart && (isArr || erd.isOptional)) {
- if (ev.erd eq erd) {
- //
- // StartArray for this unparser's array element
- //
- unparser.startArrayOrOptional(state)
- while ({
- doUnparser = unparser.shouldDoUnparser(unparser, state)
- doUnparser
- }) {
- //
- // These are so we can check invariants on these stacks being
- // pushed and popped reliably, and incremented only once
- //
- val arrayIterationIndexBefore = state.arrayIterationPos
- val arrayIterationIndexStackDepthBefore =
- state.arrayIterationIndexStack.length
- val occursIndexBefore = state.occursPos
- val occursIndexStackDepthBefore = state.occursIndexStack.length
- val groupIndexBefore = state.groupPos
- val groupIndexStackDepthBefore = state.groupIndexStack.length
+ // If the event is for this Rep unparser, we need to consume the StartArray event
+ if (ev.erd eq erd) {
+ unparser.startArrayOrOptional(state)
+ }
- Assert.invariant(
- erd.isRepresented,
- ) // since this is an array, can't have inputValueCalc
+ // Unparse each occurrence of this array in the infoset. Note that there could be zero
+ // occurrences
+ while ({
+ doUnparser = unparser.shouldDoUnparser(unparser, state)
+ doUnparser
+ }) {
+ //
+ // These are so we can check invariants on these stacks being
+ // pushed and popped reliably, and incremented only once
+ //
+ val arrayIterationIndexBefore = state.arrayIterationPos
+ val arrayIterationIndexStackDepthBefore =
+ state.arrayIterationIndexStack.length
+ val occursIndexBefore = state.occursPos
+ val occursIndexStackDepthBefore = state.occursIndexStack.length
+ val groupIndexBefore = state.groupPos
+ val groupIndexStackDepthBefore = state.groupIndexStack.length
+
+ Assert.invariant(
+ erd.isRepresented,
+ ) // since this is an array, can't have inputValueCalc
+
+ if (isArr)
Review Comment:
I don't see how isArr could be false. I think we're in a context here where it has to be true.
I believe we're still in the `case unparser: RepOrderedSeparatedSequenceChildUnparser => `
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
Review Comment:
Right here I'd insert an `Assert.invariant(erd.isArray || erd.isOptional)` because it is the fact that the child unparser is a RepOrderedSeparatedSequenceChildUnparser that tells us for sure this is an array/optional. I am a bit uncertain about the isOptional test. It may be that it must be an array full stop.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala:
##########
@@ -62,6 +62,7 @@ object SeparatedSequenceChildBehavior {
sealed abstract class PositionalLike extends Type
sealed abstract class PositionalTrailing extends PositionalLike
case object Positional extends PositionalLike
Review Comment:
is Positional still used? If not eliminate it. If it is still used we need a comment about what it represents because the others seem to cover all the bases.
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- // val isBounded = unparser.isBoundedMax // not needed for the no-suppression case
- //
- // The number of occurrances we unparse is always exactly driven
- // by the number of infoset events for the repeating/optional element.
- //
- // For RepUnparser - array/optional case - in all cases we should get a
- // startArray event. If we don't then
- // the element must be entirely optional, so we get no events for it
- // at all.
- //
+ Assert.invariant(state.inspect)
+ val ev = state.inspectAccessor
+ val isArr = erd.isArray
- if (state.inspect) {
- val ev = state.inspectAccessor
- val isArr = erd.isArray
- if (ev.isStart && (isArr || erd.isOptional)) {
- if (ev.erd eq erd) {
- //
- // StartArray for this unparser's array element
- //
- unparser.startArrayOrOptional(state)
- while ({
- doUnparser = unparser.shouldDoUnparser(unparser, state)
- doUnparser
- }) {
- //
- // These are so we can check invariants on these stacks being
- // pushed and popped reliably, and incremented only once
- //
- val arrayIterationIndexBefore = state.arrayIterationPos
- val arrayIterationIndexStackDepthBefore =
- state.arrayIterationIndexStack.length
- val occursIndexBefore = state.occursPos
- val occursIndexStackDepthBefore = state.occursIndexStack.length
- val groupIndexBefore = state.groupPos
- val groupIndexStackDepthBefore = state.groupIndexStack.length
+ // If the event is for this Rep unparser, we need to consume the StartArray event
+ if (ev.erd eq erd) {
+ unparser.startArrayOrOptional(state)
+ }
- Assert.invariant(
- erd.isRepresented,
- ) // since this is an array, can't have inputValueCalc
+ // Unparse each occurrence of this array in the infoset. Note that there could be zero
+ // occurrences
+ while ({
+ doUnparser = unparser.shouldDoUnparser(unparser, state)
+ doUnparser
+ }) {
+ //
+ // These are so we can check invariants on these stacks being
+ // pushed and popped reliably, and incremented only once
+ //
+ val arrayIterationIndexBefore = state.arrayIterationPos
+ val arrayIterationIndexStackDepthBefore =
+ state.arrayIterationIndexStack.length
+ val occursIndexBefore = state.occursPos
+ val occursIndexStackDepthBefore = state.occursIndexStack.length
+ val groupIndexBefore = state.groupPos
+ val groupIndexStackDepthBefore = state.groupIndexStack.length
+
+ Assert.invariant(
Review Comment:
We should move this up to where the erd is first made accessible.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SequenceChildParseResultHelper.scala:
##########
@@ -87,10 +87,22 @@ trait SequenceChildParseResultHelper extends Serializable {
requiredOptional: RequiredOptionalStatus,
): ParseAttemptStatus
+ /**
+ * Overridden for Positional case.
+ */
+ def arrayCompleteChecks(
+ parser: SequenceChildParser,
+ pstate: PState,
+ resultOfTry: ParseAttemptStatus,
+ priorResultOfTry: ParseAttemptStatus,
+ ): Unit = {
Review Comment:
Codecov warning here suggests this should just be an prototype method without a default implementation that does nothing.
Otherwise I think we should create a test to exercise this. It's going to be for some non-positional dfdl:separatorSuppressionPolicy='anyEmpty' case I think.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/SequenceChild.scala:
##########
@@ -465,7 +465,7 @@ class ScalarOrderedSequenceChild(sq: SequenceTermBase, term: Term, groupIndex: I
isModelGroupRepPossiblyZeroLength,
isModelGroupRepNonZeroLength,
)
- case Positional =>
+ case Positional | PositionalNever =>
Review Comment:
You added PositionalNever, but I can't see anywhere that it isn't treated as equivalent to Positional, which is what the value used to be. Can PositionalNever be eliminated again?
##########
daffodil-test/src/test/resources/org/apache/daffodil/usertests/SepTests.tdml:
##########
@@ -268,7 +268,7 @@
<!-- Test for DAFFODIL-2499. empty strings should not be creating empty elements here. -->
<tdml:parserTestCase name="test_sep_ssp_never_3" root="file1" model="s3"
- implementations="ibm">
+ implementations="daffodil ibm">
Review Comment:
We really need to fix the cross-tester and grow an ability to see if these tests work on IBM. We have never actually made any of our test suite run against IBM DFDL. Just specific schemas like EDIFACT.
That goes beyond this change set though.
##########
daffodil-test/src/test/scala/org/apache/daffodil/usertests/TestSepTests.scala:
##########
@@ -47,28 +47,18 @@ class TestSepTests {
runner.runOneTest("test_sep_trailingEmptyStrict_2")
}
- // DAFFODIL-2499 - separatorSuppressionPolicy 'never'
- // Note: this test isn't commented out, because it works for IBM DFDL in cross testing
- // The TDML for this test just has it disabled for the daffodil implementation.
- // Add daffodil to implementations to see the erroneous daffodil behavior.
Review Comment:
Hmmm. This suggests these tests were run against IBM DFDL to verify portability..... I am not sure we have actually done that except way back I attempted this, but this is years ago. and there were several hundred tests that failed, mostly due to comparison logic not taking type into consideration.
That work all predated our C-codegen back-end and the TDML refactoring to allow tests to run on the original backend or the C backend.
Maybe we can make it so if found the IBM test rig would dynamically load and then tests marked for ibm would run on IBM, but not otherwise.
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- // val isBounded = unparser.isBoundedMax // not needed for the no-suppression case
- //
- // The number of occurrances we unparse is always exactly driven
- // by the number of infoset events for the repeating/optional element.
- //
- // For RepUnparser - array/optional case - in all cases we should get a
- // startArray event. If we don't then
- // the element must be entirely optional, so we get no events for it
- // at all.
- //
+ Assert.invariant(state.inspect)
+ val ev = state.inspectAccessor
+ val isArr = erd.isArray
- if (state.inspect) {
- val ev = state.inspectAccessor
- val isArr = erd.isArray
- if (ev.isStart && (isArr || erd.isOptional)) {
- if (ev.erd eq erd) {
- //
- // StartArray for this unparser's array element
- //
- unparser.startArrayOrOptional(state)
- while ({
- doUnparser = unparser.shouldDoUnparser(unparser, state)
- doUnparser
- }) {
- //
- // These are so we can check invariants on these stacks being
- // pushed and popped reliably, and incremented only once
- //
- val arrayIterationIndexBefore = state.arrayIterationPos
- val arrayIterationIndexStackDepthBefore =
- state.arrayIterationIndexStack.length
- val occursIndexBefore = state.occursPos
- val occursIndexStackDepthBefore = state.occursIndexStack.length
- val groupIndexBefore = state.groupPos
- val groupIndexStackDepthBefore = state.groupIndexStack.length
+ // If the event is for this Rep unparser, we need to consume the StartArray event
+ if (ev.erd eq erd) {
+ unparser.startArrayOrOptional(state)
+ }
- Assert.invariant(
- erd.isRepresented,
- ) // since this is an array, can't have inputValueCalc
+ // Unparse each occurrence of this array in the infoset. Note that there could be zero
+ // occurrences
+ while ({
+ doUnparser = unparser.shouldDoUnparser(unparser, state)
+ doUnparser
+ }) {
+ //
+ // These are so we can check invariants on these stacks being
+ // pushed and popped reliably, and incremented only once
+ //
+ val arrayIterationIndexBefore = state.arrayIterationPos
+ val arrayIterationIndexStackDepthBefore =
+ state.arrayIterationIndexStack.length
+ val occursIndexBefore = state.occursPos
+ val occursIndexStackDepthBefore = state.occursIndexStack.length
+ val groupIndexBefore = state.groupPos
+ val groupIndexStackDepthBefore = state.groupIndexStack.length
+
+ Assert.invariant(
+ erd.isRepresented,
+ ) // since this is an array, can't have inputValueCalc
+
+ if (isArr)
+ if (state.dataProc.isDefined)
+ state.dataProc.get.beforeRepetition(state, this)
+
+ unparseOne(unparser, erd, state)
+ numOccurrences += 1
+ Assert.invariant(
+ state.arrayIterationIndexStack.length == arrayIterationIndexStackDepthBefore,
+ )
+ state.moveOverOneArrayIterationIndexOnly()
+ Assert.invariant(state.arrayIterationPos == arrayIterationIndexBefore + 1)
- if (isArr)
- if (state.dataProc.isDefined)
- state.dataProc.get.beforeRepetition(state, this)
+ Assert.invariant(state.occursIndexStack.length == occursIndexStackDepthBefore)
+ state.moveOverOneOccursIndexOnly()
+ Assert.invariant(state.occursPos == occursIndexBefore + 1)
- unparseOne(unparser, erd, state)
- numOccurrences += 1
- Assert.invariant(
- state.arrayIterationIndexStack.length == arrayIterationIndexStackDepthBefore,
- )
- state.moveOverOneArrayIterationIndexOnly()
- Assert.invariant(state.arrayIterationPos == arrayIterationIndexBefore + 1)
+ Assert.invariant(state.groupIndexStack.length == groupIndexStackDepthBefore)
+ state.moveOverOneGroupIndexOnly() // array elements are always represented.
+ Assert.invariant(state.groupPos == groupIndexBefore + 1)
- Assert.invariant(state.occursIndexStack.length == occursIndexStackDepthBefore)
- state.moveOverOneOccursIndexOnly()
- Assert.invariant(state.occursPos == occursIndexBefore + 1)
-
- Assert.invariant(state.groupIndexStack.length == groupIndexStackDepthBefore)
- state.moveOverOneGroupIndexOnly() // array elements are always represented.
- Assert.invariant(state.groupPos == groupIndexBefore + 1)
+ if (isArr)
Review Comment:
Again I think isArr is guaranteed true.
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- // val isBounded = unparser.isBoundedMax // not needed for the no-suppression case
- //
- // The number of occurrances we unparse is always exactly driven
- // by the number of infoset events for the repeating/optional element.
- //
- // For RepUnparser - array/optional case - in all cases we should get a
- // startArray event. If we don't then
- // the element must be entirely optional, so we get no events for it
- // at all.
- //
+ Assert.invariant(state.inspect)
+ val ev = state.inspectAccessor
+ val isArr = erd.isArray
- if (state.inspect) {
- val ev = state.inspectAccessor
- val isArr = erd.isArray
- if (ev.isStart && (isArr || erd.isOptional)) {
- if (ev.erd eq erd) {
- //
- // StartArray for this unparser's array element
- //
- unparser.startArrayOrOptional(state)
- while ({
- doUnparser = unparser.shouldDoUnparser(unparser, state)
- doUnparser
- }) {
- //
- // These are so we can check invariants on these stacks being
- // pushed and popped reliably, and incremented only once
- //
- val arrayIterationIndexBefore = state.arrayIterationPos
- val arrayIterationIndexStackDepthBefore =
- state.arrayIterationIndexStack.length
- val occursIndexBefore = state.occursPos
- val occursIndexStackDepthBefore = state.occursIndexStack.length
- val groupIndexBefore = state.groupPos
- val groupIndexStackDepthBefore = state.groupIndexStack.length
+ // If the event is for this Rep unparser, we need to consume the StartArray event
+ if (ev.erd eq erd) {
Review Comment:
Comment is unclear. Why wouldn't this also be true of elements within the array?
I get how the first time the ev.erd matches the erd (when it did not before) that we are transitioning to a new array, but we're not keeping track of whether this is the first time or not here are we?
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- // val isBounded = unparser.isBoundedMax // not needed for the no-suppression case
- //
- // The number of occurrances we unparse is always exactly driven
- // by the number of infoset events for the repeating/optional element.
- //
- // For RepUnparser - array/optional case - in all cases we should get a
- // startArray event. If we don't then
- // the element must be entirely optional, so we get no events for it
- // at all.
- //
+ Assert.invariant(state.inspect)
Review Comment:
If instead of removing the if-then and adding the Assert.invariant, you could have done:
```
if (!state.inspect) Assert.invariantFailed(....)
else {
```
Then all the code wouldn't be re-indented and the diff would be more meaningful.
Perhaps put this back in that form, and change it late in the process to just the Assert.invariant.
Or maybe just putting in a extra set of block "{ .... }" would cause all the indentation to remain as it was.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/SequenceChild.scala:
##########
@@ -465,7 +465,7 @@ class ScalarOrderedSequenceChild(sq: SequenceTermBase, term: Term, groupIndex: I
isModelGroupRepPossiblyZeroLength,
isModelGroupRepNonZeroLength,
)
- case Positional =>
+ case Positional | PositionalNever =>
Review Comment:
Ok. I found it eventually. There appears to be exactly one place where this is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on a diff in pull request #1015: Fix separatorSuppressionPolicy="never"
Posted by "tuxji (via GitHub)" <gi...@apache.org>.
tuxji commented on code in PR #1015:
URL: https://github.com/apache/daffodil/pull/1015#discussion_r1206003308
##########
daffodil-test/src/test/resources/org/apache/daffodil/usertests/SepTests.tdml:
##########
@@ -223,7 +223,7 @@
<!-- Test for DAFFODIL-2499. empty strings should not be creating empty elements here. -->
<tdml:parserTestCase name="test_sep_ssp_never_1" root="file1" model="s3"
- implementations="ibm">
+ implementations="daffodil ibm">
<!--
This test doesn't work on Daffodil due to DAFFODIL-2499.
Review Comment:
Since you've enabled this test, this test must be working on Daffodil now so you should remove or change this comment saying the test doesn't work on Daffodil.
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- // val isBounded = unparser.isBoundedMax // not needed for the no-suppression case
- //
- // The number of occurrances we unparse is always exactly driven
- // by the number of infoset events for the repeating/optional element.
- //
- // For RepUnparser - array/optional case - in all cases we should get a
- // startArray event. If we don't then
- // the element must be entirely optional, so we get no events for it
- // at all.
- //
+ Assert.invariant(state.inspect)
Review Comment:
It's hard for me to find the else branch that went away in this diff. This function is just too long and complicated to make it easy to understand how the logic has changed only from looking at the diff. Oh well, please add a comment at least explaining why state.inspect=true is an invariant now when it wasn't an invariant on this code path before.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala:
##########
@@ -94,7 +95,30 @@ trait SeparatedSequenceChildParseResultHelper extends SequenceChildParseResultHe
* Define this as final here so we aren't creating proliferation of
* traits/classes just for this one little issue.
Review Comment:
How does defining a final method avoid the need for more than one trait?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #1015: Fix separatorSuppressionPolicy="never"
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1015:
URL: https://github.com/apache/daffodil/pull/1015#discussion_r1206728978
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala:
##########
@@ -94,7 +95,30 @@ trait SeparatedSequenceChildParseResultHelper extends SequenceChildParseResultHe
* Define this as final here so we aren't creating proliferation of
* traits/classes just for this one little issue.
Review Comment:
I just copied this from the old `finalChecks` function--this follows the exact same pattern just called in a different place.
I think the idea is that things that implement this `SeparatedSequenceChildParseResultHelper` never have to think about implementing slightly different variants of this function, which could potentially lead to different traits with slightly different implementations?
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- // val isBounded = unparser.isBoundedMax // not needed for the no-suppression case
- //
- // The number of occurrances we unparse is always exactly driven
- // by the number of infoset events for the repeating/optional element.
- //
- // For RepUnparser - array/optional case - in all cases we should get a
- // startArray event. If we don't then
- // the element must be entirely optional, so we get no events for it
- // at all.
- //
+ Assert.invariant(state.inspect)
Review Comment:
Personally I'd rather remove unnecessary indentation. It sometimes makes diffs a bit uglier but I think cleaner code is worth it. Also, if you hide diff whitespace by clicking the above cog, I think it makes the diff a bit more readable and shows that the changes here are more rasonable, and it more clearly shows which else- blocks have been removed (which is actually all of them).
Here's a screen shot of where to cog is--it's kindof a hidden feature:
> 
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
Review Comment:
I added `Assert.invariant(erd.isArray)` and multiple tests failed, so I can confirm that `RepOrderedSeparatedSequenceChildUnparser` is used for both arrays and optionals.
The suggested assertions works as expected.
##########
daffodil-test/src/test/scala/org/apache/daffodil/usertests/TestSepTests.scala:
##########
@@ -47,28 +47,18 @@ class TestSepTests {
runner.runOneTest("test_sep_trailingEmptyStrict_2")
}
- // DAFFODIL-2499 - separatorSuppressionPolicy 'never'
- // Note: this test isn't commented out, because it works for IBM DFDL in cross testing
- // The TDML for this test just has it disabled for the daffodil implementation.
- // Add daffodil to implementations to see the erroneous daffodil behavior.
Review Comment:
Yeah, we need some kind of solution for this. I think it's made a bit complicated because IBM DFDL and Daffodil have conflicting dependencies (I think it's just ICU at the moment). So we need something like OSGI, or special classpath loaders or something so that running Daffodil vs IBM DFDL uses the correct classpath. It's also made harder since sbt doesn't make it easy to change classpaths.
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- // val isBounded = unparser.isBoundedMax // not needed for the no-suppression case
- //
- // The number of occurrances we unparse is always exactly driven
- // by the number of infoset events for the repeating/optional element.
- //
- // For RepUnparser - array/optional case - in all cases we should get a
- // startArray event. If we don't then
- // the element must be entirely optional, so we get no events for it
- // at all.
- //
+ Assert.invariant(state.inspect)
+ val ev = state.inspectAccessor
+ val isArr = erd.isArray
- if (state.inspect) {
- val ev = state.inspectAccessor
- val isArr = erd.isArray
- if (ev.isStart && (isArr || erd.isOptional)) {
- if (ev.erd eq erd) {
- //
- // StartArray for this unparser's array element
- //
- unparser.startArrayOrOptional(state)
- while ({
- doUnparser = unparser.shouldDoUnparser(unparser, state)
- doUnparser
- }) {
- //
- // These are so we can check invariants on these stacks being
- // pushed and popped reliably, and incremented only once
- //
- val arrayIterationIndexBefore = state.arrayIterationPos
- val arrayIterationIndexStackDepthBefore =
- state.arrayIterationIndexStack.length
- val occursIndexBefore = state.occursPos
- val occursIndexStackDepthBefore = state.occursIndexStack.length
- val groupIndexBefore = state.groupPos
- val groupIndexStackDepthBefore = state.groupIndexStack.length
+ // If the event is for this Rep unparser, we need to consume the StartArray event
+ if (ev.erd eq erd) {
Review Comment:
This check is outside the while-loop that consume the actual elements.
So we first consume the StartArray event (and do nothing for optionals) and then do the while-loop that parses all the StartElement/EndElement events. Once the while-loop ends, we unparse any missing separators, and then consume the the EndArray event.
If there are zero occurrences of this rep unparser, then we won't consume any events, and all we'll do is unparse all the needed separators.
I think looking at the code itself or with diff whitespace hidden makes this a bit more clear.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala:
##########
@@ -62,6 +62,7 @@ object SeparatedSequenceChildBehavior {
sealed abstract class PositionalLike extends Type
sealed abstract class PositionalTrailing extends PositionalLike
case object Positional extends PositionalLike
Review Comment:
It is still used. Honestly, I don't really understand what it represents, but a number of tests fail if it's not used. We do seem to need this distinction between Positional and PositionalNever. If you have any idea what it represents I can add a comment.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SequenceChildParseResultHelper.scala:
##########
@@ -87,10 +87,22 @@ trait SequenceChildParseResultHelper extends Serializable {
requiredOptional: RequiredOptionalStatus,
): ParseAttemptStatus
+ /**
+ * Overridden for Positional case.
+ */
+ def arrayCompleteChecks(
+ parser: SequenceChildParser,
+ pstate: PState,
+ resultOfTry: ParseAttemptStatus,
+ priorResultOfTry: ParseAttemptStatus,
+ ): Unit = {
Review Comment:
I tracked this down, and this is just dead code that is impossible to hit.
The only `SequenceChildParserResultHelpers` implementations that need the `arrayCompleteChecks` function are the ones where the corresponding implementation of `SequenceChildParser` overrides `arrayCompleteChecks` to call `helper.arrayCompleteChecks(...)`. Not all `SequenceChildParsers` need a helper (at least according to the interface), so not all helpers need the function.
The same issue exists for the old `finalChecks` function (renamed to `sequenceCompleteChecks` in this PR). Both implementations can be removed.
##########
daffodil-test/src/test/resources/org/apache/daffodil/usertests/SepTests.tdml:
##########
@@ -223,7 +223,7 @@
<!-- Test for DAFFODIL-2499. empty strings should not be creating empty elements here. -->
<tdml:parserTestCase name="test_sep_ssp_never_1" root="file1" model="s3"
- implementations="ibm">
+ implementations="daffodil ibm">
<!--
This test doesn't work on Daffodil due to DAFFODIL-2499.
Review Comment:
Done
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- // val isBounded = unparser.isBoundedMax // not needed for the no-suppression case
- //
- // The number of occurrances we unparse is always exactly driven
- // by the number of infoset events for the repeating/optional element.
- //
- // For RepUnparser - array/optional case - in all cases we should get a
- // startArray event. If we don't then
- // the element must be entirely optional, so we get no events for it
- // at all.
- //
+ Assert.invariant(state.inspect)
+ val ev = state.inspectAccessor
+ val isArr = erd.isArray
- if (state.inspect) {
- val ev = state.inspectAccessor
- val isArr = erd.isArray
- if (ev.isStart && (isArr || erd.isOptional)) {
- if (ev.erd eq erd) {
- //
- // StartArray for this unparser's array element
- //
- unparser.startArrayOrOptional(state)
- while ({
- doUnparser = unparser.shouldDoUnparser(unparser, state)
- doUnparser
- }) {
- //
- // These are so we can check invariants on these stacks being
- // pushed and popped reliably, and incremented only once
- //
- val arrayIterationIndexBefore = state.arrayIterationPos
- val arrayIterationIndexStackDepthBefore =
- state.arrayIterationIndexStack.length
- val occursIndexBefore = state.occursPos
- val occursIndexStackDepthBefore = state.occursIndexStack.length
- val groupIndexBefore = state.groupPos
- val groupIndexStackDepthBefore = state.groupIndexStack.length
+ // If the event is for this Rep unparser, we need to consume the StartArray event
+ if (ev.erd eq erd) {
+ unparser.startArrayOrOptional(state)
+ }
- Assert.invariant(
- erd.isRepresented,
- ) // since this is an array, can't have inputValueCalc
+ // Unparse each occurrence of this array in the infoset. Note that there could be zero
+ // occurrences
+ while ({
+ doUnparser = unparser.shouldDoUnparser(unparser, state)
+ doUnparser
+ }) {
+ //
+ // These are so we can check invariants on these stacks being
+ // pushed and popped reliably, and incremented only once
+ //
+ val arrayIterationIndexBefore = state.arrayIterationPos
+ val arrayIterationIndexStackDepthBefore =
+ state.arrayIterationIndexStack.length
+ val occursIndexBefore = state.occursPos
+ val occursIndexStackDepthBefore = state.occursIndexStack.length
+ val groupIndexBefore = state.groupPos
+ val groupIndexStackDepthBefore = state.groupIndexStack.length
+
+ Assert.invariant(
Review Comment:
Done
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/SequenceChild.scala:
##########
@@ -465,7 +465,7 @@ class ScalarOrderedSequenceChild(sq: SequenceTermBase, term: Term, groupIndex: I
isModelGroupRepPossiblyZeroLength,
isModelGroupRepNonZeroLength,
)
- case Positional =>
+ case Positional | PositionalNever =>
Review Comment:
There are a few cases where we do still assign `Positional`:
https://github.com/apache/daffodil/blob/main/daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/SequenceChild.scala#L131-L159
Those cases do not use the `PositionalNever` logic. I originally did not have this new `PositionalNever` thing and applied this check for all `Positional` children, but it broke a large number of tests. I admit I couldn't entirely follow the logic as to why, but I tried to add tests to make convince myself it was working as expected. We may need more tests to cover cases I haden't considered, if you have any thoughts let me know.
##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/SeparatedSequenceUnparsers.scala:
##########
@@ -612,120 +612,96 @@ class OrderedSeparatedSequenceUnparser(
val erd = unparser.erd
var numOccurrences = 0
val maxReps = unparser.maxRepeats(state)
- // val isBounded = unparser.isBoundedMax // not needed for the no-suppression case
- //
- // The number of occurrances we unparse is always exactly driven
- // by the number of infoset events for the repeating/optional element.
- //
- // For RepUnparser - array/optional case - in all cases we should get a
- // startArray event. If we don't then
- // the element must be entirely optional, so we get no events for it
- // at all.
- //
+ Assert.invariant(state.inspect)
+ val ev = state.inspectAccessor
+ val isArr = erd.isArray
- if (state.inspect) {
- val ev = state.inspectAccessor
- val isArr = erd.isArray
- if (ev.isStart && (isArr || erd.isOptional)) {
- if (ev.erd eq erd) {
- //
- // StartArray for this unparser's array element
- //
- unparser.startArrayOrOptional(state)
- while ({
- doUnparser = unparser.shouldDoUnparser(unparser, state)
- doUnparser
- }) {
- //
- // These are so we can check invariants on these stacks being
- // pushed and popped reliably, and incremented only once
- //
- val arrayIterationIndexBefore = state.arrayIterationPos
- val arrayIterationIndexStackDepthBefore =
- state.arrayIterationIndexStack.length
- val occursIndexBefore = state.occursPos
- val occursIndexStackDepthBefore = state.occursIndexStack.length
- val groupIndexBefore = state.groupPos
- val groupIndexStackDepthBefore = state.groupIndexStack.length
+ // If the event is for this Rep unparser, we need to consume the StartArray event
+ if (ev.erd eq erd) {
+ unparser.startArrayOrOptional(state)
+ }
- Assert.invariant(
- erd.isRepresented,
- ) // since this is an array, can't have inputValueCalc
+ // Unparse each occurrence of this array in the infoset. Note that there could be zero
+ // occurrences
+ while ({
+ doUnparser = unparser.shouldDoUnparser(unparser, state)
+ doUnparser
+ }) {
+ //
+ // These are so we can check invariants on these stacks being
+ // pushed and popped reliably, and incremented only once
+ //
+ val arrayIterationIndexBefore = state.arrayIterationPos
+ val arrayIterationIndexStackDepthBefore =
+ state.arrayIterationIndexStack.length
+ val occursIndexBefore = state.occursPos
+ val occursIndexStackDepthBefore = state.occursIndexStack.length
+ val groupIndexBefore = state.groupPos
+ val groupIndexStackDepthBefore = state.groupIndexStack.length
+
+ Assert.invariant(
+ erd.isRepresented,
+ ) // since this is an array, can't have inputValueCalc
+
+ if (isArr)
Review Comment:
These rep unparsers are used for both arrays and optionals. If isOptional is true, then isArray is always false.
##########
daffodil-test/src/test/resources/org/apache/daffodil/usertests/SepTests.tdml:
##########
@@ -268,7 +268,7 @@
<!-- Test for DAFFODIL-2499. empty strings should not be creating empty elements here. -->
<tdml:parserTestCase name="test_sep_ssp_never_3" root="file1" model="s3"
- implementations="ibm">
+ implementations="daffodil ibm">
Review Comment:
Agreed. Might be a pretty large effort though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org