-
Notifications
You must be signed in to change notification settings - Fork 50
Conversation
This comment has been minimized.
This comment has been minimized.
...tlogic/datahelix/generator/core/generation/relationships/OneToManyRelationshipProcessor.java
Show resolved
Hide resolved
...com/scottlogic/datahelix/generator/core/generation/relationships/RelationshipsProcessor.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/scottlogic/datahelix/generator/core/guice/CombinationStrategyProvider.java
Outdated
Show resolved
Hide resolved
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.
Good to see some very clear documentation on this beta feature - good job 👍
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.
Some comments and questions in place, overall:
- This seems to be a little more about "nested" data than necessarily relational data - I couldn't for example see usage of the parent record data in the child
- I've mostly focused on the "relational code" since I'm assuming the infinite streamed generation has been reviewed elsewhere
...rc/main/java/com/scottlogic/datahelix/generator/common/output/RelationalGeneratedObject.java
Show resolved
Hide resolved
|
||
import java.util.List; | ||
|
||
public interface SubGeneratedObject { |
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.
A "SubGeneratedObject" holds many generated objects?
...src/main/java/com/scottlogic/datahelix/generator/core/generation/GenerationConfigSource.java
Show resolved
Hide resolved
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
public class GeneratedRelationalData implements GeneratedObject, RelationalGeneratedObject { |
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.
Interface inheritance question: can something be a RelationalGeneratedObject without also being a GeneratedObject?
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.
In theory yes, in practice no. So the RelationalGeneratedObject interface could extend the GeneratedObject interface. As a matter of course, interface inheritance is something I generally avoid
} | ||
|
||
public void addSubObject(Relationship relationship, SubGeneratedObject subObject) { | ||
if (!subObjects.containsKey(relationship.getName())) { |
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.
Is there a reason to use the "relationship name" instead of the relationship as the key?
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.
Sub objects are keyed based on how they would be emitted rather than their java instance reference. This models the data rather than the state of the application
.../main/java/com/scottlogic/datahelix/generator/core/generation/DecisionTreeDataGenerator.java
Outdated
Show resolved
Hide resolved
import java.util.List; | ||
import java.util.stream.Stream; | ||
|
||
public class ExtentAugmentedFields implements Fields { |
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.
I think it would good to describe what these fields are - as far as I can tell they are extra pseudo-fields that are added to relational fields to control how many child records are generated. And the fields must be named "min" and "max" if they are to be manipulated in the extents section?
int numberOfObjects = getNumberOfObjectsToProduce(range.getMin(), range.getMax()); | ||
|
||
generatedObject.addSubObject(relationship, new SubGeneratedObject() { | ||
@Override |
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.
Are you creating a new object here from the interface? Is there a better way to do this?
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.
Yes creating an anonymous object that implements the interface.
There could be a concrete type to produce this data, just an alternative way of producing the data as required
return; | ||
} | ||
|
||
generatedObject.addSubObject(relationship, new SubGeneratedObject() { |
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.
Similar code above in adding this SubGeneratedObject
...com/scottlogic/datahelix/generator/core/generation/relationships/RelationshipsProcessor.java
Outdated
Show resolved
Hide resolved
@MattCline-SL
Yes. You could say that, nested objects are a means to create relational data.
The other changes are in this PR also as the refactoring was required to be able to achieve relational/nested data production |
Description
This pull request introduces the capability to produce relational data for the JSON output format.
Changes
Additional notes
This is considered to be a beta feature to prove and provide the capability of relational data construction within DataHelix. Only a few scenarios have been considered, more scenarios could require a fundamental change to the implementation and could require a change to the profile format. That said every attempt has been made to ensure the profile format is sufficiently abstract from the implementation to reduce the chance of it requiring a change.
Issue
Related to #841
Related to #1534