APEXMALHAR-2417 Adding Pojo Outer join accumulation #568

KapoorHitesh · 2017-03-01T06:48:05Z

@chinmaykolhatkar @tushargosavi please review

chinmaykolhatkar

Please make all the files evolving.

chinmaykolhatkar · 2017-03-01T06:50:51Z

library/src/main/java/org/apache/apex/malhar/lib/window/accumulation/AbstractPojoJoin.java

+ * Join Accumulation for Pojo Streams.
+ *
+ */
+public abstract class AbstractPojoJoin<InputT1, InputT2>


Please make this @evolving

All the classes except PojoInnerJoin needs to be marked as Evolving. This is not done completely yet.

chinmaykolhatkar

Please take care of comments. Otherwise, looks good.

chinmaykolhatkar · 2017-03-01T06:58:38Z

library/src/main/java/org/apache/apex/malhar/lib/window/accumulation/AbstractPojoJoin.java

+
+  public AbstractPojoJoin(Class<?> outClass, String... keys)
+  {
+    if (keys.length % 2 != 0) {


Why %2? How should user know in which orders the keys should be mentioned... Suggesting to follow either of the 2 approaches:

2 parameters to contructor with 2 different keyExpressions.

2 parameters to constructor with 2 string[].

Thank you Chinmay for this input, I will create a separate Jira under APEXMALHAR-2413 for this.

This Jira does not capture it. Moreover, please close this Jira and create a seperate Jira for performance improvement of PojoJoins..

Opened Jira https://issues.apache.org/jira/browse/APEXMALHAR-2429 for this

chinmaykolhatkar · 2017-03-01T07:00:32Z

library/src/main/java/org/apache/apex/malhar/lib/window/accumulation/AbstractPojoJoin.java

+{
+  protected final String[] keys;
+  protected final Class<?> outClass;
+  private transient List<KeyValPair<String,PojoUtils.Getter>>  gettersStream1;


Can you use Map instead of List? for all the 3 variables.

chinmaykolhatkar · 2017-03-01T07:01:13Z

library/src/main/java/org/apache/apex/malhar/lib/window/accumulation/AbstractPojoJoin.java

+  private transient List<KeyValPair<String,PojoUtils.Getter>>  gettersStream1;
+  private transient List<KeyValPair<String,PojoUtils.Getter>>  gettersStream2;
+  private transient List<KeyValPair<String,PojoUtils.Setter>>  setters;
+  public transient Set<String> keySetStream2;


Can keySetStream1 & keySetStream2 be private variables?

Actually you can make it protected..

chinmaykolhatkar · 2017-03-01T07:03:43Z

library/src/main/java/org/apache/apex/malhar/lib/window/accumulation/AbstractPojoJoin.java

+  {
+    // TODO: If a stream never sends out any tuple during one window, a wrong key would not be detected.
+
+    input.getClass().getDeclaredField(keys[index]);


Why is the purpose of this line?

chinmaykolhatkar · 2017-03-01T07:06:41Z

library/src/main/java/org/apache/apex/malhar/lib/window/accumulation/AbstractPojoJoin.java

+  public List<?> getOutput(List<List<Map<String, Object>>> accumulatedValue)
+  {
+   // List<Map<String, Object>> result = new ArrayList<>();
+    if (setters == null) {


A minor knit... Just to be modular...
if (seters == null) {
// create setters
}

if (keySetStream1 == null) {
// populate set.
}

This will call additional if condition everytime.

chinmaykolhatkar · 2017-03-01T08:11:59Z

library/src/main/java/org/apache/apex/malhar/lib/window/accumulation/PojoInnerJoin.java

 */
 public class PojoInnerJoin<InputT1, InputT2>
-    implements MergeAccumulation<InputT1, InputT2, List<List<Map<String, Object>>>, List<?>>
+    extends AbstractPojoJoin


AbstractPojoJoin<InputT1, InputT2>

Similarly for other classes as well.

chinmaykolhatkar · 2017-03-01T08:42:44Z

library/src/main/java/org/apache/apex/malhar/lib/window/accumulation/AbstractPojoJoin.java

+ *
+ */
+public abstract class AbstractPojoJoin<InputT1, InputT2>
+    implements MergeAccumulation<InputT1, InputT2, List<List<Map<String, Object>>>, List<?>>


I have a suggest here in terms of how accumulation data structure is created... Instead of having a list of map... we can have List<MultiMap<Object, Object>>.. The key to multimap is a unique object generated for set of keys configured... and value of multimap can contain more than one object for the same key.

This way, while making the comparison, it becomes straightforward how to get the matching pairs.
And I believe it'll reduce the cost of iterating over the various maps in place.

Just something to consider later on for improvsation.

Thank you Chinmay for this input, I will create a separate Jira under APEXMALHAR-2413 for this

This Jira does not capture it. Moreover, please close this Jira and create a seperate Jira for performance improvement of PojoJoins..

Opened https://issues.apache.org/jira/browse/APEXMALHAR-2430

chinmaykolhatkar · 2017-03-01T09:11:05Z

library/src/main/java/org/apache/apex/malhar/lib/window/accumulation/PojoFullOuterJoin.java

+  }
+
+  @Override
+  public Map<String, Object> joinTwoMapsWithKeys(Map map1, Map map2)


This method seems to be doing the same thing as the one present in super class. Remove this?

KapoorHitesh · 2017-03-01T10:57:22Z

@chinmaykolhatkar have made the changes please review and merge.

chinmaykolhatkar · 2017-03-02T06:18:20Z

@Hitesh-Scorpio Some changes like creation of right Jira and Making the classes Evolving not done.
Please take care of it.
Also please squash and rebase.

I'll merge once this is done.

KapoorHitesh · 2017-03-02T08:09:59Z

@chinmaykolhatkar have created the suggested Jira's have squashed the comments and rebased.

chinmaykolhatkar reviewed Mar 1, 2017

View reviewed changes

chinmaykolhatkar suggested changes Mar 1, 2017

View reviewed changes

KapoorHitesh force-pushed the APEXMALHAR-2417_NewOuterJoinAccumulation branch 2 times, most recently from 98e13a8 to a634e64 Compare March 2, 2017 07:09

APEXMALHAR-2417 Adding Pojo Outer join accumulation

4b36bf3

KapoorHitesh force-pushed the APEXMALHAR-2417_NewOuterJoinAccumulation branch from 6d9ae0a to 4b36bf3 Compare March 2, 2017 10:02

asfgit merged commit 4b36bf3 into apache:master Mar 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APEXMALHAR-2417 Adding Pojo Outer join accumulation #568

APEXMALHAR-2417 Adding Pojo Outer join accumulation #568

KapoorHitesh commented Mar 1, 2017

chinmaykolhatkar left a comment

chinmaykolhatkar Mar 1, 2017

KapoorHitesh Mar 1, 2017

chinmaykolhatkar Mar 2, 2017 •

edited

chinmaykolhatkar left a comment

chinmaykolhatkar Mar 1, 2017

KapoorHitesh Mar 1, 2017

chinmaykolhatkar Mar 2, 2017

KapoorHitesh Mar 2, 2017

chinmaykolhatkar Mar 1, 2017

KapoorHitesh Mar 1, 2017

chinmaykolhatkar Mar 1, 2017

chinmaykolhatkar Mar 1, 2017

KapoorHitesh Mar 1, 2017

chinmaykolhatkar Mar 1, 2017

KapoorHitesh Mar 1, 2017

chinmaykolhatkar Mar 1, 2017

KapoorHitesh Mar 1, 2017

chinmaykolhatkar Mar 2, 2017

chinmaykolhatkar Mar 1, 2017

chinmaykolhatkar Mar 1, 2017

KapoorHitesh Mar 1, 2017

chinmaykolhatkar Mar 1, 2017

KapoorHitesh Mar 1, 2017

chinmaykolhatkar Mar 2, 2017

KapoorHitesh Mar 2, 2017

chinmaykolhatkar Mar 1, 2017

KapoorHitesh Mar 1, 2017

KapoorHitesh commented Mar 1, 2017

chinmaykolhatkar commented Mar 2, 2017

KapoorHitesh commented Mar 2, 2017

APEXMALHAR-2417 Adding Pojo Outer join accumulation #568

APEXMALHAR-2417 Adding Pojo Outer join accumulation #568

Conversation

KapoorHitesh commented Mar 1, 2017

chinmaykolhatkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaykolhatkar Mar 2, 2017 • edited

Choose a reason for hiding this comment

chinmaykolhatkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KapoorHitesh commented Mar 1, 2017

chinmaykolhatkar commented Mar 2, 2017

KapoorHitesh commented Mar 2, 2017

chinmaykolhatkar Mar 2, 2017 •

edited