[GitHub] [daffodil] mbeckerle commented on a diff in pull request #931: Fix issue with groups containing choices: WIP

["mbeckerle (via GitHub)" <gi...@apache.org>]

mbeckerle commented on code in PR #931:
URL: https://github.com/apache/daffodil/pull/931#discussion_r1086790095


##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/PartialNextElementResolver.scala:
##########
@@ -302,7 +302,7 @@ class SeveralPossibilitiesForNextElement(
       }
 
     val res = optnextERD
-    if (isRequiredStreamingUnparserEvent && res.isEmpty) {
+    if (isRequiredStreamingUnparserEvent && res.isEmpty && !trd.isInstanceOf[ChoiceRuntimeData]) {

Review Comment:
   It's not at all unlikely that your added predicate fixes an oversight in the original code. 
   However, we do need a rationale for this, and we need to put a comment in that contains that rationale. 
   
   This section of the code is checking whether we found a unique ERD corresponding to the incoming infoset element event, or not. 
   
   The predicate you have enhanced is testing for 
   
   1. the element is required to appear in the data stream (not optional element within the current group)
   2. no ERD is found for it. (in this ERD resolver from this TRD on the trd stack)
   
   You've added that it is also only an error if the current term is not a choice group.  If it is not an error, then the search will continue with the next resolver down the stack.  I think Steve is perhaps right that this is not about choices in general, but about choices with empty or all-optional branches. 
   
   I'm inclined to think what is wrong is more likely to be that isRequiredStreamingUnparserEvent is true when it should be false.  That's my theory anyway. 
   
   As for how to test this, ... Re: steve's thought: _Another thought, should all choices skip this logic like you have now, or is this specific to choices that could have a branch with no element (e.g. empty sequence, all optional elements)?_
   
   We should create tests that try choices where there are branches which are not the first branch, and which contain: 
   - nothing (empty sequence)
   - syntax only (like a terminator, but on an otherwise empty sequence)
   - optional content only (two cases: a nested choice with an empty sequence branch, and minOccurs=0)
   
   We definitely also need a test where the shared group is a sequence with only optional content. 
   
   Once we have those tests, I think we'll be in a better position to write a rationale and understand if we're missing a concept for choices, or if the problem is with isRequiredStreamingUnparserEvent, or ... something else entirely. 
   



-- 
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