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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
