#105: Refactoring Measurement, from Agent side #104
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| /* | ||
| * Copyright 2021 ABSA Group Limited | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package za.co.absa.atum.agent.model | ||
|
|
||
| import za.co.absa.atum.agent.exception.MeasurementException | ||
| import za.co.absa.atum.model.dto.MeasureResultDTO.ResultValueType | ||
|
|
||
| trait MeasureResult { | ||
| val resultValue: Any | ||
|
There was a problem hiding this comment. Isn't it better to use generic There was a problem hiding this comment. Yes I'm absolutely aware of that. The thing is that if this would be of a generic type, then this trait would need to be of a generic type as well, and every usages of it would need to take that into an account. I gave it a try (1-2 hours) twice in the past and I didn't like the solution at all. I even wanted to include There was a problem hiding this comment. @TebaleloS @benedeki There was a problem hiding this comment. I really don't like to use trait MeasureResult {
type ResultType,
val resultValue: ResultType
}There was a problem hiding this comment. @lsulak There was a problem hiding this comment. trait MeasureResult[T] {
val resultValue: T
val resultValueType: ResultValueType.ResultValueType
}basically this ^ would mean, that I would need to adjust code in I really like David's solution though, the whole 'generic' problem is nicely 'encapsulated' within the whole module There was a problem hiding this comment. I think you are actually right about this one. Anyway I have one suggestion, to replace This approach provides more flexibility because you can add methods and fields to the case classes if needed. It also provides more type safety because the compiler can check that you've handled all possible cases when you use pattern matching. sealed trait ResultValueType
case class StringResult(value: String) extends ResultValueType
case class LongResult(value: Long) extends ResultValueType
case class BigDecimalResult(value: BigDecimal) extends ResultValueType
case class DoubleResult(value: Double) extends ResultValueType
trait MeasureResult {
val resultValue: ResultValueType
}There was a problem hiding this comment. I don't know. I know that this is a common Scala pattern, but the original solution with the Enumerate is extremely small, really easy to refactor and extend if needed: object ResultValueType extends Enumeration {
type ResultValueType = Value
val String, Long, BigDecimal, Double = ValueThere was a problem hiding this comment. I leave it up to you to consider these options and decide. My personal preference would be the idiomatic solution with sealed trait and case classes. The reasoning was mentioned above, plus you wouldn't have to have a separate field just to keep track of the type. |
||
| val resultType: ResultValueType.ResultValueType | ||
| } | ||
|
|
||
| object MeasureResult { | ||
| private final case class MeasureResultWithType[T](resultValue: T, resultType: ResultValueType.ResultValueType) | ||
| extends MeasureResult | ||
|
|
||
| /** | ||
| * When the Atum Agent itself performs the measurements, using Spark, then in some cases some adjustments are | ||
| * needed - thus we are converting the results to strings always - but we need to keep the information about | ||
| * the actual type as well. | ||
| * | ||
| * These adjustments are needed to be performed - to avoid some floating point issues | ||
| * (overflows, consistent representation of numbers - whether they are coming from Java or Scala world, and more). | ||
| */ | ||
| def apply(resultValue: String, resultType: ResultValueType.ResultValueType): MeasureResult = { | ||
| MeasureResultWithType[String](resultValue, resultType) | ||
| } | ||
|
|
||
| /** | ||
| * When the application/user of Atum Agent provides actual results by himself, the type is precise and we don't need | ||
| * to do any adjustments. | ||
| */ | ||
| def apply(resultValue: Any): MeasureResult = { | ||
| resultValue match { | ||
|
|
||
| case l: Long => | ||
| MeasureResultWithType[Long](l, ResultValueType.Long) | ||
| case d: Double => | ||
| MeasureResultWithType[Double](d, ResultValueType.Double) | ||
| case bd: BigDecimal => | ||
| MeasureResultWithType[BigDecimal](bd, ResultValueType.BigDecimal) | ||
| case s: String => | ||
| MeasureResultWithType[String](s, ResultValueType.String) | ||
|
|
||
| case unsupportedType => | ||
| val className = unsupportedType.getClass.getSimpleName | ||
| throw MeasurementException( | ||
| s"Unsupported type of measurement: $className for provided result: $resultValue") | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,65 +16,4 @@ | |
|
|
||
| package za.co.absa.atum.agent.model | ||
|
|
||
| final case class Measurement(measure: Measure, result: MeasureResult) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIny: (do only in any reasonable change is needed too)
Add a TODO message referencing the optimization ticket that would group the execution into one place. I think this is te right starting point. #98