tuxji commented on code in PR #1199:
URL: https://github.com/apache/daffodil/pull/1199#discussion_r1543907086
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -3041,6 +3047,101 @@ case class DFDLCheckConstraintsExpr(
}
+case class DFDLCheckRangeInclusiveExpr(
+ nameAsParsed: String,
+ fnQName: RefQName,
+ args: List[Expression],
+) extends FunctionCallBase(nameAsParsed, fnQName, args) {
+
+ lazy val List(arg1, arg2, arg3) = { checkArgCount(3); args }
+
+ override lazy val children = args
+
+ override lazy val compiledDPath = {
+ checkArgCount(3)
+ val argDPath = args(0).compiledDPath
+ val rangeFrom = args(1).compiledDPath
+ val rangeTo = args(2).compiledDPath
+ val c = conversions
+ val res = new CompiledDPath(DFDLCheckRangeInclusive(argDPath, rangeFrom,
rangeTo) +: c)
+ res
+ }
+ override def targetTypeForSubexpression(subexpr: Expression): NodeInfo.Kind
= {
+ val targetType = (arg1.inherentType, arg2.inherentType, arg3.inherentType)
match {
+ case (
+ testType: NodeInfo.Numeric.Kind,
+ minType: NodeInfo.Numeric.Kind,
+ maxType: NodeInfo.Numeric.Kind,
+ ) => {
+ val rangeType = NodeInfoUtils.generalizeArgTypesForComparisonOp(
+ "checkRangeInclusive",
+ minType,
+ maxType,
+ )
+ val targetType = NodeInfoUtils.generalizeArgTypesForComparisonOp(
+ "checkRangeInclusive",
+ testType,
+ rangeType,
+ )
+ targetType
+ }
+ case (test, min, max) =>
+ SDE(s"Cannot call $nameAsParsed with non-numeric types: $test, $min,
$max")
+ }
+ targetType
+ }
+
+ override lazy val inherentType: NodeInfo.Kind = NodeInfo.Boolean
+
+}
+
+case class DFDLCheckRangeExclusiveExpr(
+ nameAsParsed: String,
+ fnQName: RefQName,
+ args: List[Expression],
+) extends FunctionCallBase(nameAsParsed, fnQName, args) {
+
+ lazy val List(arg1, arg2, arg3) = { checkArgCount(3); args }
+
+ override lazy val children = args
+
+ override lazy val compiledDPath = {
+ checkArgCount(3)
+ val argDPath = args(0).compiledDPath
+ val rangeFrom = args(1).compiledDPath
+ val rangeTo = args(2).compiledDPath
Review Comment:
Likewise, can use arg1, arg2, arg3 here.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/DFDLFunctions.scala:
##########
@@ -39,6 +42,94 @@ case class DFDLCheckConstraints(recipe: CompiledDPath)
extends RecipeOpWithSubRe
}
}
+case class DFDLCheckRangeInclusive(
+ dataRecipe: CompiledDPath,
+ minRecipe: CompiledDPath,
+ maxRecipe: CompiledDPath,
+) extends RecipeOpWithSubRecipes(dataRecipe, minRecipe, maxRecipe) {
+ override def run(dstate: DState): Unit = {
+ 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: DataValue.DataValuePrimitiveNullable,
+ minVal: DataValue.DataValuePrimitiveNullable,
+ maxVal: DataValue.DataValuePrimitiveNullable,
Review Comment:
I'm confused by the syntax here. If this is a function call, which it
appears to be, why do the arguments look like parameter definitions? That is,
why not just call executeCheck(dataVal, minVal, maxVal)?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -3041,6 +3047,101 @@ case class DFDLCheckConstraintsExpr(
}
+case class DFDLCheckRangeInclusiveExpr(
+ nameAsParsed: String,
+ fnQName: RefQName,
+ args: List[Expression],
+) extends FunctionCallBase(nameAsParsed, fnQName, args) {
+
+ lazy val List(arg1, arg2, arg3) = { checkArgCount(3); args }
+
+ override lazy val children = args
+
+ override lazy val compiledDPath = {
+ checkArgCount(3)
+ val argDPath = args(0).compiledDPath
+ val rangeFrom = args(1).compiledDPath
+ val rangeTo = args(2).compiledDPath
Review Comment:
You can use arg1, arg2, and arg3 which are evaluated only once (their
evaluation calls checkArgCount(3) too) instead of repeating work. That is,
let's remove the checkArgCount(3) call and change the next 3 lines to:
```scala
val argDPath = arg1.compiledDPath
val rangeFrom = arg2.compiledDPath
val rangeTo = arg3.compiledDPath
```
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/DFDLFunctions.scala:
##########
@@ -39,6 +42,94 @@ case class DFDLCheckConstraints(recipe: CompiledDPath)
extends RecipeOpWithSubRe
}
}
+case class DFDLCheckRangeInclusive(
+ dataRecipe: CompiledDPath,
+ minRecipe: CompiledDPath,
+ maxRecipe: CompiledDPath,
+) extends RecipeOpWithSubRecipes(dataRecipe, minRecipe, maxRecipe) {
+ override def run(dstate: DState): Unit = {
+ 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: DataValue.DataValuePrimitiveNullable,
+ minVal: DataValue.DataValuePrimitiveNullable,
+ maxVal: DataValue.DataValuePrimitiveNullable,
+ )
+ dstate.setCurrentValue(res)
+ }
+
+ def executeCheck(
+ dataVal: DataValue.DataValuePrimitiveNullable,
+ minVal: DataValue.DataValuePrimitiveNullable,
+ maxVal: DataValue.DataValuePrimitiveNullable,
+ ): Boolean = {
+ (dataVal.value, minVal.value, maxVal.value) match {
+ case (data: Integer, min: Integer, max: Integer) => data >= min && data
<= max
+ case (data: java.lang.Double, min: java.lang.Double, max:
java.lang.Double) =>
+ data >= min && data <= max
+ case (data: java.lang.Float, min: java.lang.Float, max: java.lang.Float)
=>
+ data >= min && data <= max
+ case (data: java.math.BigDecimal, min: java.math.BigDecimal, max:
java.math.BigDecimal) =>
+ data.compareTo(min) >= 0 && data.compareTo(max) <= 0
+ case (data: BigInteger, min: BigInteger, max: BigInteger) =>
+ data.compareTo(min) >= 0 && data.compareTo(max) <= 0
+ case (_, _, _) => false
+ }
+ }
+}
+
+case class DFDLCheckRangeExclusive(
+ dataRecipe: CompiledDPath,
+ minRecipe: CompiledDPath,
+ maxRecipe: CompiledDPath,
+) extends RecipeOpWithSubRecipes(dataRecipe, minRecipe, maxRecipe) {
+ override def run(dstate: DState): Unit = {
+ 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: DataValue.DataValuePrimitiveNullable,
+ minVal: DataValue.DataValuePrimitiveNullable,
+ maxVal: DataValue.DataValuePrimitiveNullable,
+ )
+ dstate.setCurrentValue(res)
+ }
+
+ def executeCheck(
+ dataVal: DataValue.DataValuePrimitiveNullable,
+ minVal: DataValue.DataValuePrimitiveNullable,
+ maxVal: DataValue.DataValuePrimitiveNullable,
+ ): Boolean = {
+ (dataVal.value, minVal.value, maxVal.value) match {
+ case (data: Integer, min: Integer, max: Integer) => data >= min && data
< max
+ case (data: java.lang.Double, min: java.lang.Double, max:
java.lang.Double) =>
+ data >= min && data < max
Review Comment:
This range check looks like it's still doing an inclusive comparison on the
min side and doing an exclusive comparison only on the max side. Is that how
the DFDL function "checkRangeExclusive" is supposed to work?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/DFDLFunctions.scala:
##########
@@ -39,6 +42,94 @@ case class DFDLCheckConstraints(recipe: CompiledDPath)
extends RecipeOpWithSubRe
}
}
+case class DFDLCheckRangeInclusive(
+ dataRecipe: CompiledDPath,
+ minRecipe: CompiledDPath,
+ maxRecipe: CompiledDPath,
+) extends RecipeOpWithSubRecipes(dataRecipe, minRecipe, maxRecipe) {
+ override def run(dstate: DState): Unit = {
+ 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: DataValue.DataValuePrimitiveNullable,
+ minVal: DataValue.DataValuePrimitiveNullable,
+ maxVal: DataValue.DataValuePrimitiveNullable,
+ )
+ dstate.setCurrentValue(res)
+ }
+
+ def executeCheck(
+ dataVal: DataValue.DataValuePrimitiveNullable,
+ minVal: DataValue.DataValuePrimitiveNullable,
+ maxVal: DataValue.DataValuePrimitiveNullable,
+ ): Boolean = {
+ (dataVal.value, minVal.value, maxVal.value) match {
+ case (data: Integer, min: Integer, max: Integer) => data >= min && data
<= max
+ case (data: java.lang.Double, min: java.lang.Double, max:
java.lang.Double) =>
+ data >= min && data <= max
+ case (data: java.lang.Float, min: java.lang.Float, max: java.lang.Float)
=>
+ data >= min && data <= max
+ case (data: java.math.BigDecimal, min: java.math.BigDecimal, max:
java.math.BigDecimal) =>
+ data.compareTo(min) >= 0 && data.compareTo(max) <= 0
+ case (data: BigInteger, min: BigInteger, max: BigInteger) =>
+ data.compareTo(min) >= 0 && data.compareTo(max) <= 0
+ case (_, _, _) => false
+ }
+ }
+}
+
+case class DFDLCheckRangeExclusive(
+ dataRecipe: CompiledDPath,
+ minRecipe: CompiledDPath,
+ maxRecipe: CompiledDPath,
+) extends RecipeOpWithSubRecipes(dataRecipe, minRecipe, maxRecipe) {
+ override def run(dstate: DState): Unit = {
+ 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
Review Comment:
I'm not familiar with evalution of expressions. Why does this code call
dstate.setCurrentNode(saved) after both dataRecipe.run(state) and
minRecipe.run(dstate), but not after maxRecipe.run(dstate)? Why doesn't dstate
need to have its saved state restored after the last maxRecipe,run(dstate)?
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml:
##########
@@ -7684,4 +7684,217 @@ blastoff
<tdml:infoset><tdml:dfdlInfoset><add01>4</add01></tdml:dfdlInfoset></tdml:infoset>
</tdml:parserTestCase>
+ <tdml:defineSchema name="DFDLCheckRange">
+ <xs:include
schemaLocation="/org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat" />
+ <xs:element name="literalMinMax_int">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:element name="data" dfdl:lengthKind="explicit" dfdl:length="1"
type="xs:integer"/>
+ <xs:element name="isInRangeInc" type="xs:boolean"
+ dfdl:inputValueCalc="{
dfdl:checkRangeInclusive(../ex:data, 0, 8) }"/>
+ <xs:element name="isInRangeExc" type="xs:boolean"
+ dfdl:inputValueCalc="{
dfdl:checkRangeExclusive(../ex:data, 0, 8) }"/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="expMinMax_double">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:sequence dfdl:separatorPosition="infix" dfdl:separator="|">
+ <xs:element name="data" dfdl:lengthKind="delimited"
type="xs:double"/>
+ <xs:element name="min" dfdl:lengthKind="delimited"
type="xs:double"/>
+ <xs:element name="max" dfdl:lengthKind="delimited"
type="xs:double"/>
+ </xs:sequence>
+ <xs:element name="isInRangeInc" type="xs:boolean"
+ dfdl:inputValueCalc="{
dfdl:checkRangeInclusive(../ex:data, ../ex:min, ../ex:max) }"/>
+ <xs:element name="isInRangeExc" type="xs:boolean"
+ dfdl:inputValueCalc="{
dfdl:checkRangeExclusive(../ex:data, ../ex:min, ../ex:max) }"/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="expMinMax_float">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:sequence dfdl:separatorPosition="infix" dfdl:separator="|">
+ <xs:element name="data" dfdl:lengthKind="delimited"
type="xs:float"/>
+ <xs:element name="min" dfdl:lengthKind="delimited"
type="xs:float"/>
+ <xs:element name="max" dfdl:lengthKind="delimited"
type="xs:float"/>
+ </xs:sequence>
+ <xs:element name="isInRangeInc" type="xs:boolean"
+ dfdl:inputValueCalc="{
dfdl:checkRangeInclusive(../ex:data, ../ex:min, ../ex:max) }"/>
+ <xs:element name="isInRangeExc" type="xs:boolean"
+ dfdl:inputValueCalc="{
dfdl:checkRangeExclusive(../ex:data, ../ex:min, ../ex:max) }"/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="expMinMax_decimal">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:sequence dfdl:separatorPosition="infix" dfdl:separator="|">
+ <xs:element name="data" dfdl:lengthKind="delimited"
type="xs:decimal"/>
+ <xs:element name="min" dfdl:lengthKind="delimited"
type="xs:decimal"/>
+ <xs:element name="max" dfdl:lengthKind="delimited"
type="xs:decimal"/>
+ </xs:sequence>
+ <xs:element name="isInRangeInc" type="xs:boolean"
+ dfdl:inputValueCalc="{
dfdl:checkRangeInclusive(../ex:data, ../ex:min, ../ex:max) }"/>
+ <xs:element name="isInRangeExc" type="xs:boolean"
+ dfdl:inputValueCalc="{
dfdl:checkRangeExclusive(../ex:data, ../ex:min, ../ex:max) }"/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="expMinMax_mixedIntAndFloat">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:sequence dfdl:separatorPosition="infix" dfdl:separator="|">
+ <xs:element name="data" dfdl:lengthKind="delimited"
type="xs:integer"/>
+ <xs:element name="min" dfdl:lengthKind="delimited"
type="xs:float"/>
+ <xs:element name="max" dfdl:lengthKind="delimited"
type="xs:float"/>
+ </xs:sequence>
+ <xs:element name="isInRangeInc" type="xs:boolean"
+ dfdl:inputValueCalc="{
dfdl:checkRangeInclusive(../ex:data, ../ex:min, ../ex:max) }"/>
+ <xs:element name="isInRangeExc" type="xs:boolean"
+ dfdl:inputValueCalc="{
dfdl:checkRangeExclusive(../ex:data, ../ex:min, ../ex:max) }"/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="expMinMax_string">
+ <xs:complexType>
+ <xs:sequence>
+ <xs:sequence dfdl:separatorPosition="infix" dfdl:separator="|">
+ <xs:element name="data" dfdl:lengthKind="delimited"
type="xs:string"/>
+ <xs:element name="min" dfdl:lengthKind="delimited"
type="xs:string"/>
+ <xs:element name="max" dfdl:lengthKind="delimited"
type="xs:string"/>
+ </xs:sequence>
+ <xs:element name="isInRangeInc" type="xs:boolean"
+ dfdl:inputValueCalc="{
dfdl:checkRangeInclusive(../ex:data, ../ex:min, ../ex:max) }"/>
+ <xs:element name="isInRangeExc" type="xs:boolean"
+ dfdl:inputValueCalc="{
dfdl:checkRangeExclusive(../ex:data, ../ex:min, ../ex:max) }"/>
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
+
+ </tdml:defineSchema>
+
+ <tdml:parserTestCase name="DFDLCheckRange_01" validation="on"
+ root="literalMinMax_int" model="DFDLCheckRange">
+ <tdml:document>8</tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <literalMinMax_int>
+ <data>8</data>
+ <isInRangeInc>true</isInRangeInc>
+ <isInRangeExc>false</isInRangeExc>
+ </literalMinMax_int>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
+ <tdml:parserTestCase name="DFDLCheckRange_02" validation="on"
+ root="expMinMax_double" model="DFDLCheckRange">
+ <tdml:document>2.3|2.3|2.5</tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <expMinMax_double>
+ <data>2.3</data>
+ <min>2.3</min>
+ <max>2.5</max>
+ <isInRangeInc>true</isInRangeInc>
+ <isInRangeExc>true</isInRangeExc>
+ </expMinMax_double>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
+ <tdml:parserTestCase name="DFDLCheckRange_03" validation="on"
+ root="expMinMax_float" model="DFDLCheckRange">
+ <tdml:document>2.5|2.3|2.5</tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <expMinMax_float>
+ <data>2.5</data>
+ <min>2.3</min>
+ <max>2.5</max>
+ <isInRangeInc>true</isInRangeInc>
+ <isInRangeExc>false</isInRangeExc>
+ </expMinMax_float>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
+ <tdml:parserTestCase name="DFDLCheckRange_04" validation="on"
+ root="expMinMax_decimal" model="DFDLCheckRange">
+ <tdml:document>2.5|2.3|2.5</tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <expMinMax_decimal>
+ <data>2.5</data>
+ <min>2.3</min>
+ <max>2.5</max>
+ <isInRangeInc>true</isInRangeInc>
+ <isInRangeExc>false</isInRangeExc>
+ </expMinMax_decimal>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
+ <tdml:parserTestCase name="DFDLCheckRange_05" validation="on"
+ root="expMinMax_mixedIntAndFloat"
model="DFDLCheckRange">
+ <tdml:document>1|1.9|2.5</tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <expMinMax_mixedIntAndFloat>
+ <data>1</data>
+ <min>1.9</min>
+ <max>2.5</max>
+ <isInRangeInc>false</isInRangeInc>
+ <isInRangeExc>false</isInRangeExc>
+ </expMinMax_mixedIntAndFloat>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
+ <tdml:parserTestCase name="DFDLCheckRange_06" validation="on"
+ root="literalMinMax_int" model="DFDLCheckRange">
+ <tdml:document>9</tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <literalMinMax_int>
+ <data>9</data>
+ <isInRangeInc>false</isInRangeInc>
+ <isInRangeExc>false</isInRangeExc>
+ </literalMinMax_int>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
+ <tdml:parserTestCase name="DFDLCheckRange_07" validation="on"
+ root="expMinMax_string" model="DFDLCheckRange">
+ <tdml:document>2|1.9|2.5</tdml:document>
+
+ <tdml:errors>
+ <tdml:error>Schema Definition Error</tdml:error>
+ <tdml:error>Cannot call dfdl:checkRangeInclusive</tdml:error>
+ <tdml:error>with non-numeric types</tdml:error>
+ <tdml:error>string, string, string</tdml:error>
+ </tdml:errors>
+<!-- <tdml:infoset>-->
+<!-- <tdml:dfdlInfoset>-->
+<!-- <expMinMax_mixedIntAndFloat>-->
+<!-- <data>1</data>-->
+<!-- <min>1.9</min>-->
+<!-- <max>2.5</max>-->
+<!-- <isInRangeInc>false</isInRangeInc>-->
+<!-- <isInRangeExc>false</isInRangeExc>-->
+<!-- </expMinMax_mixedIntAndFloat>-->
+<!-- </tdml:dfdlInfoset>-->
+<!-- </tdml:infoset>-->
Review Comment:
Commented out code shouldn't be left around for a long time.
--
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]
