You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "olabusayoT (via GitHub)" <gi...@apache.org> on 2024/03/28 20:10:06 UTC
[PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
olabusayoT opened a new pull request, #1199:
URL: https://github.com/apache/daffodil/pull/1199
- add implementation for checkRangeInclusive/Exclusive in Expressions and DFDLFunctions
- add tests for checkRangeInclusive/Exclusive for numeric types and non-numeric types
DAFFODIL-1515
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1199:
URL: https://github.com/apache/daffodil/pull/1199#discussion_r1545876124
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -258,116 +258,21 @@ case class ComparisonExpression(op: String, adds: List[Expression])
with BooleanExpression {
lazy val compareOp: CompareOpBase = {
- import NodeInfo.PrimType._
- import NodeInfo.ArrayIndex
- (op, convergedArgType) match {
- case ("<", _) => subsetError("Unsupported operation '%s'. Use 'lt' instead.", op)
- case (">", _) => subsetError("Unsupported operation '%s'. Use 'gt' instead.", op)
- case ("<=", _) => subsetError("Unsupported operation '%s'. Use 'le' instead.", op)
- case (">=", _) => subsetError("Unsupported operation '%s'. Use 'ge' instead.", op)
- case ("=", _) => subsetError("Unsupported operation '%s'. Use 'eq' instead.", op)
- case ("!=", _) => subsetError("Unsupported operation '%s'. Use 'ne' instead.", op)
-
- case ("eq", HexBinary) => EQ_CompareByteArray
- case ("ne", HexBinary) => NE_CompareByteArray
- case ("eq", _) => EQ_Compare
- case ("ne", _) => NE_Compare
-
- case ("lt", Boolean) => LT_Boolean
- case ("gt", Boolean) => GT_Boolean
- case ("le", Boolean) => LE_Boolean
- case ("ge", Boolean) => GE_Boolean
-
- case ("lt", Date) => LT_Date
- case ("gt", Date) => GT_Date
- case ("le", Date) => LE_Date
- case ("ge", Date) => GE_Date
-
- case ("lt", Time) => LT_Time
- case ("gt", Time) => GT_Time
- case ("le", Time) => LE_Time
- case ("ge", Time) => GE_Time
-
- case ("lt", DateTime) => LT_DateTime
- case ("gt", DateTime) => GT_DateTime
- case ("le", DateTime) => LE_DateTime
- case ("ge", DateTime) => GE_DateTime
-
- case ("lt", String) => LT_String
- case ("gt", String) => GT_String
- case ("le", String) => LE_String
- case ("ge", String) => GE_String
-
- case ("lt", Decimal) => LT_Decimal
- case ("gt", Decimal) => GT_Decimal
- case ("le", Decimal) => LE_Decimal
- case ("ge", Decimal) => GE_Decimal
-
- case ("lt", Integer) => LT_Integer
- case ("gt", Integer) => GT_Integer
- case ("le", Integer) => LE_Integer
- case ("ge", Integer) => GE_Integer
-
- case ("lt", NonNegativeInteger) => LT_NonNegativeInteger
- case ("gt", NonNegativeInteger) => GT_NonNegativeInteger
- case ("le", NonNegativeInteger) => LE_NonNegativeInteger
- case ("ge", NonNegativeInteger) => GE_NonNegativeInteger
-
- case ("lt", UnsignedLong) => LT_UnsignedLong
- case ("gt", UnsignedLong) => GT_UnsignedLong
- case ("le", UnsignedLong) => LE_UnsignedLong
- case ("ge", UnsignedLong) => GE_UnsignedLong
-
- case ("lt", Long) => LT_Long
- case ("gt", Long) => GT_Long
- case ("le", Long) => LE_Long
- case ("ge", Long) => GE_Long
-
- case ("lt", UnsignedInt) => LT_UnsignedInt
- case ("gt", UnsignedInt) => GT_UnsignedInt
- case ("le", UnsignedInt) => LE_UnsignedInt
- case ("ge", UnsignedInt) => GE_UnsignedInt
-
- case ("lt", ArrayIndex) => LT_UnsignedInt
- case ("gt", ArrayIndex) => GT_UnsignedInt
- case ("le", ArrayIndex) => LE_UnsignedInt
- case ("ge", ArrayIndex) => GE_UnsignedInt
-
- case ("lt", Int) => LT_Int
- case ("gt", Int) => GT_Int
- case ("le", Int) => LE_Int
- case ("ge", Int) => GE_Int
-
- case ("lt", UnsignedShort) => LT_UnsignedShort
- case ("gt", UnsignedShort) => GT_UnsignedShort
- case ("le", UnsignedShort) => LE_UnsignedShort
- case ("ge", UnsignedShort) => GE_UnsignedShort
-
- case ("lt", Short) => LT_Short
- case ("gt", Short) => GT_Short
- case ("le", Short) => LE_Short
- case ("ge", Short) => GE_Short
-
- case ("lt", UnsignedByte) => LT_UnsignedByte
- case ("gt", UnsignedByte) => GT_UnsignedByte
- case ("le", UnsignedByte) => LE_UnsignedByte
- case ("ge", UnsignedByte) => GE_UnsignedByte
-
- case ("lt", Byte) => LT_Byte
- case ("gt", Byte) => GT_Byte
- case ("le", Byte) => LE_Byte
- case ("ge", Byte) => GE_Byte
-
- case ("lt", Float) => LT_Float
- case ("gt", Float) => GT_Float
- case ("le", Float) => LE_Float
- case ("ge", Float) => GE_Float
-
- case ("lt", Double) => LT_Double
- case ("gt", Double) => GT_Double
- case ("le", Double) => LE_Double
- case ("ge", Double) => GE_Double
-
+ val (eq, ne, lt, le, gt, ge) = ComparisonOps.forType(convergedArgType)
+
+ op match {
+ case "<" => subsetError("Unsupported operation '%s'. Use 'lt' instead.", op)
+ case ">" => subsetError("Unsupported operation '%s'. Use 'gt' instead.", op)
+ case "<=" => subsetError("Unsupported operation '%s'. Use 'le' instead.", op)
+ case ">=" => subsetError("Unsupported operation '%s'. Use 'ge' instead.", op)
+ case "=" => subsetError("Unsupported operation '%s'. Use 'eq' instead.", op)
+ case "!=" => subsetError("Unsupported operation '%s'. Use 'ne' instead.", op)
+ case "eq" => eq
+ case "ne" => ne
+ case "lt" => lt
+ case "gt" => gt
+ case "le" => le
+ case "ge" => ge
case _ => subsetError("Unsupported operation '%s' on type %s.", op, convergedArgType)
Review Comment:
> we should leave the error as a subsetError that results in a SDE rather than an Assert.invariantFailed
Yeah, the way you made the change this should stay a subset error. In my head the change would be something like this:
```scala
if (convergedArgType == PrimType.HexBinary && (op == "lt || op == "gt" || op == "le" || op == ge")) {
subsetError(...)
}
op match {
...
}
```
So the hexBinary test would be outside the match case. If it were done that way the default case would want to be an Assert.invariant. But they way did in the match case you're right that it should be a subset error. And your way is probably more scala-y so is better than what I was thinking. And your new hexBinary tests confirm its doing the right thing :+1:
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
Posted by "tuxji (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1199:
URL: https://github.com/apache/daffodil/pull/1199#discussion_r1544581708
##########
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 don't remember, but maybe we do it to mimic PState, which is mutable? Back in the early days PState wasn't mutable and we just copied it each time something changed, but they overhead of copies started becoming very noticeable so we switched to a mutable PState.
DState might be small enough that the overhead of creating multiple DStates might not be noticeable? Hard to say with out making the change, and it would probably be a pretty significant change--all the run() functions would need to be changed to allocate a new DState. I would be much cleaner and less prone to error though.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1199:
URL: https://github.com/apache/daffodil/pull/1199#discussion_r1544650251
##########
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:
Here's the latest DFDL spec: https://github.com/OpenGridForum/DFDL/blob/master/docs/current/gwdrp-dfdl-v1.0.8-GFD-R-P.240.pdf
Page 184 talks about these functions, but doesn't clarify what it means by inclusive/exclusive.
An alternative interpretation is that the min/max values are treated similar to minInclusive, maxInclusive, minExclusive, and maxExclusive facet restrictions, which is more like what tuxji (and me) were expecting. But your reasoning makes sense too.
@mbeckerle, do you know the intention of these functions?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1199:
URL: https://github.com/apache/daffodil/pull/1199#discussion_r1544731538
##########
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
+ val c = conversions
+ val res = new CompiledDPath(DFDLCheckRangeExclusive(argDPath, rangeFrom, rangeTo) +: c)
Review Comment:
Just a note that the above code is run treats the minVal as inclusive, or exclusive depending on the function called, answering the question within this comment: https://github.com/apache/daffodil/pull/1199#discussion_r1544650251
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1199:
URL: https://github.com/apache/daffodil/pull/1199#discussion_r1544640833
##########
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:
There wasn't any information on the DFDL spec about how to treat exclusive, and the redmine link on the tickets leads to a 404 error and attempts to find the issue referenced were not successful so I used the mathematical definition of exclusive, which deals with just the upper value (as seen in this SO post: https://stackoverflow.com/questions/39010041/what-is-the-meaning-of-exclusive-and-inclusive-when-describing-number-ranges)
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
Posted by "tuxji (via GitHub)" <gi...@apache.org>.
tuxji commented on code in PR #1199:
URL: https://github.com/apache/daffodil/pull/1199#discussion_r1546386712
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/ComparisonOps.scala:
##########
@@ -17,10 +17,176 @@
package org.apache.daffodil.runtime1.dpath
+import org.apache.daffodil.lib.exceptions.Assert
import org.apache.daffodil.lib.util.Numbers._
+import org.apache.daffodil.runtime1.dpath.NodeInfo.PrimType
import org.apache.daffodil.runtime1.infoset.DataValue.DataValueBool
import org.apache.daffodil.runtime1.infoset.DataValue.DataValuePrimitive
+/**
+ * Case class used for ordering the return of the appropriate comparison operations for each primitive kind
+ * @param eq represents the equality comparison
+ * @param ne represents the unequality comparison
+ * @param lt represents the less than comparison
+ * @param le represents the less than and equal to comparison
Review Comment:
less than or equal to
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/ComparisonOps.scala:
##########
@@ -17,10 +17,176 @@
package org.apache.daffodil.runtime1.dpath
+import org.apache.daffodil.lib.exceptions.Assert
import org.apache.daffodil.lib.util.Numbers._
+import org.apache.daffodil.runtime1.dpath.NodeInfo.PrimType
import org.apache.daffodil.runtime1.infoset.DataValue.DataValueBool
import org.apache.daffodil.runtime1.infoset.DataValue.DataValuePrimitive
+/**
+ * Case class used for ordering the return of the appropriate comparison operations for each primitive kind
+ * @param eq represents the equality comparison
+ * @param ne represents the unequality comparison
+ * @param lt represents the less than comparison
+ * @param le represents the less than and equal to comparison
+ * @param gt represents the greater than comparison
+ * @param ge represents the greater than and equal to comparison
Review Comment:
greater than or equal to
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1199:
URL: https://github.com/apache/daffodil/pull/1199#discussion_r1544395957
##########
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
+ val c = conversions
+ val res = new CompiledDPath(DFDLCheckRangeExclusive(argDPath, rangeFrom, rangeTo) +: c)
Review Comment:
DFDLCheckRangeExclusiveExpr is exactly the same as the Inclusive version except for this one line. What are your thoughts on only having a single `DFDLCheckRangeExpr` that takes a boolean parameter (e.g. `isExcsluve`) that signifies inclusive vs exclusive? Then, using a suggestion below you could have a single `DFDLCheckRange` class that accepts min/max comparision operations, something like:
```scala
val (_, _, lt, le, _) = ComparisonOpts.forType(targetType)
val compare = if (isExclusive) lt else le
val res = new CompiledDPath(DFDLCheckRange(argDPath, rangeFrom, rangeTo, compare) +: c)
```
Then we can combine DFDLCheckRangeInclusive/Exclusive to a single `DFDLCheckRange` class that does something like:
```scala
def run(...) {
...
val res = compare.operate(minValue, dataValue) && compare.operate(dataValue, maxValue)
res
}
```
The benefit to this is approach is we avoid lots of duplication and we also figure out the right comparison logic/function at schema compile time instead of run-time.
##########
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
Review Comment:
This match case isn't exhaustive. It's missing Short, Byte, and Long.
Though, instead of doing the type check at runtime and duplicating a bunch of checks, what if we figure out the right [ComparisonOps](https://github.com/apache/daffodil/blob/main/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/ComparisonOps.scala) to use at compile time?
I suggest we create a new static function, something like
```scala
object ComparisonOps {
def forType(type: NodeInfo.Kind): (CompareOpBase, CompareOpBase, CompareOpBase, CompareOpBase, CompareOpBase, CompareOpBase) = {
type match {
case PrimType.Boolean => (EQ_Compare, NE_Compare, LT_Boolean, LE_Boolean, GT_Boolean, GE_Boolean)
case PrimType.Integer => (EQ_Compare, NE_Compare, LT_Integer, LE_Integer, GT_Integer, GE_Integer)
...
}
}
...
}
```
So it returns a 6-tuple of all the comparison functions that might be needed for a specific type. We can use this anywhere we need comparison functions, like in the `DFDLCheckRangeExpr` suggested above. We could also use it in `ComparisonExpression`. That probably wants to be something like this:
```
lazy val compareOp = {
val (eq, ne, lt, le, gt, ge) = CopmarisonOps.forType(convertedArgType)
op match {
...
case "eq"=> eq
case "ne" => ne
case "lt" => lt
...
}
}
```
Note that all the comparison functions used in `ComparisionExpression` should be used in the new `forType` function.
We could also could use it in `repTypeComparers`, e.g.
```scala
lazy val (
repTypeCompareLT: NumberCompareOp,
repTypeCompareLE: NumberCompareOp,
) = LV('repTypeComparers) {
val (_, _, lt, le, _, _) = ComparisonOps.forType(repTypeElementDec.primType)
(lt, le)
}
```
There might be other places where we have a match/case to figure out comparison ops that could be simplified in this way an remove some duplication.
##########
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:
Also, if you reference arg1/arg2/arg3 it will force evaluation of line 3056 and call checkArgCount(3), so you can also remove the duplicate check on 3061.
##########
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:
The `currentNode` DState can be used by a `run` implementation to know where to evaluate a relative path expression from (e.g. `../foo/bar/baz`).
But calling `run` can also potentially mutate `currentNode`, changing it to something else, which can interfere with other calls to run, so it needs to be reset.
For example, say we had something like this:
```
dfdl:checkRangeExclusive(../foo/bar/baz, ../uno/dos/tres, ../one/two/three)`
```
Before we call `run()`, let's say `currentNode` points to an element called `root`.
After we call `dataRecipe.run()` which evaluates `../foo/bar/baz` relative to `root`, the `currentNode` will point the `baz` element, because DState was mutated. If we do not reset `currentNode` back to `root`, then when we call `minRecipe.run()` to evaluate `../uno/dos/tres` that relative path will be evaluated relative to the `baz` element instead of the `root` element. So that's why we need to save and restore curentNode after each call to run.
Note that we don't have to restore `currentNode` after calling `maxReipce.run()` because we aren't personally calling anymore runs, so whatever `currentNode` ended up as doesn't matter to us. If some other expression was called that led to this run() being called, then they were responsible for saving and restoring currentNode if they needed it.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1199:
URL: https://github.com/apache/daffodil/pull/1199#discussion_r1545790493
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -258,116 +258,21 @@ case class ComparisonExpression(op: String, adds: List[Expression])
with BooleanExpression {
lazy val compareOp: CompareOpBase = {
- import NodeInfo.PrimType._
- import NodeInfo.ArrayIndex
- (op, convergedArgType) match {
- case ("<", _) => subsetError("Unsupported operation '%s'. Use 'lt' instead.", op)
- case (">", _) => subsetError("Unsupported operation '%s'. Use 'gt' instead.", op)
- case ("<=", _) => subsetError("Unsupported operation '%s'. Use 'le' instead.", op)
- case (">=", _) => subsetError("Unsupported operation '%s'. Use 'ge' instead.", op)
- case ("=", _) => subsetError("Unsupported operation '%s'. Use 'eq' instead.", op)
- case ("!=", _) => subsetError("Unsupported operation '%s'. Use 'ne' instead.", op)
-
- case ("eq", HexBinary) => EQ_CompareByteArray
- case ("ne", HexBinary) => NE_CompareByteArray
- case ("eq", _) => EQ_Compare
- case ("ne", _) => NE_Compare
-
- case ("lt", Boolean) => LT_Boolean
- case ("gt", Boolean) => GT_Boolean
- case ("le", Boolean) => LE_Boolean
- case ("ge", Boolean) => GE_Boolean
-
- case ("lt", Date) => LT_Date
- case ("gt", Date) => GT_Date
- case ("le", Date) => LE_Date
- case ("ge", Date) => GE_Date
-
- case ("lt", Time) => LT_Time
- case ("gt", Time) => GT_Time
- case ("le", Time) => LE_Time
- case ("ge", Time) => GE_Time
-
- case ("lt", DateTime) => LT_DateTime
- case ("gt", DateTime) => GT_DateTime
- case ("le", DateTime) => LE_DateTime
- case ("ge", DateTime) => GE_DateTime
-
- case ("lt", String) => LT_String
- case ("gt", String) => GT_String
- case ("le", String) => LE_String
- case ("ge", String) => GE_String
-
- case ("lt", Decimal) => LT_Decimal
- case ("gt", Decimal) => GT_Decimal
- case ("le", Decimal) => LE_Decimal
- case ("ge", Decimal) => GE_Decimal
-
- case ("lt", Integer) => LT_Integer
- case ("gt", Integer) => GT_Integer
- case ("le", Integer) => LE_Integer
- case ("ge", Integer) => GE_Integer
-
- case ("lt", NonNegativeInteger) => LT_NonNegativeInteger
- case ("gt", NonNegativeInteger) => GT_NonNegativeInteger
- case ("le", NonNegativeInteger) => LE_NonNegativeInteger
- case ("ge", NonNegativeInteger) => GE_NonNegativeInteger
-
- case ("lt", UnsignedLong) => LT_UnsignedLong
- case ("gt", UnsignedLong) => GT_UnsignedLong
- case ("le", UnsignedLong) => LE_UnsignedLong
- case ("ge", UnsignedLong) => GE_UnsignedLong
-
- case ("lt", Long) => LT_Long
- case ("gt", Long) => GT_Long
- case ("le", Long) => LE_Long
- case ("ge", Long) => GE_Long
-
- case ("lt", UnsignedInt) => LT_UnsignedInt
- case ("gt", UnsignedInt) => GT_UnsignedInt
- case ("le", UnsignedInt) => LE_UnsignedInt
- case ("ge", UnsignedInt) => GE_UnsignedInt
-
- case ("lt", ArrayIndex) => LT_UnsignedInt
- case ("gt", ArrayIndex) => GT_UnsignedInt
- case ("le", ArrayIndex) => LE_UnsignedInt
- case ("ge", ArrayIndex) => GE_UnsignedInt
-
- case ("lt", Int) => LT_Int
- case ("gt", Int) => GT_Int
- case ("le", Int) => LE_Int
- case ("ge", Int) => GE_Int
-
- case ("lt", UnsignedShort) => LT_UnsignedShort
- case ("gt", UnsignedShort) => GT_UnsignedShort
- case ("le", UnsignedShort) => LE_UnsignedShort
- case ("ge", UnsignedShort) => GE_UnsignedShort
-
- case ("lt", Short) => LT_Short
- case ("gt", Short) => GT_Short
- case ("le", Short) => LE_Short
- case ("ge", Short) => GE_Short
-
- case ("lt", UnsignedByte) => LT_UnsignedByte
- case ("gt", UnsignedByte) => GT_UnsignedByte
- case ("le", UnsignedByte) => LE_UnsignedByte
- case ("ge", UnsignedByte) => GE_UnsignedByte
-
- case ("lt", Byte) => LT_Byte
- case ("gt", Byte) => GT_Byte
- case ("le", Byte) => LE_Byte
- case ("ge", Byte) => GE_Byte
-
- case ("lt", Float) => LT_Float
- case ("gt", Float) => GT_Float
- case ("le", Float) => LE_Float
- case ("ge", Float) => GE_Float
-
- case ("lt", Double) => LT_Double
- case ("gt", Double) => GT_Double
- case ("le", Double) => LE_Double
- case ("ge", Double) => GE_Double
-
+ val (eq, ne, lt, le, gt, ge) = ComparisonOps.forType(convergedArgType)
+
+ op match {
+ case "<" => subsetError("Unsupported operation '%s'. Use 'lt' instead.", op)
+ case ">" => subsetError("Unsupported operation '%s'. Use 'gt' instead.", op)
+ case "<=" => subsetError("Unsupported operation '%s'. Use 'le' instead.", op)
+ case ">=" => subsetError("Unsupported operation '%s'. Use 'ge' instead.", op)
+ case "=" => subsetError("Unsupported operation '%s'. Use 'eq' instead.", op)
+ case "!=" => subsetError("Unsupported operation '%s'. Use 'ne' instead.", op)
+ case "eq" => eq
+ case "ne" => ne
+ case "lt" => lt
+ case "gt" => gt
+ case "le" => le
+ case "ge" => ge
case _ => subsetError("Unsupported operation '%s' on type %s.", op, convergedArgType)
Review Comment:
Created https://issues.apache.org/jira/browse/DAFFODIL-2886
Though I think for ease of creating TDML tests, we should leave the error as a subsetError that results in a SDE rather than an Assert.invariantFailed. I can easily see someone creating a schema that tries to compare hexBinary elems and an SDE would be a better error for them to get until this is implemented.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
Posted by "tuxji (via GitHub)" <gi...@apache.org>.
tuxji commented on code in PR #1199:
URL: https://github.com/apache/daffodil/pull/1199#discussion_r1544570225
##########
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:
> Note that we don't have to restore currentNode after calling maxReipce.run() because we aren't personally calling anymore runs, so whatever currentNode ended up as doesn't matter to us. If some other expression was called that led to this run() being called, then they were responsible for saving and restoring currentNode if they needed it.
Actually, why do `run` implementations even mutate expression state (current node, current value) inside `dstate` at all? A more functional way would be to pass `dstate` or a smaller expression state contained by `dstate` to `run(exprState)` as a parameter and receive a new expression state (with the new current node and new current value) as a result value from the `run(exprState)` call. Then these `dstate.setCurrentNode(saved)` calls would go away because dstate's expression state (current node, current value) never gets mutated even after it's passed to the `run` implementation.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1199:
URL: https://github.com/apache/daffodil/pull/1199#discussion_r1544535003
##########
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
Review Comment:
An improvement, maybe `forType` can be implemented as a `Map` for constant lookup and reduced allocations? e.g.
```scala
object ComparisonOps {
val forType: Map[NodeInfo.Kind, (CompareOpBase, CompareOpBase, CompareOpBase, CompareOpBase, CompareOpBase, CompareOpBase)] =
Map(
PrimType.Boolean -> (EQ_Compare, NE_Compare, LT_Boolean, LE_Boolean, GT_Boolean, GE_Boolean),
PrimType.Integer -> (EQ_Compare, NE_Compare, LT_Integer, LE_Integer, GT_Integer, GE_Integer),
...
)
}
```
The use is still the same, e.g. `ComparisonOps.forType(somePrimType)`
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1199:
URL: https://github.com/apache/daffodil/pull/1199#discussion_r1545271846
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/ComparisonOps.scala:
##########
@@ -17,10 +17,42 @@
package org.apache.daffodil.runtime1.dpath
+import org.apache.daffodil.lib.exceptions.Assert
import org.apache.daffodil.lib.util.Numbers._
+import org.apache.daffodil.runtime1.dpath.NodeInfo.PrimType
import org.apache.daffodil.runtime1.infoset.DataValue.DataValueBool
import org.apache.daffodil.runtime1.infoset.DataValue.DataValuePrimitive
+object ComparisonOps {
+ lazy val forType: Map[
Review Comment:
Can you add scaladoc for this, especially making sure to mention the order of the returned 6-tuple so we don't have to read the code to figure out which operation is which? It's not too hard to figure it out, but it would make things just a little easier. Maybe also include an example of its use with the `val (...) = ...` to easily extract the comparison ops?
##########
daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_expressions/expressions.tdml:
##########
@@ -7684,4 +7684,230 @@ 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_inc">
+ <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:sequence>
+ </xs:complexType>
+ </xs:element>
+
+ <xs:element name="expMinMax_string_exc">
+ <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="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>false</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>
Review Comment:
Nice test :+1:, if we converged the types wrong there's a good chance `1.9` would be cast to int, become `1`, and `isInRangeInc` would be true and the test would fail.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -3041,6 +2952,56 @@ case class DFDLCheckConstraintsExpr(
}
+case class DFDLCheckRangeExpr(
+ nameAsParsed: String,
+ fnQName: RefQName,
+ args: List[Expression],
+ isExclusive: Boolean,
+) extends FunctionCallBase(nameAsParsed, fnQName, args) {
+
+ lazy val List(arg1, arg2, arg3) = { checkArgCount(3); args }
+
+ override lazy val children = args
+
+ override lazy val compiledDPath = {
+ val argDPath = arg1.compiledDPath
+ val rangeFrom = arg2.compiledDPath
+ val rangeTo = arg3.compiledDPath
+ val c = conversions
+ val (_, _, lt, le, _, _) = ComparisonOps.forType(targetTypeForSubExpr)
+ val compare = if (isExclusive) lt else le
+ val res = new CompiledDPath(DFDLCheckRange(argDPath, rangeFrom, rangeTo, compare) +: c)
+ res
+ }
+
+ // we set the calculation to a lazy val so it's evaluated only once for each
+ // call to targetTypeForSubexpression since we don't care about the specific
+ // value of subexpr in targetTypeForSubexpression, and we need all 3 sub
+ // expressions to come to the right target target for the args
+ private lazy val targetTypeForSubExpr: NodeInfo.Kind = {
Review Comment:
I think in places where we do something like this (i.e. figure out a common type for all arguments to convert to) we call this `convergedArgType`. We should do the same here for consistentcy.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -258,116 +258,21 @@ case class ComparisonExpression(op: String, adds: List[Expression])
with BooleanExpression {
lazy val compareOp: CompareOpBase = {
- import NodeInfo.PrimType._
- import NodeInfo.ArrayIndex
- (op, convergedArgType) match {
- case ("<", _) => subsetError("Unsupported operation '%s'. Use 'lt' instead.", op)
- case (">", _) => subsetError("Unsupported operation '%s'. Use 'gt' instead.", op)
- case ("<=", _) => subsetError("Unsupported operation '%s'. Use 'le' instead.", op)
- case (">=", _) => subsetError("Unsupported operation '%s'. Use 'ge' instead.", op)
- case ("=", _) => subsetError("Unsupported operation '%s'. Use 'eq' instead.", op)
- case ("!=", _) => subsetError("Unsupported operation '%s'. Use 'ne' instead.", op)
-
- case ("eq", HexBinary) => EQ_CompareByteArray
- case ("ne", HexBinary) => NE_CompareByteArray
- case ("eq", _) => EQ_Compare
- case ("ne", _) => NE_Compare
-
- case ("lt", Boolean) => LT_Boolean
- case ("gt", Boolean) => GT_Boolean
- case ("le", Boolean) => LE_Boolean
- case ("ge", Boolean) => GE_Boolean
-
- case ("lt", Date) => LT_Date
- case ("gt", Date) => GT_Date
- case ("le", Date) => LE_Date
- case ("ge", Date) => GE_Date
-
- case ("lt", Time) => LT_Time
- case ("gt", Time) => GT_Time
- case ("le", Time) => LE_Time
- case ("ge", Time) => GE_Time
-
- case ("lt", DateTime) => LT_DateTime
- case ("gt", DateTime) => GT_DateTime
- case ("le", DateTime) => LE_DateTime
- case ("ge", DateTime) => GE_DateTime
-
- case ("lt", String) => LT_String
- case ("gt", String) => GT_String
- case ("le", String) => LE_String
- case ("ge", String) => GE_String
-
- case ("lt", Decimal) => LT_Decimal
- case ("gt", Decimal) => GT_Decimal
- case ("le", Decimal) => LE_Decimal
- case ("ge", Decimal) => GE_Decimal
-
- case ("lt", Integer) => LT_Integer
- case ("gt", Integer) => GT_Integer
- case ("le", Integer) => LE_Integer
- case ("ge", Integer) => GE_Integer
-
- case ("lt", NonNegativeInteger) => LT_NonNegativeInteger
- case ("gt", NonNegativeInteger) => GT_NonNegativeInteger
- case ("le", NonNegativeInteger) => LE_NonNegativeInteger
- case ("ge", NonNegativeInteger) => GE_NonNegativeInteger
-
- case ("lt", UnsignedLong) => LT_UnsignedLong
- case ("gt", UnsignedLong) => GT_UnsignedLong
- case ("le", UnsignedLong) => LE_UnsignedLong
- case ("ge", UnsignedLong) => GE_UnsignedLong
-
- case ("lt", Long) => LT_Long
- case ("gt", Long) => GT_Long
- case ("le", Long) => LE_Long
- case ("ge", Long) => GE_Long
-
- case ("lt", UnsignedInt) => LT_UnsignedInt
- case ("gt", UnsignedInt) => GT_UnsignedInt
- case ("le", UnsignedInt) => LE_UnsignedInt
- case ("ge", UnsignedInt) => GE_UnsignedInt
-
- case ("lt", ArrayIndex) => LT_UnsignedInt
- case ("gt", ArrayIndex) => GT_UnsignedInt
- case ("le", ArrayIndex) => LE_UnsignedInt
- case ("ge", ArrayIndex) => GE_UnsignedInt
-
- case ("lt", Int) => LT_Int
- case ("gt", Int) => GT_Int
- case ("le", Int) => LE_Int
- case ("ge", Int) => GE_Int
-
- case ("lt", UnsignedShort) => LT_UnsignedShort
- case ("gt", UnsignedShort) => GT_UnsignedShort
- case ("le", UnsignedShort) => LE_UnsignedShort
- case ("ge", UnsignedShort) => GE_UnsignedShort
-
- case ("lt", Short) => LT_Short
- case ("gt", Short) => GT_Short
- case ("le", Short) => LE_Short
- case ("ge", Short) => GE_Short
-
- case ("lt", UnsignedByte) => LT_UnsignedByte
- case ("gt", UnsignedByte) => GT_UnsignedByte
- case ("le", UnsignedByte) => LE_UnsignedByte
- case ("ge", UnsignedByte) => GE_UnsignedByte
-
- case ("lt", Byte) => LT_Byte
- case ("gt", Byte) => GT_Byte
- case ("le", Byte) => LE_Byte
- case ("ge", Byte) => GE_Byte
-
- case ("lt", Float) => LT_Float
- case ("gt", Float) => GT_Float
- case ("le", Float) => LE_Float
- case ("ge", Float) => GE_Float
-
- case ("lt", Double) => LT_Double
- case ("gt", Double) => GT_Double
- case ("le", Double) => LE_Double
- case ("ge", Double) => GE_Double
-
+ val (eq, ne, lt, le, gt, ge) = ComparisonOps.forType(convergedArgType)
+
+ op match {
+ case "<" => subsetError("Unsupported operation '%s'. Use 'lt' instead.", op)
+ case ">" => subsetError("Unsupported operation '%s'. Use 'gt' instead.", op)
+ case "<=" => subsetError("Unsupported operation '%s'. Use 'le' instead.", op)
+ case ">=" => subsetError("Unsupported operation '%s'. Use 'ge' instead.", op)
+ case "=" => subsetError("Unsupported operation '%s'. Use 'eq' instead.", op)
+ case "!=" => subsetError("Unsupported operation '%s'. Use 'ne' instead.", op)
+ case "eq" => eq
+ case "ne" => ne
+ case "lt" => lt
+ case "gt" => gt
+ case "le" => le
+ case "ge" => ge
case _ => subsetError("Unsupported operation '%s' on type %s.", op, convergedArgType)
Review Comment:
I think we currently hit this subsetError at compile time for lt, le, gt, ge operations with xs:hexBinary, but I don't think we will anymore? Instead we will create the new LT_HexBinary, etc. classes at compile time and fail at runtime with an assertion. We probably want to keep the same subset behavior so it SDEs at compile time?
So, can you add a check somewhere in this function that creates this subset error for hex binary and those operations so we get the compile time subset error instead of a runtime assertion?
Also, I think this case probably wants to be an `Assert.invariantFailed` now, since this ComparisonExpression should only be created with one of these operators (see `def Comp` in DFDLExpressionParser.scala). If we ever hit this case, it's probably a bug.
Also, should we create a ticket to implement the rest of the operators for hexBinary and can remove this subset error and support those operators for hexBinary? Maybe we wait until we drop support for Java 8 and can use [Arrays.compare()](https://docs.oracle.com/javase%2F9%2Fdocs%2Fapi%2F%2F/java/util/Arrays.html#compare-byte:A-byte:A-) so it should be a trivial change.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1199:
URL: https://github.com/apache/daffodil/pull/1199#discussion_r1544750045
##########
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:
Inclusive means both ends are part of the range, Exclusive means both ends are outside the range.
This is only seems ambiguous in English as in "1 to 10 inclusive" where it becomes a question as to whether you mean inclusive to apply to "1 to 10", or just "10". But as "inclusive" is an adjective that describes a characteristic of an interval/range, not a single number. So I claim this is not ambiguous. It applies to both ends of the range, always. It requires more words to talk about the mixed combinations of including and excluding.
In the DFDL functions, both ends of the ranges are definitely what was intended.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1199:
URL: https://github.com/apache/daffodil/pull/1199#discussion_r1545371322
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/ComparisonOps.scala:
##########
@@ -17,10 +17,42 @@
package org.apache.daffodil.runtime1.dpath
+import org.apache.daffodil.lib.exceptions.Assert
import org.apache.daffodil.lib.util.Numbers._
+import org.apache.daffodil.runtime1.dpath.NodeInfo.PrimType
import org.apache.daffodil.runtime1.infoset.DataValue.DataValueBool
import org.apache.daffodil.runtime1.infoset.DataValue.DataValuePrimitive
+object ComparisonOps {
+ lazy val forType: Map[
Review Comment:
Another option might be to create a new case class and make that the value of the map, e.g.:
```scala
case class ComparisonOps(eq: ComparisonOp, ne: ComparisonOp, lt: ComparisonOp, ...)
...
Map(
PrimType.Boolean -> ComparisonOps(EQ_Compare, NE_Compare, LT_Boolean, ...),
...
)
```
Then uses become something like:
```scala
val compOps = ComparisonOps.forType(targetTypeForSubExpr)
val compare = if (isExclusive) compOps.lt else compOps.le
```
It's a little more verbose, but using it is a bit more clear and less prone to errors since it's impossible for callers of `forType` to get the order of the 6-tuple wrong.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT merged PR #1199:
URL: https://github.com/apache/daffodil/pull/1199
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org