[GitHub] [daffodil] mbeckerle commented on a diff in pull request #1070: Remove dfdlx:inputTypeCalc and dfdlx:outputTypeCalc

["mbeckerle (via GitHub)" <gi...@apache.org>]

mbeckerle commented on code in PR #1070:
URL: https://github.com/apache/daffodil/pull/1070#discussion_r1298825577


##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/TypeCalculator.scala:
##########
@@ -223,159 +186,34 @@ class KeysetValueTypeCalculatorUnordered(
   override def inputTypeCalc(
     x: DataValuePrimitive,
     xType: NodeInfo.Kind,
-  ): (DataValuePrimitiveNullable, Maybe[Error]) = {
+  ): Either[Error, DataValuePrimitiveNullable] = {
     valueMap.get(x) match {
-      case Some(a) => (a, Maybe.Nope)
-      case None =>
-        (DataValue.NoValue, One(s"Value ${x} not found in enumeration dfdlx:repValues"))
+      case Some(a) => Right(a)
+      case None => Left(s"Value ${x} not found in enumeration dfdlx:repValues")
     }
   }
 
   override def outputTypeCalc(
     x: DataValuePrimitive,
     xType: NodeInfo.Kind,
-  ): (DataValuePrimitiveNullable, Maybe[Error]) = {
+  ): Either[Error, DataValuePrimitiveNullable] = {
     unparseMap.get(x) match {
-      case Some(v) => (v, Maybe.Nope)
-      case None => (DataValue.NoValue, One(s"Value ${x} not found in enumeration"))
+      case Some(v) => Right(v)
+      case None => Left(s"Value ${x} not found in enumeration")
     }
   }
 
 }
 
-class ExpressionTypeCalculator(
-  private val maybeInputTypeCalcDelay: Delay[Maybe[CompiledExpression[AnyRef]]],
-  private val maybeOutputTypeCalcDelay: Delay[Maybe[CompiledExpression[AnyRef]]],
-  srcType: NodeInfo.Kind,
-  dstType: NodeInfo.Kind,
-) extends TypeCalculator(srcType, dstType) {
-
-  /*
-   * objects with Delay arguments for functional programming construction of
-   * cyclic graphs, need a way to force the delays, resulting in an ordinary
-   * (though cyclic) data structure.
-   */
-  final override def initialize(): Unit = {
-    super.initialize()
-    maybeInputTypeCalc
-    maybeOutputTypeCalc
-  }
-
-  override def supportsParse = maybeInputTypeCalc.isDefined
-  override def supportsUnparse = maybeOutputTypeCalc.isDefined
-
-  /*
-   * Compiling DPath expressions may need to evaluate typeCalculators in order to lookup their srcType and dstType.
-   * To prevent circular dependencies, this means that the underlying expressions must be lazy.
-   *
-   * Since these fields must be lazy, we cannot use them to determine supportsParse or supportUnparse
-   */
-  lazy val maybeInputTypeCalc = maybeInputTypeCalcDelay.value
-  lazy val maybeOutputTypeCalc = maybeOutputTypeCalcDelay.value
-
-  // The class TypeValueCalc will verify that supports(Un)Parse is true when nessasary
-  // Therefore, if we ever call the below functions, we know that the relevent Maybe object is defined.
-
-  // TODO, pass x into DPath state
-
+class IdentityTypeCalculator(srcType: NodeInfo.Kind) extends TypeCalculator(srcType, srcType) {

Review Comment:
   Curious as to why we still need this. For unit tests?



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/TypeCalculator.scala:
##########
@@ -21,135 +21,98 @@ import scala.collection.immutable.HashMap
 import scala.collection.immutable.HashSet
 
 import org.apache.daffodil.lib.exceptions.Assert
-import org.apache.daffodil.lib.util.Delay
 import org.apache.daffodil.lib.util.Maybe
-import org.apache.daffodil.lib.util.Maybe.One
 import org.apache.daffodil.lib.util.Numbers
-import org.apache.daffodil.lib.xml.QNameBase
-import org.apache.daffodil.runtime1.dpath.DState
 import org.apache.daffodil.runtime1.dpath.NodeInfo
-import org.apache.daffodil.runtime1.dsom.CompiledExpression
 import org.apache.daffodil.runtime1.infoset.DataValue
 import org.apache.daffodil.runtime1.infoset.DataValue.DataValuePrimitive
 import org.apache.daffodil.runtime1.infoset.DataValue.DataValuePrimitiveNullable
 import org.apache.daffodil.runtime1.processors.parsers.PState
 import org.apache.daffodil.runtime1.processors.parsers.ParseError
 import org.apache.daffodil.runtime1.processors.unparsers.UState
 
+/**
+ * A TypeCalculator is a class that performs some kind of conversion from one DataValuePrimitive
+ * to another DataValuePrimitive. In general, this is done to convert a parsed physical
+ * representation from the data to a logical representation that goes in the infoset, and
+ * reverse on unparse. The former is done in the inputTypeCalc functions, and the latter in the
+ * outputTypeCalc functions. Generally the conversion is to a different type (e.g. xs:int to
+ * xs:string), but this need not be the case.

Review Comment:
   Do we in fact still support other than int to/from string?
   
   Possibly this class/framework does, but other aspects of  these extensions do not?
   
   If this is only used (and tested) for int to/from string, we should not lay claim to other conversions. 



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/TypeCalculator.scala:
##########
@@ -21,135 +21,98 @@ import scala.collection.immutable.HashMap
 import scala.collection.immutable.HashSet
 
 import org.apache.daffodil.lib.exceptions.Assert
-import org.apache.daffodil.lib.util.Delay
 import org.apache.daffodil.lib.util.Maybe
-import org.apache.daffodil.lib.util.Maybe.One
 import org.apache.daffodil.lib.util.Numbers
-import org.apache.daffodil.lib.xml.QNameBase
-import org.apache.daffodil.runtime1.dpath.DState
 import org.apache.daffodil.runtime1.dpath.NodeInfo
-import org.apache.daffodil.runtime1.dsom.CompiledExpression
 import org.apache.daffodil.runtime1.infoset.DataValue
 import org.apache.daffodil.runtime1.infoset.DataValue.DataValuePrimitive
 import org.apache.daffodil.runtime1.infoset.DataValue.DataValuePrimitiveNullable
 import org.apache.daffodil.runtime1.processors.parsers.PState
 import org.apache.daffodil.runtime1.processors.parsers.ParseError
 import org.apache.daffodil.runtime1.processors.unparsers.UState
 
+/**
+ * A TypeCalculator is a class that performs some kind of conversion from one DataValuePrimitive
+ * to another DataValuePrimitive. In general, this is done to convert a parsed physical
+ * representation from the data to a logical representation that goes in the infoset, and
+ * reverse on unparse. The former is done in the inputTypeCalc functions, and the latter in the
+ * outputTypeCalc functions. Generally the conversion is to a different type (e.g. xs:int to
+ * xs:string), but this need not be the case.
+ *
+ * The different TypeCalculator implementations allow for different conversions based on
+ * different properties provided in the schema (e.g. repType, enumerations, unions, repValues,

Review Comment:
   I'm not recollecting what we do with unions and this stuff. That may be only the floating type calc functions that have been removed. 



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/TypeCalculator.scala:
##########
@@ -223,159 +186,34 @@ class KeysetValueTypeCalculatorUnordered(
   override def inputTypeCalc(
     x: DataValuePrimitive,
     xType: NodeInfo.Kind,
-  ): (DataValuePrimitiveNullable, Maybe[Error]) = {
+  ): Either[Error, DataValuePrimitiveNullable] = {
     valueMap.get(x) match {
-      case Some(a) => (a, Maybe.Nope)
-      case None =>
-        (DataValue.NoValue, One(s"Value ${x} not found in enumeration dfdlx:repValues"))
+      case Some(a) => Right(a)
+      case None => Left(s"Value ${x} not found in enumeration dfdlx:repValues")
     }
   }
 
   override def outputTypeCalc(
     x: DataValuePrimitive,
     xType: NodeInfo.Kind,
-  ): (DataValuePrimitiveNullable, Maybe[Error]) = {
+  ): Either[Error, DataValuePrimitiveNullable] = {
     unparseMap.get(x) match {
-      case Some(v) => (v, Maybe.Nope)
-      case None => (DataValue.NoValue, One(s"Value ${x} not found in enumeration"))
+      case Some(v) => Right(v)
+      case None => Left(s"Value ${x} not found in enumeration")
     }
   }
 
 }
 
-class ExpressionTypeCalculator(
-  private val maybeInputTypeCalcDelay: Delay[Maybe[CompiledExpression[AnyRef]]],
-  private val maybeOutputTypeCalcDelay: Delay[Maybe[CompiledExpression[AnyRef]]],
-  srcType: NodeInfo.Kind,
-  dstType: NodeInfo.Kind,
-) extends TypeCalculator(srcType, dstType) {
-
-  /*
-   * objects with Delay arguments for functional programming construction of
-   * cyclic graphs, need a way to force the delays, resulting in an ordinary
-   * (though cyclic) data structure.
-   */
-  final override def initialize(): Unit = {
-    super.initialize()
-    maybeInputTypeCalc
-    maybeOutputTypeCalc
-  }
-
-  override def supportsParse = maybeInputTypeCalc.isDefined
-  override def supportsUnparse = maybeOutputTypeCalc.isDefined
-
-  /*
-   * Compiling DPath expressions may need to evaluate typeCalculators in order to lookup their srcType and dstType.
-   * To prevent circular dependencies, this means that the underlying expressions must be lazy.
-   *
-   * Since these fields must be lazy, we cannot use them to determine supportsParse or supportUnparse
-   */
-  lazy val maybeInputTypeCalc = maybeInputTypeCalcDelay.value
-  lazy val maybeOutputTypeCalc = maybeOutputTypeCalcDelay.value
-
-  // The class TypeValueCalc will verify that supports(Un)Parse is true when nessasary
-  // Therefore, if we ever call the below functions, we know that the relevent Maybe object is defined.
-
-  // TODO, pass x into DPath state
-
+class IdentityTypeCalculator(srcType: NodeInfo.Kind) extends TypeCalculator(srcType, srcType) {
   override def inputTypeCalc(
     x: DataValuePrimitive,
     xType: NodeInfo.Kind,
-  ): (DataValuePrimitiveNullable, Maybe[Error]) =
-    Assert.invariantFailed(
-      "inputTypeCalc not implemented on ExpressionTypeCalculator. Call the more specialized forms directly",
-    )
+  ): Either[Error, DataValuePrimitiveNullable] = Right(x)
   override def outputTypeCalc(
     x: DataValuePrimitive,
     xType: NodeInfo.Kind,
-  ): (DataValuePrimitiveNullable, Maybe[Error]) =
-    Assert.invariantFailed(
-      "outputTypeCalc not implemented on ExpressionTypeCalculator. Call the more specialized forms directly",
-    )
-
-  override def inputTypeCalcParse(
-    state: PState,
-    context: RuntimeData,
-    x: DataValuePrimitive,
-    xType: NodeInfo.Kind,
-  ): DataValuePrimitiveNullable = {
-    val dstate = state.dState
-    val oldRepValue = dstate.repValue
-    val oldLogicalValue = dstate.logicalValue
-    dstate.repValue = x
-    dstate.logicalValue = DataValue.NoValue
-
-    val ans = Maybe(maybeInputTypeCalc.get.evaluate(state))
-
-    dstate.repValue = oldRepValue
-    dstate.logicalValue = oldLogicalValue
-    if (ans.isDefined) {
-      DataValue.unsafeFromAnyRef(ans.get)
-    } else {
-      DataValue.NoValue;
-    }
-  }
-  override def outputTypeCalcUnparse(
-    state: UState,
-    context: RuntimeData,
-    x: DataValuePrimitive,
-    xType: NodeInfo.Kind,
-  ): DataValuePrimitiveNullable = {
-    val dstate = state.dState
-    val oldRepValue = dstate.repValue
-    val oldLogicalValue = dstate.logicalValue
-    dstate.repValue = DataValue.NoValue
-    dstate.logicalValue = x
-
-    val ans = maybeOutputTypeCalc.get.evaluate(state)
-
-    dstate.repValue = oldRepValue
-    dstate.logicalValue = oldLogicalValue
-    DataValue.unsafeFromAnyRef(ans)
-  }
-
-  override def inputTypeCalcRun(
-    dstate: DState,
-    x: DataValuePrimitive,
-    xType: NodeInfo.Kind,
-  ): Unit = {
-    val oldRepValue = dstate.repValue
-    val oldLogicalValue = dstate.logicalValue
-    dstate.repValue = x
-    dstate.logicalValue = DataValue.NoValue
-
-    maybeInputTypeCalc.get.run(dstate)
-
-    dstate.repValue = oldRepValue
-    dstate.logicalValue = oldLogicalValue
-
-  }
-  override def outputTypeCalcRun(
-    dstate: DState,
-    x: DataValuePrimitive,
-    xType: NodeInfo.Kind,
-  ): Unit = {
-    val oldRepValue = dstate.repValue
-    val oldLogicalValue = dstate.logicalValue
-    dstate.repValue = DataValue.NoValue
-    dstate.logicalValue = x
-
-    maybeOutputTypeCalc.get.run(dstate)
-
-    dstate.repValue = oldRepValue
-    dstate.logicalValue = oldLogicalValue
-  }
-}
-
-class IdentifyTypeCalculator(srcType: NodeInfo.Kind) extends TypeCalculator(srcType, srcType) {
-  override def inputTypeCalc(
-    x: DataValuePrimitive,
-    xType: NodeInfo.Kind,
-  ): (DataValuePrimitiveNullable, Maybe[Error]) = (x, Maybe.Nope)
-  override def outputTypeCalc(
-    x: DataValuePrimitive,
-    xType: NodeInfo.Kind,
-  ): (DataValuePrimitiveNullable, Maybe[Error]) = (x, Maybe.Nope)
+  ): Either[Error, DataValuePrimitiveNullable] = Right(x)
 }
 
 class UnionTypeCalculator(

Review Comment:
   I suspect this might be able to go away.  I'm not understanding any feature of unions. I mean do we allow simple type unions as a repType or can one combine enums with a rep type using unions? 
   
   This change set is complex enough, so if you want to put off further investigation of this I'm good with that.
   
   And I also could be entirely mistaken and this stuff may need to be here. 



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/TypeCalculator.scala:
##########
@@ -183,9 +146,9 @@ class KeysetValueTypeCalculatorOrdered(
   override def inputTypeCalc(
     x: DataValuePrimitive,
     xType: NodeInfo.Kind,
-  ): (DataValuePrimitiveNullable, Maybe[Error]) = {
+  ): Either[Error, DataValuePrimitiveNullable] = {

Review Comment:
   Ah, so you also changed this return type from tuple to Either. That's cleaner also. 



-- 
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