mbeckerle commented on code in PR #1137:
URL: https://github.com/apache/daffodil/pull/1137#discussion_r1446534505
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2369,6 +2381,7 @@ case class FNCountExpr(nameAsParsed: String, fnQName:
RefQName, args: List[Expre
override lazy val compiledDPath = {
checkArgCount(1)
+ checkArgArray()
Review Comment:
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.
##########
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
- }
- }
-
def arrayLength: Long =
- if (isAnArray()) currentArray.length
- else 1L
-
- def finalArrayLength: Long =
- if (isAnArray()) {
- currentArray.requireFinal
- currentArray.length
- } else 1L
+ currentArray.length
+
+ def finalArrayLength: Long = {
+ currentArray.requireFinal()
+ currentArray.length
Review Comment:
This craziness of "array" vs. "array or optional" has gone on too long. We
should adopt firm terminology and try to use it everywhere: array means 2+
occurrences are possible, optional means can have only 0 or 1 occurrence, for
the combined thing we should standardize all identifiers on arrayOpt, meaning
array or optional, or equivalently, non-scalar. The term scalar is already used
to mean minOccurs=1 maxOccurs=1 i.e., without dimensions.
##########
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:
I agree we can remove the warnID and the warning. This should just be an SDE
if violated. The argument for leaving in the warnID is to not break existing
config files that are suppressing this warning, however, those schemas would be
broken anyway by this, ... and that's assuming such schemas even exist, which I
doubt since they would almost certainly be incorrect/buggy anyway.
The DFDL spec says clearly fn:count must be called on array/optional only
and it is an SDE otherwise. Yet the description of fn:count doesn't say that,
it's elsewhere in discussion of the kinds of errors.
So I think there is some uncertainty about the DFDL spec.
--
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]
