Issue #692 obv dupe changes for ftd and matcher #694

vikasgupta78 · 2023-10-18T14:16:43Z

Issue #692 obv dupe changes for ftd and matcher

sonalgoyal · 2023-10-19T00:44:08Z

common/core/src/main/java/zingg/common/core/executor/ObvDupeFilter.java

@@ -0,0 +1,247 @@
+package zingg.common.core.executor;


this class is not an executor and should be broken down into two classes at least. they can live in a separate package zingg.common.core.obviousDupes. one that takes args, dsutil and creates filters. one that applies those.

I had thought that may be we might want to create a phase out of it in future? 100% match filter kinds? If no such plan can break it down, no problem

So we are talking of two different things here.

A new possible phase for obviousDupes. If we do that, we need some kind of executor.

Breaking down the class

The first one can always be defined later using the classes in second.

Refactor to two seperate classes in commit a3d77a6 and f3480e1

sonalgoyal · 2023-10-19T00:45:19Z

common/core/src/main/java/zingg/common/core/executor/Matcher.java

@@ -126,7 +121,7 @@ public void execute() throws ZinggClientException {
 			ZFrame<D, R, C> obvDupePairs = getObvDupePairs(blocked);


this logic should move to obvious dupes. the matcher should just ask the obv dupes to add/remove dupes as necessary by passing blocks/pairs whatever

The logic is already moved , this method delegates work to the obv dupe class. The method is kept as it gets overriden in enterprise matcher for purpose of instrumentation

That is getObvDupePairs method in Matcher

then we should override the new class in enterprise and introduce im and weave into the ent matcher.

Obvious dupes logic should be encapsulated in its own package and classes.

I went back and checked the code. The following should not be part of the Matcher.
if (obvDupePairs != null) { long obvDupeCount = obvDupePairs.count(); LOG.debug("obvDupePairs count " + obvDupeCount); if (obvDupeCount > 0) { blocks = removeObvDupesFromBlocks(blocks); } }

The methods in the Matcher that you have defined can continue to call the obv dupes classes.

done in commit 70f1c33, please review

sonalgoyal · 2023-10-19T00:52:53Z

common/core/src/main/java/zingg/common/core/executor/ObvDupeFilter.java

+	 * @return
+	 */
+
+	public C getObviousDupesFilter(ZFrame<D, R, C> df1, ZFrame<D, R, C> dfToJoin, String obviousDupeString, C extraAndCond) {


if you define the obvious filter not through a string put through json, this code becomes much simpler. how about

"obviousDupes": [
"matchCondition": [{"column":"fname"},{"column":"lname"}]
"matchCondition":[{}]
]

You can simply define this object as part of the args. It gets populated automatically, no need to parse, trim etc.

Will create a JSON sample for various scenarios and get it reviewed before implementation

Done in commit c2ddce9 please review

vikasgupta78 · 2023-10-20T11:11:45Z

have completed all review comments of obv dupe filter in OSS
once you have merged than will do enterprise side of changes

sonalgoyal · 2023-10-21T00:12:53Z

common/core/src/main/java/zingg/common/core/obviousdupes/ObvDupeFilter.java

+	public static final Log LOG = LogFactory.getLog(ObvDupeFilter.class); 
+
+    protected Arguments args;	
+    protected Context<S,D,R,C,T> context;


Do we need to pass Context or can we pass DSUtil ?

also see if S and T are needed for this class?

done in commit b29758c , please review

sonalgoyal · 2023-10-21T00:15:48Z

common/core/src/main/java/zingg/common/core/obviousdupes/ObvDupeFilter.java

+		ZFrame<D,R,C> onlyIds = null;
+
+		// loop thru the values and build a filter condition		
+		for (int i = 0; i < obviousDupes.length; i++) {		


is tyhere a reason to loop through this one by one? I thought the filter would return the and, or conditions to join on completely?

We have to loop thru all the match conditions and within match conditions for every field name.

e.g "obviousDupes":[{ "matchCondition":[ { "fieldName":"fname" }, { "fieldName":"stNo" }, { "fieldName":"add1" } ]},{"matchCondition":[ { "fieldName":"ssn" }] }, { "matchCondition":[{"fieldName":"fname" }, {"fieldName":"stNo" }, {"fieldName":"lname" } ]} ],

so we loop thru to get 3 matchCondition and then within each matchCondition for each fieldName

forming something like
(fname = z_fname and stNo = z_stNo and add1 = z_add1) OR
(ssn = z_ssn) OR
(fname = z_fname and stNo = z_stNo and lname = z_lname)

yes, I get that. The point I am trying to make is can we have one single Column returned for all the ors along with the ands? Eventually the query push down is anyways going to do it in one single statement. Do we need to do the unions or can we get the resuklt using one single join?

This is query formation.

We don't actually use it exactly like this.

In case of getting obv dupes from blocked:

We use each and condition

union all the dfs got from above

do distinct
Directly doing OR was causing performance issues

For removing obv dupe from blocks
We first form a combined condition like above and than return a combined condition with NOT:
NOT (
(fname = z_fname and stNo = z_stNo and add1 = z_add1) OR
(ssn = z_ssn) OR
(fname = z_fname and stNo = z_stNo and lname = z_lname)
)

The way we make Column filter expression is independent of how we use it.

Basically we don;t use one final Column in end and we are flexible. You have asked similar question below, I hope this answers both

By keeping build of column of filter expression independent of how we use it we can use it as needed

also many of the methods are private only the public/protected can be used by outside code

common/core/src/main/java/zingg/common/core/obviousdupes/ObvDupeFilter.java

sonalgoyal · 2023-10-21T00:22:20Z

common/core/src/main/java/zingg/common/core/obviousdupes/ObvDupeUtil.java

+	 * @param obvDupes
+	 * @return
+	 */
+	public ZFrame<D,R,C> massageObvDupes(ZFrame<D,R,C> obvDupes) {


this belongs to the other class. It seems that the class names can be swapped - ObvDupeUtil can be renamed to ObviousDupeCondiiton or even ObvDupeFilter as it is only building the join/filter conditions.

Please tell the names of both and I will do it.

method moved : commit 45e9205

ObvDupeUtil is the class that applies the filters, adds and removes obv dupes
ObvDupeCondition or ObvDupeFilter is the class that reads the args vaues and creates the conditions

Renamed in commit 6db0092 , please review

…params

sonalgoyal · 2023-10-21T06:53:27Z

common/core/src/main/java/zingg/common/core/obviousdupes/ObvDupeFilter.java

+		ZFrame<D,R,C> onlyIds = null;
+
+		// loop thru the values and build a filter condition		
+		for (int i = 0; i < obviousDupes.length; i++) {		


yes, I get that. The point I am trying to make is can we have one single Column returned for all the ors along with the ands? Eventually the query push down is anyways going to do it in one single statement. Do we need to do the unions or can we get the resuklt using one single join?

common/core/src/main/java/zingg/common/core/obviousdupes/ObvDupeFilter.java

sonalgoyal · 2023-10-21T07:06:09Z

common/core/src/main/java/zingg/common/core/obviousdupes/ObvDupeUtil.java

+	 * @param obvDupes
+	 * @return
+	 */
+	public ZFrame<D,R,C> massageObvDupes(ZFrame<D,R,C> obvDupes) {


ObvDupeUtil is the class that applies the filters, adds and removes obv dupes
ObvDupeCondition or ObvDupeFilter is the class that reads the args vaues and creates the conditions

vikasgupta78 added 13 commits October 17, 2023 22:33

1st draft, refactor to separate class

b69bde8

junit 1st cut

2eaceae

null check and better junits

394398d

log level change issue zinggAI#649

232a09c

make constructor concise

8578d0a

obv dupe filter

1ada4d4

refactoring

9a3738d

refactor SparkFrame

00cb66c

refactoring obv dupe filter from ZFrame

57a7c90

cleanup

273cfd2

message if all sample is obv dupe

36f68c3

merging latest from 0.4.0

f7a9431

merge from 0.4.0 , resolved conflict

b53e876

sonalgoyal requested changes Oct 19, 2023

View reviewed changes

vikasgupta78 added 8 commits October 19, 2023 10:17

review comments from PR zinggAI#694

70f1c33

junits improvement

5936ace

Use JSON instead of String for obv dupe condition

c2ddce9

test for obv dupe cond in args

ca66e6e

refactor package

8603114

refactor

52ff3ce

refactor

f3480e1

refactor

a3d77a6

vikasgupta78 added 5 commits October 20, 2023 22:00

refactor , make more readable/modular

a8815db

renamed

38e4fb7

renamed

57a9fd7

refactor : separate out test cases for filter and util

0af67f4

null check

2bf6297

sonalgoyal requested changes Oct 21, 2023

View reviewed changes

changed from passing context to dsutil and changed number of generic …

b29758c

…params

moved massageObvDupes method

45e9205

sonalgoyal requested changes Oct 21, 2023

View reviewed changes

vikasgupta78 added 5 commits October 21, 2023 13:40

renamed

d51d92a

renamed junits

f94b557

renamed obv dupe classes

6db0092

removed redundant null check

eb6b42f

comment updated

7d55b2e

sonalgoyal merged commit 7cfb69d into zinggAI:0.4.0 Oct 22, 2023

vikasgupta78 mentioned this pull request Oct 22, 2023

ObvDupe filter for ftd #692

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #692 obv dupe changes for ftd and matcher #694

Issue #692 obv dupe changes for ftd and matcher #694

vikasgupta78 commented Oct 18, 2023

sonalgoyal Oct 19, 2023

vikasgupta78 Oct 19, 2023

sonalgoyal Oct 19, 2023

vikasgupta78 Oct 20, 2023

sonalgoyal Oct 19, 2023

vikasgupta78 Oct 19, 2023

vikasgupta78 Oct 19, 2023

sonalgoyal Oct 19, 2023

sonalgoyal Oct 19, 2023

vikasgupta78 Oct 19, 2023

sonalgoyal Oct 19, 2023

vikasgupta78 Oct 19, 2023

vikasgupta78 Oct 20, 2023

vikasgupta78 commented Oct 20, 2023

sonalgoyal Oct 21, 2023

sonalgoyal Oct 21, 2023

vikasgupta78 Oct 21, 2023

sonalgoyal Oct 21, 2023

vikasgupta78 Oct 21, 2023

sonalgoyal Oct 21, 2023

vikasgupta78 Oct 21, 2023

vikasgupta78 Oct 21, 2023

vikasgupta78 Oct 21, 2023

vikasgupta78 Oct 21, 2023

sonalgoyal Oct 21, 2023

vikasgupta78 Oct 21, 2023

vikasgupta78 Oct 21, 2023

sonalgoyal Oct 21, 2023

vikasgupta78 Oct 21, 2023

sonalgoyal Oct 21, 2023

sonalgoyal Oct 21, 2023

		@@ -126,7 +121,7 @@ public void execute() throws ZinggClientException {
		ZFrame<D, R, C> obvDupePairs = getObvDupePairs(blocked);

Issue #692 obv dupe changes for ftd and matcher #694

Issue #692 obv dupe changes for ftd and matcher #694

Conversation

vikasgupta78 commented Oct 18, 2023

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

vikasgupta78 commented Oct 20, 2023

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