stevedlawrence commented on code in PR #1137:
URL: https://github.com/apache/daffodil/pull/1137#discussion_r1479861987
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -744,21 +753,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
+ 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 // root is never an array
else if (isSelf) false // self path is never an array
Review Comment:
These comments mention "an array", but this val name no longer has anything
to do with this being an array or not. Suggest we just remove them to avoid
confusion. The name now is pretty clear what it's checking and it directly
matches the code.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2295,6 +2332,20 @@ abstract class FunctionCallBase(
res
}
+ protected def checkArgArrayOrOptional(): Unit = {
Review Comment:
If this function is going to be in FunctionCallBase, then it would be useful
if it could be generic enough to work for any possible function. Right now, it
only looks at expression.head, so it's only useful for functions that require
array/optional as the first function parameter. I'd suggest changing it to
allow it to pass in an index that can be used to pick which expression it
checks. The fn:count function would call it like `checkArgArrayOrOption(0)`.
Alternatively, move this logic into FunctionCallArrayBase, since it's likely
the only thing that need will need this kindof check. It already has
`arrPath.isPathToOneWholeArray`, maybe that just needs to be updated to require
this?
That said, I'm not sure why arrayPath.isPathToOneWholeArray in
FunctionCallArrayBase doesn't already result in an SDE. Seems like that's
already doing the check we want. I wonder if there's just a bug there and
that's not getting checked correctly, and we don't actually need this new
checkArgArrayOrOptional check?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -744,21 +753,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
+ 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 // 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 {
+ case _: Up => true
+ case _: Up2 => SDE("Up2 step must not have indexing")
+ case _: SelfStepExpression => SDE("Self step must not have indexing")
+ case step => !step.isArray || step.pred.isDefined
Review Comment:
I would expect `Up` and `Up2` to have the exact same logic. They are
basically the same thing, just one asserts the parent QName must be a specific
name. I'm not sure why one is `true` and the other is an SDE? I would expect
these could be combined into a single `case _: UpStepExpression`.
Also, "Up2" in the error message is an internal identifier that probably
won't make sense to users. Maybe just call it an "up step".
Though, thinking about this more, I don't think I understand these Up2 and
SelfStep case. Say we have an expression like
`fn:count(../parent::foo/bar/baz)`. I believe this function will throw an SDE
here because of the `parent:foo` up-step? But this path is perfectly fine.
Based on the error message, I think this case trying to check against
something like `fn:count(../..[2]/some/other/element)', where the `..[2]` isn't
legal? In fact, a related issue is mentioned in a comment in
`SelfStepExpression` and references DAFFODIL-2182. So we probably aren't
dealing with predicates with up- and self-steps correctly, but I'm not sure
this is the right function, or PR, to fix it. The goal of this function just
wants to make sure any steps to arrays have a predicate, which I think the last
case correctly checks.
Up- and self-steps can technically go to arrays (i.e. isArray can be true
for them) but they always resolve to a single instance in the array, so they
don't actually need predicates, so instead of SDE'ing, I think they just want
to be `true`, like the Up-step case. So I think those other cases just want to
be something like:
```scala
// up and self expressions do not need a predicate because they always
reference a single element, even if that element is part of an array
case _: UpStepExpression | _: SelfStepExpression => true
// either not an array, or is an array with a predicate
case step => !step.isArray || step.pred.isDefined
```
Though, we already have that check here:
https://github.com/apache/daffodil/blob/main/daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala#L1223
I wonder if the issue is that the `targetType` for the `fn:count` is
`NodeInfo.Exists`, which means that part of code that normally requires
predicates isn't doing it? I think that is a bug? Maybe that wants to be
changed to something like:
```scala
} else if (!isLast && targetType == NodeInfo.Exists) {
new DownArrayExists(nqn)
} else {
```
This way, even with fn:exists or fn:count, we still require a predicate for
all intermediate named steps. The only one that doesn't require a predicate is
the last step. This means you can no longer do something like this:
```
fn:exists(/path/arrayOne/foo/bar/arrayTwo)
```
Where arrayOne is an array without a predicate. You could do that in the
past, but that doesnt make any sense. Only the arrayTwo step can not have a
predicate. I'm not even sure what would currently happen in that case, since we
require a specific instance of arrayOne to look for the foo child.
Sorry for the long wall of text, but in sumary, I think we just need the
`!isLast` change, and then priorStepsHavingIndexingPredicates goes away, since
that is now enforced by the isLast change.
--
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]
