You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "stevedlawrence (via GitHub)" <gi...@apache.org> on 2024/04/01 00:52:16 UTC
Re: [PR] Add support for dfdl:checkRangeInclusive/checkRangeExclusive functions [daffodil]
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