stevedlawrence commented on code in PR #1137:
URL: https://github.com/apache/daffodil/pull/1137#discussion_r1462073299
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -744,21 +744,41 @@ 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
+ 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.
+ */
+ lazy val isPathToOneWholeArrayOrOptional: Boolean = {
+ isNotRootOrSelf &&
+ (steps.last.isArray || steps.last.isOptional) &&
+ priorStepsHaveIndexingPredicates
+ }
+
+ private lazy val isNotRootOrSelf: Boolean = {
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
- }
+ else true
+ }
+
+ private lazy val priorStepsHaveIndexingPredicates: Boolean = {
+ steps.last.pred.isEmpty && // last cannot have a [N] pred
Review Comment:
Based on the name of this val, it feels like this `last.pred.isEmpty` check
shouldn't be done here since. For example, you could want to ask if
"priorStepsHaveIndxingPredicates" for a path not used in fn-count. In fact, the
answer to that question should always be true for all path expressions, right?
For all path expressions, all init steps must have a predicate if they are
arrays/optional.
So maybe the last.pred.isEmpty check wants to be somewhere else, and all
path expressions need something like
```
SDEUnless(priorStepsHaveIndexingPredicates)
```
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -744,21 +744,41 @@ 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
+ 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.
+ */
+ lazy val isPathToOneWholeArrayOrOptional: Boolean = {
+ isNotRootOrSelf &&
+ (steps.last.isArray || steps.last.isOptional) &&
+ priorStepsHaveIndexingPredicates
+ }
+
+ private lazy val isNotRootOrSelf: Boolean = {
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
- }
+ else true
+ }
+
+ private lazy val priorStepsHaveIndexingPredicates: Boolean = {
+ steps.last.pred.isEmpty && // last cannot have a [N] pred
+ steps.dropRight(1).forall {
Review Comment:
I believe this is the same as `steps.init.forall { ...`. Related, I wonder
if using `initSteps` instead of `priorSteps` might be more clear, since "init"
means a specific thing, at least in the scala world?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -744,21 +744,41 @@ 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
+ 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.
+ */
+ lazy val isPathToOneWholeArrayOrOptional: Boolean = {
+ isNotRootOrSelf &&
+ (steps.last.isArray || steps.last.isOptional) &&
+ priorStepsHaveIndexingPredicates
+ }
+
+ private lazy val isNotRootOrSelf: Boolean = {
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
- }
+ else true
+ }
+
+ private lazy val priorStepsHaveIndexingPredicates: Boolean = {
+ steps.last.pred.isEmpty && // 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
Review Comment:
What about `Up2`, `Self`, and `Self2`? Do we need exceptions for those other
kinds steps?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -969,16 +989,27 @@ sealed abstract class StepExpression(val step: String,
val pred: Option[Predicat
}
final lazy val isArray: Boolean = {
- val (arrays, scalars) = stepElements.partition { _.isArray }
- (arrays.length, scalars.length) match {
+ checkAmbiguousPath
+ stepElements.exists(_.isArray)
+ }
+
+ final lazy val isOptional: Boolean = {
+ checkAmbiguousPath
+ stepElements.exists(_.isOptional)
+ }
Review Comment:
These feel potentially confusing. I feel like our definition is usually an
element is either an array OR an optional, but never both. But with step
expressions, because of ambiguities, it's possible for isArray and isOptional
to both be true. That seems reasonable considering the way dpath works and
supports this ambiguity, but it might be worth adding a comment.
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_functions/Functions.tdml:
##########
@@ -5150,6 +5147,51 @@
</xs:complexType>
</xs:element>
+ <xs:element name="count07">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="int" type="xs:int" minOccurs="2" maxOccurs="2"
dfdl:representation="binary" />
+ <xs:element name="count" type="xs:int" dfdl:representation="binary"
dfdl:inputValueCalc="{ fn:count(../ex:int) }" />
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="count08">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="int" type="xs:int" dfdl:representation="binary" />
+ <xs:element name="count" type="xs:int" dfdl:representation="binary"
dfdl:inputValueCalc="{ fn:count(../ex:int) }" />
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="count09">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="int" 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:int) }" />
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="count10">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="int" type="xs:int" dfdl:representation="binary" />
+ <xs:element name="count" type="xs:int" dfdl:representation="binary"
dfdl:outputValueCalc="{ fn:count(../ex:int) }" />
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="count11">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="int" type="xs:int" dfdl:representation="binary" />
+ <xs:element name="count" type="xs:int" dfdl:representation="binary"
dfdl:occursCountKind="expression" dfdl:occursCount="{ fn:count(../ex:int) }" />
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
Review Comment:
Can you add a test that has a conditional inside the fn:count expression?
Maybe some with good or bad ambiguities and mixing arrays and optionals? e.g.
```xquery
fn:count(if (../foo) then ../someArray else ../someOptional)
```
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -744,21 +744,41 @@ 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
+ 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.
+ */
+ lazy val isPathToOneWholeArrayOrOptional: Boolean = {
+ isNotRootOrSelf &&
+ (steps.last.isArray || steps.last.isOptional) &&
+ priorStepsHaveIndexingPredicates
+ }
+
+ private lazy val isNotRootOrSelf: Boolean = {
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
- }
+ else true
+ }
+
+ private lazy val priorStepsHaveIndexingPredicates: Boolean = {
+ steps.last.pred.isEmpty && // 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 // no special case to check
for optional
Review Comment:
Optional elements do not need a predicate? I thought DPath treated them like
arrays so they must have a `[1]` predicate?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2295,6 +2295,18 @@ abstract class FunctionCallBase(
res
}
+ final def checkArgArray(): Unit = {
+ lazy val isArray = expressions.head match {
+ case rpe: RelativePathExpression => rpe.steps.last.isArray
+ case rpe: RootPathExpression => rpe.steps.last.isArray
+ case _ => true
Review Comment:
I don't think my original comment is addressed? Or are you saying there is a
check somewhere else within the type system that makes sure things work as
expected? Are there tests for this?
--
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]
