fn:count(non-array) must issue SDE

[pkatlic]

Throw a SDE during compilation if the fn:count function is called on a path
expression that ends with a unique scalar element.

DAFFODIL-2711

Expression.scala: Remove unused isTypeCorrect and checkTypeCorrectness properties. Add functions to check for Array or Optional elements and combine common code, update naming to include OrOptional. Rename isOptional to isArrayOrOptional. Check for isLastStep in downwardStep. Add checkArgArrayOrOptional function to check for an array or optional element and call for FNCount and FNExactlyOneExpr expressions.

ElementBaseRuntime1Mixin.scala: Add isOptional property.

dafext.xsd: Remove unused enumeration.

ArrayRelated.scala: Modify FNCount run implementation to handle Optional elements.

DState.scala: Remove isAnArray function since it is now redundant.

FNFunctions.scala: Fix typo.

CompiledExpression1.scala: Add isOptional property.

RuntimeData.scala: Add isOptional to constructor.

testWarnings.tdml: Update error message check.

TestTDMLRunnerWarnings.scala: Update error message check.

PrefixedTests.tdml: Update element to use fn:exists instead of fn:count.

expressions.tdml: Update element to use fn:exists instead of fn:count.

Functions.tdml: Add tests for fn:count, update error message checks and comments. Note that using a group reference for an optional when combined with an array will throw a ClassCastException, this may be a bug where an optional is evaluated as a DownArray instead of a DownElement. The issue DAFFODIL-2879 contains more details.

TestDFDLExpressions.scala: Add new test cases for Functions.tdml.

[tuxji]

Oops, the checkArgArray function in Expression.scala needs to return true if the last step ends in an optional element as well as an array. Once that's fixed, and if no one else sees a problem, we can approve and merge the PR.

[stevedlawrence]

Nice change, it'll nice to have this all be static checking and no runing timechecking. A couple minor comments/questions. Also, before we merge this we'll need to rebase to remove the merge commit.

[stevedlawrence]

This checkArgArray function is only used for fn:count, which only has one argument so it's fine to just check expressions.head, but without knowing that it could be confusing why this only checks head and not all of the function arguments. I wonder if this could be made more generic somehow? Maybe it takes an index into the expressions list and checks that that specific expression is an array? And then the usage in FnCountExpr would be checkArgArray(0)? Or maybe it takes an actual expression, so the usage would be checkArgArray(args(0))? I'm not sure if a similar check we do somewhere else that we could copy the pattern from?

[mbeckerle]

Hmmm. I'm a bit uncertain if this is the right approach. It has been a while since I wrote this stuff.

The expressions have a target type which can either be the type of the element they are populating or the type required by expression context (e.g., the predicate of an IF has a target type of boolean, the args to + have number target types, etc.) fn:count should have an array/optional target type. This creates a requirement that the expression which is the argument must satisfy either directly, or by propagation of the requirement, or by automatic insertion of a conversion.

Above at line 2365, there is an arrPath.isPathToOneWholeArray. If that predicate is true then the expression is type correct. You want to be true for array/optional elements, not true otherwise. My quick dig into the code shows this is going to be false for optionals. Fixing that (and renaming to isPathToOneWholeArrayOrOptional) would be the right fix rather than writing a new mechanism.

I think.... it's been a while.

[tuxji]

Good comments by Steve, let's update the PR one more time.

[tuxji]

We are getting very close to approving this PR! Just one last thing to debug to make sure the logic is correct.

[tuxji]

Looking at this code, I'm confused why tests count_17 and count_18 work correctly. I saw the || and it looks to me like we want to replace || with && since we want both parts (thenPart & elsePart) to be paths to arrays or optional elements, not just one of them. However, these tests seem to show this logic works correctly (e.g., fail if thenPart and elsePart have different values for isPathToOneWholeArrayOrOptional, pass otherwise). Maybe Daffodil has type checking elsewhere which fails the test before this logic has a chance to run? Please set a breakpoint here and debug the test count_18 to see whether this logic is actually executed.

[stevedlawrence]

Agreed, I would expect an && here too.

Another thing maybe worth considering is that this doesn't allow nested ifs, since it only checks one level deep. So if the then or else branches contain another if-statement, even if those resolve to a path to a whole array/optional, it will fail.

Though, I'm not sure if that matters, and in fact, I'm wondering if we should just require that fn:count() accepts exactly a path--no conditionals, no other functions, etc. So we don't allow fn:count(if(cond) then /path1 else /path2) and that fails with an SDE like "fn:count argument must be a path". If you want conditional counts then you do it like this: if (cond) then fn:count(/path1) else fn:count(/path2). It's a bit more verbose, but is more clear and avoids potential edge cases.

[tuxji]

I like the simplicity of allowing only a path as fn:count's argument. I checked the DFDL specification and it was not very clear whether we must allow any DFDL expression to be given to fn:count as argument:

Function	Meaning
fn:count($arg)	Returns the number of items in the value of $arg as an xs:integer. Returns 0 if $arg is the empty sequence.

The following are Schema Definition Errors, regardless of whether they are detected in advance of processing or once processing begins:

• Expression value is not single node
  - Most DFDL expression contexts require an expression to identify a single node, not an array (aka sequence of nodes). There are a few exceptions such as the fn:count(…) function, where the path expression must be to an array or optional element.
• Expression value is not array element or optional element.
  - Some DFDL expression contexts require an array or an optional element.
  - Example: The fn:count(...) function argument must be to an array or optional element. It is a Schema Definition Error if the argument expression is otherwise.

Mike, please advise us with your deeper knowledge of DPath and XPath.

[mbeckerle]

Ok. I think the DFDL spec is not clear on whether fn:count must take a path (not a DPath expression, a literally step/step/step thingy) or if it can be if/then/else so long as the then/else are both arrays or both optionals.

I think this code doesn't work right for nested ifs. To get that to work this would have to compose like the way the type system composes. I.e., the

tp.isPathToOneWholeArrayOrOptional || ep.isPathToOneWholeArrayOrOptional

(which should be && not ||) that would need to inductively be computed for the then-part and else part.
Putting in that infrastructure, and then testing it sufficiently feels very not worth it.

I think I'd rather keep it simpler and just say fn:count's argument must literally be a path (i.e., relative or absolute path steps). It's a fair interpretation of the DFDL spec to put this restriction in.

[stevedlawrence]

It might be cleaner to write this as a match/case, we can also combine Up/Up2 with the parent UpStepExpression and similarly for Self/Self2. Maybe something like this:

step match {
  case _: UpStepExpression => SDE("up step must not have indexing")
  case _: SelfStepExpression => SDE("self step must not have indexing")
  case _ => !step.isArray || step.pred.isDefined
}

[pkatlic]

When referencing a group here a ClassCastException is thrown, which may be a bug.

class org.apache.daffodil.runtime1.infoset.DISimple cannot be cast to class org.apache.daffodil.runtime1.infoset.DIArray

[stevedlawrence]

Does this ClassCastException still occur, or did newer changes fix this?

[pkatlic]

This exception still occurs for this case with an array and optional, as it seems that the optional is evaluated as a DownArray rather than a DownElement.

[tuxji]

+1

If the tests are checking the right outcomes, then the code is working correctly. Good job!

[tuxji]

Ask your IDEA if there are any uses ofthisisArrayOrOptional` function. Code coverage says this function is not covered by tests, which implies that this function is never used anywhere else (if so, we should remove it).

[pkatlic]

This was added as a potentially-useful helper in the future, but can be removed as it is currently not used.

[stevedlawrence]

I'm trying to understand our existing code more, and if feels like we already have parts of the pieces implemented such that some of the new code isn't actually need, we just have bugs where the existing logic isn't getting hit? I wonder if we just need to fix those bugs? I possibly identified two.

[stevedlawrence]

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.

[stevedlawrence]

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?

[stevedlawrence]

I took another scan at this, and it looks like isTypeCorrect (which already calls isPathToOneWholeArray) is never actually used. I suggest we just delete this val and the couple of references to it, and then move your checkArgArrayOrOptional function to FunctionCallArrayBase. It can then use arrPath (instead of expression.head) to verify that is goes to a whole array. And then update implementations of FunctionCallArrayBase to call this function, since they should all go to a whole array.

[stevedlawrence]

Looks good, one question abou the class cast exception and minor cleanup

[stevedlawrence]

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.

[pkatlic]

This priorStepsHaveIndexingPredicates expression is required in order to properly process array elements. With this removed, we saw several tests fail.

[stevedlawrence]

Did you add steps.last.pred.isEmpty to isPathToOneWholeArrayOrOptional and isPathToOneWholeArray?

[pkatlic]

Good catch! After this update, the priorStepsHaveIndexingPredicates expression is no longer needed.

[stevedlawrence]

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.

[pkatlic]

Yes, moved this to PathExpression.

[stevedlawrence]

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.

[pkatlic]

This call to checkArgArrayOrOptional(0) now made by FNExactlyOneExpr.

[stevedlawrence]

Does this ClassCastException still occur, or did newer changes fix this?

[stevedlawrence]

This definitely sounds like a bug. Can you open a ticket so we remember to look into it? If possible, include steps to reproduce (e.g. uncomment this group reference, run XYZ test) and the exception?

[tuxji]

This PR looks ready to be approved once the new bug is created in the JIRA database.

[stevedlawrence]

+1, agreed, looks good to me. Just needs to be squashed to a single commit

[tuxji]

Peter has created DAFFODIL-2879 as requested. PR is approved and ready for merge, will merge it.