rdblue commented on a change in pull request #828:
URL: https://github.com/apache/iceberg/pull/828#discussion_r434851651
##########
File path: spark/src/test/java/org/apache/iceberg/spark/data/RandomData.java
##########
@@ -442,4 +474,226 @@ private static BigInteger randomUnscaled(int precision,
Random random) {
return new BigInteger(sb.toString());
}
+
+ private static class DictionaryEncodedDataGenerator extends
RandomDataGenerator {
Review comment:
I think the way that this extends the base class is awkward. It
overrides primitive, but then needs to call `getPrimitive`. What `getPrimitive`
is doing is not obvious in the child class, and the `Random` variable needs to
be directly accessible causing the need to override checkstyle.
Instead, the base class should add a method to generate a value for a
primitive, like `randomValue` and pass a `Random` into it. Then `random` can
stay `private` and we don't need to override checkstyle. Also, the conversion
only needs to happen in one place, the implementation of `primitive`.
```java
@Override
public Object primitive(Type.PrimitiveType primitive) {
Object result = randomValue(primitive, random);
// For the primitives that Avro needs a different type than Spark, fix
// them here.
switch (primitive.typeId()) {
...
}
}
protected Object randomValue(Type.PrimitiveType primitive, Random rand) {
return generatePrimitive(primitive, rand);
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
