stevedlawrence commented on code in PR #1137:
URL: https://github.com/apache/daffodil/pull/1137#discussion_r1500799302
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -234,6 +223,15 @@ abstract class Expression extends OOLAGHostImpl() with
BasicComponent {
}
.get
}
+
+ /**
+ * Path to a single array element without a [N] predicate qualifying it
+ * or to an optional element.
+ *
+ * That is, the kind of expression used to access information
+ * about an array or optional such as its current count.
+ */
+ lazy val isPathToOneWholeArrayOrOptional: Boolean = false
Review Comment:
Can we remove this from `Expression` so it is only a member of
`PathExpression`? Seems odd to ever ask a generic `Expression` if it is a
special kind of path. Note that `isPathToOneWholeArray` is only a member of
PathExpression, so removing this would make the two functions consistent.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -744,21 +742,38 @@ abstract class PathExpression() extends Expression {
* about an array such as its current count.
*/
lazy val isPathToOneWholeArray: Boolean = {
- if (steps == Nil) false // root is never an array
- if (steps == Nil) false // root is never an array
- else if (isSelf) false // self path is never an array
- else
- steps.last.isArray &&
- !steps.last.pred.isDefined && // last cannot have a [N] pred
- steps.dropRight(1).forall {
- // for all the prior steps
- // if they mention an array element, there
- // must be a [N] predicate.
- step =>
- if (step.isInstanceOf[Up]) true
- else if (step.isArray) step.pred.isDefined
- else true
- }
+ isNotRootOrSelf &&
+ steps.last.isArray &&
+ priorStepsHaveIndexingPredicates
+ }
+
+ /**
+ * Path to a single array element without a [N] predicate qualifying it
+ * or to an optional element.
+ *
+ * That is, the kind of path expression used to access information
+ * about an array or optional such as its current count.
+ */
+ override lazy val isPathToOneWholeArrayOrOptional: Boolean = {
+ isNotRootOrSelf &&
+ steps.last.isArrayOrOptional &&
+ priorStepsHaveIndexingPredicates
+ }
+
+ private lazy val isNotRootOrSelf: Boolean = {
+ if (steps == Nil) false
+ else if (isSelf) false
+ else true
+ }
+
+ private lazy val priorStepsHaveIndexingPredicates: Boolean = {
Review Comment:
Can this val be removed? With the `isLastStep` addition in `NamedStep`, I
think we should now SDE if any prior steps are arrays without an index
predicate, so now the `steps.dropRight(1).forAll` check should always evaluate
to true an isn't really needed. I think we do still need the
`steps.last.pred.isEmpty` check but that probably makes more sense as an added
check in `isPathToOneWholeArrayOrOptional` and `isPathToOneWholeArray`.
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_functions/Functions.tdml:
##########
@@ -5212,6 +5212,80 @@
</xs:complexType>
</xs:element>
+ <xs:group name="countGroup">
+ <xs:sequence>
+ <xs:element name="count" type="xs:int" dfdl:representation="binary"
dfdl:inputValueCalc="{ fn:count(../ex:foo) }"/>
+ </xs:sequence>
+ </xs:group>
+ <xs:element name="scalarOptional">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="scalar">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="foo" type="xs:int"
dfdl:representation="binary"/>
+ <xs:group ref="countGroup"/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+ <xs:element name="optional">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="foo" type="xs:int" minOccurs="0"
dfdl:representation="binary"/>
+ <xs:group ref="countGroup"/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+ <xs:element name="scalarArray">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="scalar">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="foo" type="xs:int"
dfdl:representation="binary"/>
+ <xs:group ref="countGroup"/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+ <xs:element name="array">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="foo" type="xs:int" minOccurs="2"
maxOccurs="2" dfdl:representation="binary"/>
+ <xs:group ref="countGroup"/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+ <xs:element name="arrayOptional">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="array">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="foo" type="xs:int" minOccurs="2"
maxOccurs="2" dfdl:representation="binary"/>
+ <xs:group ref="countGroup"/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+ <xs:element name="optional">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="foo" type="xs:int" minOccurs="0"
maxOccurs="1" dfdl:representation="binary"/>
+ <xs:element name="count" type="xs:int"
dfdl:representation="binary" dfdl:inputValueCalc="{ fn:count(../ex:foo) }"/>
+ <!-- throws ClassCastException -->
Review Comment:
Does this ClassCastException still occur, or did newer changes fix this?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2378,7 +2414,7 @@ case class FNCountExpr(nameAsParsed: String, fnQName:
RefQName, args: List[Expre
}
case class FNExactlyOneExpr(nameAsParsed: String, fnQName: RefQName, args:
List[Expression])
- extends FunctionCallArrayBase(nameAsParsed, fnQName, args) {
+ extends FunctionCallArrayOrOptionalBase(nameAsParsed, fnQName, args) {
Review Comment:
Can we update this compiledDPath in this class to also call
`checkArgArrayOrOptional(0)`? We don't currently supported it, but if we do add
support it would be good to already have this check.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
