stevedlawrence commented on code in PR #1137:
URL: https://github.com/apache/daffodil/pull/1137#discussion_r1446439958
##########
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
Review Comment:
Can these two be combined into this?
```scala
case pe: PathExpression => pe.steps.last.isArray
```
I think these are the only classes that extend that base class?
Also, is `isArray` sufficient? I know optional elements (i.e. minOccurs=0
maxOccurs=1) are represented in the infoset as an array, but sometimes aren't
considered as actual arrays in other parts of the code. I'm not sure if the
StepExpression isArray handles that? I think we want `fn:count` to still work
on optional elements?
##########
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:
One thing to consider is this doesn't catch expressions like this:
```xquery
fn:count(if (some_condition) then /some/path else /some/other/path)
```
I think that's legal and I think would be considered invalid because of this
case.
I'm not sure if there's a good way to check that all branches of a
subexpression result in an array. We must be able to get some basic information
since we can statically check that all branches have the same type. I think we
do that via `inherentType`, but I'm not sure the result would return
information about being an array or not. Maybe this is just a limitation of our
checking and it's good enough? And if someone whats the above, they would need
to do something like this:
```xquery
if (some_condition) then fn:count(/some/path) else fn:count(/some/other/path)
```
##########
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 {
Review Comment:
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?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/DState.scala:
##########
@@ -215,45 +218,26 @@ case class DState(
def booleanValue: Boolean = currentValue.getBoolean
def longValue: Long = asLong(currentValue.getAnyRef)
+
def intValue: Int = longValue.toInt
+
def doubleValue: Double = asDouble(currentValue.getAnyRef)
def integerValue: JBigInt = asBigInt(currentValue.getAnyRef)
+
def decimalValue: JBigDecimal = asBigDecimal(currentValue.getAnyRef)
+
def stringValue: String = currentValue.getString
def isNilled: Boolean = currentElement.isNilled
- private def isAnArray(): Boolean = {
- if (!currentNode.isInstanceOf[DIArray]) {
- Assert.invariant(errorOrWarn.isDefined)
- if (currentNode.isInstanceOf[DIElement]) {
- errorOrWarn.get.SDW(
- WarnID.PathNotToArray,
- "The specified path to element %s is not to an array. Suggest using
fn:exists instead.",
- currentElement.name,
- )
- } else {
- errorOrWarn.get.SDW(
- WarnID.PathNotToArray,
- "The specified path is not to an array. Suggest using fn:exists
instead.",
- )
- }
- false
- } else {
- true
- }
- }
-
Review Comment:
So this is an 100% static check now? Nice! It might be worth adding a
comment to the `arrayLength` and `finalArrayLength` functions that we must
statically know current dpath result is an array, otherwise these might return
some weird results or throw an exception. They are currently only used by
FNCount so it should be safe, but if another function ever uses those we need
to make sure they are doing the checkArgArray thing.
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_functions/Functions.tdml:
##########
@@ -11390,6 +11417,110 @@
</tdml:infoset>
</tdml:unparserTestCase>
+ <!--
+ Test Name: count_12
+ Schema: XPathFunctions
+ Root: count07
+ Purpose: This test demonstrates the count() function works as
expected
+ when parsing an array of scalars
+ -->
+
+ <tdml:parserTestCase name="count_12" root="count07"
+ model="XPathFunctions" description="Section 23 - Functions - fn:count -
DFDL-23-122R">
+ <tdml:document>
+ <tdml:documentPart type="byte">00 00 00 01 00 00 00
02</tdml:documentPart>
+ </tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:count07 xmlns:ex="http://example.com";>
+ <ex:int>1</ex:int>
+ <ex:int>2</ex:int>
+ <ex:count>2</ex:count>
+ </ex:count07>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
+ <!--
+ Test Name: count_13
+ Schema: XPathFunctions
+ Root: count08
+ Purpose: This test demonstrates the count() function works as
expected
+ when parsing a unique scalar element
+ -->
+
+ <tdml:parserTestCase name="count_13" root="count08" roundTrip="true"
+ model="XPathFunctions" description="Section 23 -
Functions - fn:count - DFDL-23-122R">
+ <tdml:document>
+ <tdml:documentPart type="byte">00 00 00 01</tdml:documentPart>
+ </tdml:document>
+ <tdml:errors>
+ <tdml:error>Schema Definition Error</tdml:error>
+ <tdml:error>fn:count</tdml:error>
+ <tdml:error>array</tdml:error>
+ </tdml:errors>
+ </tdml:parserTestCase>
+
+ <!--
+ Test Name: count_14
+ Schema: XPathFunctions
+ Root: count09
+ Purpose: This test demonstrates the count() function works as
expected
+ when parsing an optional element
+ -->
+
+ <tdml:parserTestCase name="count_14" root="count09" roundTrip="true"
+ model="XPathFunctions" description="Section 23 -
Functions - fn:count - DFDL-23-122R">
+ <tdml:document>
+ <tdml:documentPart type="byte">00 00 00 01</tdml:documentPart>
+ </tdml:document>
+ <tdml:errors>
+ <tdml:error>Schema Definition Error</tdml:error>
+ <tdml:error>fn:count</tdml:error>
+ <tdml:error>array</tdml:error>
+ </tdml:errors>
Review Comment:
Agreed, I think that is the correct behavior. Only minOccurs=maxOccurs=1
should trigger the SDE.
--
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]
