stevedlawrence commented on a change in pull request #425:
URL: https://github.com/apache/incubator-daffodil/pull/425#discussion_r498191763
##########
File path:
daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
##########
@@ -2419,6 +2422,25 @@ case class DFDLCheckConstraintsExpr(nameAsParsed:
String, fnQName: RefQName,
}
+case class DFDLCheckRangeInclusiveExpr(nameAsParsed: String, fnQName:
RefQName, args: List[Expression])
+ extends FunctionCallBase(nameAsParsed, fnQName, args) {
+
+ override lazy val children = args
+
+ override lazy val compiledDPath = {
+ checkArgCount(3)
+ val compiledDPath = args(0).compiledDPath
+ val rangeFrom: Int = args(1).text.toInt
+ val rangeTo: Int = args(2).text.toInt
+ val res = new CompiledDPath(DFDLCheckRangeInclusive(compiledDPath,
rangeFrom, rangeTo))
+ res
+ }
+ override def targetTypeForSubexpression(subexpr: Expression): NodeInfo.Kind
= NodeInfo.AnyAtomic
Review comment:
I think this is where you need to handle type information about the
subexpressions. The targetType for the first paramater should probably be
Numeric. The targetType for the min/max parameters should be the inherentType
of the first parameter.
With this type information, Daffodil will insert the appropriate conversions
(or error if not allowed) to convert the min/max parameters to the type of the
node. This does mean the ``DFDLCheckRangeInclusive`` class will need to have
logic to evaluate those subexpressions and handle different numeric types.
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DFDLFunctions.scala
##########
@@ -39,6 +41,31 @@ case class DFDLCheckConstraints(recipe: CompiledDPath)
extends RecipeOpWithSubRe
}
}
+case class DFDLCheckRangeInclusive(recipe: CompiledDPath, rangeFrom: Int,
rangeTo: Int)
+ extends RecipeOpWithSubRecipes(recipe) {
+
+ override def run(dstate: DState): Unit = {
+ recipe.run(dstate)
+ if (dstate.currentElement.valid.isDefined) {
+ dstate.setCurrentValue(dstate.currentElement.valid.get)
+ } else {
+ val res = executeCheck(dstate, rangeFrom, rangeTo).isOK
+ dstate.currentElement.setValid(true)
+ dstate.setCurrentValue(res)
+ }
+ }
+
+ def executeCheck(dstate: DState, rangeFrom: Integer, rangeTo: Integer):
OKOrError = {
+ val testValue: BigInt = dstate.currentValue.getBigInt
+ if (testValue.intValue() >= rangeFrom.intValue() && testValue.intValue()
<= rangeTo.intValue()) {
+ OKOrError.OK
+ } else {
+ OKOrError.Error("Error...")
+ }
Review comment:
This DFDLCheckRange functions don't need to return any error
information. Either a value is within range or it's not. It isn't considered an
error to not be in range. So this should just return a boolean and
setCurrentValue just sets the returned value.
I would also suggest we make some effort to avoid BigInts. Firstly because
it's going to be somewhat slow, but secondgly because the currentValue can be
one of many different types, including those that aren't integers. This
function should be able to statically determine the type and perform an
appropriate range change based on that type.
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DFDLFunctions.scala
##########
@@ -39,6 +41,31 @@ case class DFDLCheckConstraints(recipe: CompiledDPath)
extends RecipeOpWithSubRe
}
}
+case class DFDLCheckRangeInclusive(recipe: CompiledDPath, rangeFrom: Int,
rangeTo: Int)
+ extends RecipeOpWithSubRecipes(recipe) {
+
+ override def run(dstate: DState): Unit = {
+ recipe.run(dstate)
+ if (dstate.currentElement.valid.isDefined) {
+ dstate.setCurrentValue(dstate.currentElement.valid.get)
+ } else {
Review comment:
You shouldn't need to check or set the value of ``valid`` here. That
variable just says whether the the value of ``currentElement`` is valid with
respect to the restrictions defined in the schema, which is unrelated to
checking the if the value of node is between some range.
Ultimately, this function probably wants to look something like the below so
that we run a recipe for both the data value and min/max value. Note that we
need to save the currentNode when running multiple expressions:
```scala
val saved = dstate.currentNode
dataRecipe.run(dstate)
val dataVal = dstate.currentValue
dstate.setCurrentNode(saved)
minRecipe.run(dstate)
val minVal = dstate.currentValue
dstate.setCurrentNode(saved)
maxRecipe.run(dstate)
val maxVal = dstate.currentValue
val res = executeCheck(dataVal, minVal, maxVal)
dstate.setCurrentValue(res)
```
##########
File path:
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml
##########
@@ -27,7 +27,48 @@
xmlns:daf="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:ext"
xmlns:fn="http://www.w3.org/2005/xpath-functions";
defaultRoundTrip="true">
-
+
+ <tdml:defineSchema name="DFDLCheckRange">
+ <xs:include
schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat" />
+ <xs:element name="root1" dfdl:lengthKind="explicit" dfdl:length="1"
type="xs:integer">
+ <xs:annotation>
+ <xs:appinfo source="http://www.ogf.org/dfdl/";>
+ <dfdl:discriminator test="{ dfdl:checkRangeInclusive(., 0, 1) }"
message="Discriminator failed ..." />
Review comment:
Although discrminators works, it adds extra complexity to the test that
can sometimes make it difficult to ensure if the function is behaving
correctly. I would suggest just using inputValueCalc, e.g.:
```xml
<xs:element name="isInRange" ... dfdl:inputValueCalc="{
dfdl:checkRangeInclusive(...) }" />
```
This way you can get actual true/false values in the expected infoset,
rather than assuming a discriminator passing/failing implies the function
worked as expected.
##########
File path:
daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
##########
@@ -2419,6 +2422,25 @@ case class DFDLCheckConstraintsExpr(nameAsParsed:
String, fnQName: RefQName,
}
+case class DFDLCheckRangeInclusiveExpr(nameAsParsed: String, fnQName:
RefQName, args: List[Expression])
+ extends FunctionCallBase(nameAsParsed, fnQName, args) {
+
+ override lazy val children = args
+
+ override lazy val compiledDPath = {
+ checkArgCount(3)
+ val compiledDPath = args(0).compiledDPath
+ val rangeFrom: Int = args(1).text.toInt
+ val rangeTo: Int = args(2).text.toInt
+ val res = new CompiledDPath(DFDLCheckRangeInclusive(compiledDPath,
rangeFrom, rangeTo))
Review comment:
The redmine doc says:
> The type of $val1 and $val2 must be compatible with the type of $node, and
must be a derivative of xs:decimal, xs:float or xs:double. It is a schema
definition error if the $node argument is a complex element.
Your code also assumes arg1 and arg2 are integers, but the types of min/max
depend on the type of node, which can be many different numeric types. You also
treat the min/max values as constant, but I don't think anything prevents them
from being expressions, for example, this should be allowed:
```xml
dfdl:checkRangeInclusive(../value, ../min, ../max)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
