Add signature computation support for arbitrary logical plans #77

pirz · 2020-06-30T22:51:16Z

What changes were proposed in this pull request?

Currently, Hyperspace has one signature provider: FileBasedSignatureProvider, which only covers LogicalRelation nodes in a plan to compute the plan signature.
This limits the DataFrames that could potentially be indexed to only those whose plans consist of only logical relation nodes.
Moving forward, Hyperspace needs to support generating signature for DataFrames with arbitrary logical plans.

Below changes address issue #76
1- Adds new signature provider which computes a signature for arbitrary logical plans.
2- Changes existing Hyperspace rules to comply with changes in the signature provider.
3- Changes the API for signature() in Signature provider to return an Option (instead of a default value) to avoid potential issues due to using a default signature.
4- It adds test cases for the new signature provider and refactors existing test code.

Below change addresses Issue #31
5- It refactors the test code in JoinIndex and FilterIndex rules tests.

Why are the changes needed?

Supporting signature computation for arbitrary logical plans.

Refactoring and cleaning up test code.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Test cases are added under new files: IndexSignatureProviderTest and RulesHelperTest.
Backward combability tested manually. A sample application was used to generate indexes with current master (0.1.0 code without this change) and new application code using "current master + this change" used to read and leverage those indexes. The output was verified manually to confirm the indexes created with an older version of bits are still usable with new bits including this change.

apoorvedave1 · 2020-07-02T22:22:11Z

src/main/scala/com/microsoft/hyperspace/index/PlanSignatureProvider.scala

+    var signature = ""
+    logicalPlan.foreachUp(o => signature += HashingUtils.md5Hex(o.nodeName))


could you please add a final HashingUtils.md5Hex(signature) on the overall signature? (to keep it of same length as FileBasedSignatureProvider and IndexSignatureProvider

also p instead of o as in "plan"?

@imback82 o is actually added for "Operator" because for each operator we add hash of its node name to the signature.

I didn't think "operator" when I saw o since technically it's logicalPlan.foreachUp(p: LogicalPlan => ...). But it's nit anyway.

Got it, I will change it in the next push.

apoorvedave1 · 2020-07-02T23:38:31Z

src/main/scala/com/microsoft/hyperspace/index/rules/RulesHelper.scala

+    logicalPlan match {
+      case r: LogicalRelation => Some(r)
+      case p: LogicalPlan =>
+        if (p.children.size == 1) {
+          getLogicalRelation(p.children.head)
+        } else {
+          None // plan is non-linear or it does not have any LogicalRelation
+        }
+    }


can simplify to

val lrs = logicalPlan.collect { case r: LogicalRelation => r } if (lrs.count == 1) Some(lrs.head) else None

or something like

logicalPlan.collectLeaves match { case r : LogicalRelation :: Nil => r case _ => None }

Thnx, It is changed and this code is now in RuleUtils.scala.

apoorvedave1 · 2020-07-02T23:48:09Z

src/test/scala/com/microsoft/hyperspace/index/IndexSignatureProviderTest.scala

+  private def createSimplePlan(
+                                length: Long,
+                                modificationTime: Long,
+                                path: Path): LogicalRelation = {


Fixed, thnx.

apoorvedave1 · 2020-07-03T03:09:32Z

src/main/scala/com/microsoft/hyperspace/index/rules/FilterIndexRule.scala

+    RulesHelper.getLogicalRelation(project) match {
+      case Some(r) =>


Could you please explain the reason for this change? To me, this looks like hard-coding signature calculation logic to a signature provider which works only on LogicalRelation. I think here we should keep it relaxed to any "plan" node and not just logical relation. I also don't think it saves on calculation so some explanation could help. Thanks.

This is not done for saving calculations, but it is needed for correctness.
Assume we have a query plan as:
QueryPlan (P): Scan(R: a, b, c) -> Filter(a>5) -> Project(b)
and we have two indexes defined as:
Covering Index 1 (IX1): R(a, b)
Index 2 (IX2): Scan(a, b, c) -> Filter(a>100) -> Project(b, c)
As the LogicalRelation node is the same for all 3 plans, FileBasedSignatureProvider generates the same signature for all three of them.
IndexSignatureProvider (introduced by this change) includes node names in signature calculation and as a result it generate the same signature for P and IX2 and a different signature for IX1.

In FilterIndex and JoinIndex rules, a candidate index has to meet below two conditions:

Index has to be defined on the same LogicalRelation as the one query uses,

Index has to cover all records (i.e. index plan should not have a Filter, Join, Agg or any other node that removes or modifies original relation's records).

As a result, when we look for indexes in FilterIndex and JoinIndex rules we need to: "Extract a single logical relation node from original query plan and compare the signature of that subplan with the signature of covering indexes".
This way the signature comparison will satisfy both conditions mentioned above for these rules and only returns covering indexes that include all records of original relation.
In the above example, this procedure of extracting logical relation from query plan and comparing its signature with index signatures (which are calculated and stored using IndexSignatureProvider) results in only marking IX1 as a candidate index.

Please let me know if above explanation makes sense; we can discuss it further if you prefer.

apoorvedave1 · 2020-07-03T03:12:07Z

src/main/scala/com/microsoft/hyperspace/index/rules/JoinIndexRule.scala

+    RulesHelper.getLogicalRelation(plan) match {
+      case Some(r) =>


same comment as in FilterIndexRule, copy pasting here.

Could you please explain the reason for this change? To me, this looks like hard-coding signature calculation logic to a signature provider which works only on LogicalRelation. I think here we should keep it relaxed to any "plan" node and not just logical relation. I also don't think it saves on calculation so some explanation could help. Thanks.

I added an explanation to the previous comment in FilterIndex rule. Please check that and let me know if further discussion is required. Thnx

apoorvedave1

Thanks @pirz, left some comments.

imback82

few minor comments, but generally looking good to me. @pirz please let us know after you test against 0.1.0. thanks.

src/main/scala/com/microsoft/hyperspace/index/IndexSignatureProvider.scala

imback82 · 2020-07-22T22:52:58Z

src/main/scala/com/microsoft/hyperspace/index/rules/FilterIndexRule.scala

+            fsRelation.fileFormat)
+        }
+
+      case None =>


nit:

// There is zero or more than one LogicalRelation nodes in Filter's subplan. case None => Nil

imback82 · 2020-07-22T23:00:48Z

src/main/scala/com/microsoft/hyperspace/index/rules/RuleUtils.scala

+          .signature(plan) match {
+          case Some(s) =>
+            signatureMap.put(sourcePlanSignature.provider, s)
+          case None => return false


I think we should still put the value if the signature generation is already tried. You probably need to change the map to val signatureMap = mutable.Map[String, Option[String]]().

Makes sense, changed it. Thnx

pirz · 2020-07-23T00:04:48Z

@imback82 I verified backward compatibility and updated PR description to reflect that with details.

imback82 · 2020-07-23T00:06:43Z

@imback82 I verified backward compatibility and updated PR description to reflect that with details.

Thanks!

imback82 · 2020-07-23T00:17:24Z

src/main/scala/com/microsoft/hyperspace/index/rules/RuleUtils.scala

@@ -47,7 +48,10 @@ object RuleUtils {
          .signature(plan)
        signatureMap.put(sourcePlanSignature.provider, signature)


can we use getOrElseUpdate to remove double look up?

nice, added it.

imback82 · 2020-07-23T00:23:42Z

src/test/scala/com/microsoft/hyperspace/index/IndexManagerTests.scala

+                LogicalPlanSignatureProvider.create().name,
+                s)))))
+        val sourceFiles = df.queryExecution.optimizedPlan.collect {
+          case LogicalRelation(
          HadoopFsRelation(location: PartitioningAwareFileIndex, _, _, _, _, _),


indentation looks off? is this from auto-format?

imback82 · 2020-07-23T00:23:53Z

src/test/scala/com/microsoft/hyperspace/index/IndexManagerTests.scala

+          Map())
+        entry.state = state
+        entry
+      case None => throw HyperspaceException("Invalid plan for index dataFrame.")


should we just do assert? I mean fail

imback82 · 2020-07-23T00:25:21Z

src/test/scala/com/microsoft/hyperspace/index/IndexSignatureProviderTest.scala

+  private def createIndexSignature(logicalPlan: LogicalPlan): String =
+    new IndexSignatureProvider().signature(logicalPlan) match {
+      case Some(s) => s
+      case None => throw HyperspaceException("Invalid plan for signature generation.")


just fail here? unless you are testing the scenario where exception is thrown

imback82 · 2020-07-23T00:39:16Z

src/test/scala/com/microsoft/hyperspace/index/IndexSignatureProviderTest.scala

@@ -130,7 +130,7 @@ class IndexSignatureProviderTest extends SparkFunSuite with SparkInvolvedSuite {
  private def createIndexSignature(logicalPlan: LogicalPlan): String =
    new IndexSignatureProvider().signature(logicalPlan) match {
      case Some(s) => s
-      case None => throw HyperspaceException("Invalid plan for signature generation.")
+      case None => fail // Invalid plan for signature generation.


I think you can do fail("Invalid plan for signature generation")

imback82

LGTM, thanks @pirz!

imback82 · 2020-07-23T01:03:52Z

@apoorvedave1 Any other comments?

apoorvedave1 · 2020-07-23T01:24:18Z

src/main/scala/com/microsoft/hyperspace/index/PlanSignatureProvider.scala

+   */
+  def signature(logicalPlan: LogicalPlan): Option[String] = {
+    var signature = ""
+    logicalPlan.foreachUp(p => signature += HashingUtils.md5Hex(p.nodeName))


nit: we can avoid this calculation by just doing md5Hex in the last step in match-case statement. It would be faster and still correct. Right?

cc @imback82

This is an approach that we take in FileBasedSignatureProvider as well and it makes the final value non-sensitive to the order of strings (node names in this case or files in case of FileBasedSignatureProvider). This is important as for example for a plan with a join node the signature should be independent of order of visiting join children. Otherwise the signature computation would not be reproducible if the order of visiting left and right children swap.
Visiting plan as Join,LeftChild,RightChild should produce the same signature as visiting the plan as Join,RightChild,LeftChild. Calculating Hash value per node name and summing them up makes this achievable but concatenating node names and calling hash() on final String breaks this.

@imback82 @apoorvedave1 Can you please check above comment and merge this PR if you are fine with it?

Visiting plan as Join,LeftChild,RightChild should produce the same signature as visiting the plan as Join,RightChild,LeftChild. Calculating Hash value per node name and summing them up makes this achievable but concatenating node names and calling hash() on final String breaks this.

@pirz are you sure about this? hash(a) + hash(b) is the same as hash(b) + hash(a) when hash returns a string?

@apoorvedave1 I am not too sure about the performance improvement here (<100 nodes) and would depend on how we append the strings. I would say it would be negligible. Do you have some numbers to back up?

OK, can we do this instead?

var signature = "" logicalPlan.foreachUp(p => signature = HashingUtils.md5Hex(signature + p.nodeName)) signature match { case "" => None case _ => signature }

We will always bound the string size and also consistent with file based signature.

Since this PR is getting long, I will merge this now. @pirz Can you do a follow up PR to address @apoorvedave1's concern? (if we are going with one md5hex, let's use StringBuffer).

Makes sense, I updated it. @imback82 and @apoorvedave1 Can you plz check/approve again?

approved. Thanks!

@imback82 Sure, Thanks. I guess we first need some numbers to see if calling Hash multiple times on the plans (which are normally <100 nodes as you said) is a perf bottleneck or not. If that turned out to be an issue, we will switch.

apoorvedave1

one minor comment (which could slightly improve performance) but otherwise LGTM. (Also it would be breaking change if we do it in future.)

apoorvedave1

LGTM, Thanks @pirz

imback82

LGTM, thanks @pirz!

pirz and others added 16 commits June 23, 2020 17:04

Update _config.yml

767abd4

Update 07-dg-code-structure.md

d004f0e

Update _config.yml

bac87fb

Update 07-dg-code-structure.md

8065717

Update 07-dg-code-structure.md

3cf094c

Merge branch 'master' into master

2878020

Update 07-dg-code-structure.md

1b03e4a

Merge branch 'master' into master

691f36a

Merge branch 'master' into master

de1bc9c

Update 07-dg-code-structure.md

f311575

Merge branch 'master' into master

08c9d6d

Merge branch 'master' of https://github.com/pirz/hyperspace-1

df8c26e

Merge remote-tracking branch 'upstream/master'

05a5cba

Merge remote-tracking branch 'upstream/master'

910974f

add index signature code and add/cleanup tests

3b6ad0e

small test code refactor

51ba9b2

pirz requested review from imback82, apoorvedave1, AFFogarty, rapoth and thrajput June 30, 2020 22:52

pirz self-assigned this Jun 30, 2020

pirz added the enhancement New feature or request label Jun 30, 2020

apoorvedave1 reviewed Jul 2, 2020

View reviewed changes

apoorvedave1 reviewed Jul 3, 2020

View reviewed changes

apoorvedave1 suggested changes Jul 3, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master'

af5bc94

Code changes in index signature and tests

474419c

imback82 reviewed Jul 22, 2020

View reviewed changes

small refactoring in RuleUtils and FilterIndex rule code

e630c2f

imback82 reviewed Jul 23, 2020

View reviewed changes

Pouria Pirzadeh added 2 commits July 22, 2020 17:34

minor code changes in RuleUtils and tests

cb0d1b9

removed unnecessary import

b42ff02

imback82 reviewed Jul 23, 2020

View reviewed changes

minor change in test code

408c968

imback82 previously approved these changes Jul 23, 2020

View reviewed changes

apoorvedave1 reviewed Jul 23, 2020

View reviewed changes

apoorvedave1 suggested changes Jul 23, 2020

View reviewed changes

Pouria Pirzadeh added 2 commits July 23, 2020 09:12

Merge remote-tracking branch 'upstream/master'

58666c1

Merge branch 'master' into pouriap/ixdf-p2

323dd9b

apoorvedave1 previously approved these changes Jul 23, 2020

View reviewed changes

slight change in plan signature computation

a188b52

pirz dismissed stale reviews from apoorvedave1 and imback82 via a188b52 July 23, 2020 18:54

apoorvedave1 approved these changes Jul 23, 2020

View reviewed changes

imback82 approved these changes Jul 23, 2020

View reviewed changes

imback82 merged commit 48b3332 into microsoft:master Jul 23, 2020

pirz mentioned this pull request Jul 23, 2020

Add signature computation support for arbitrary logical plans #76

Closed

rapoth linked an issue Jul 24, 2020 that may be closed by this pull request

Add signature computation support for arbitrary logical plans #76

Closed

imback82 mentioned this pull request Jul 27, 2020

Revisit logical plan serialization #95

Open

sezruby mentioned this pull request Mar 30, 2021

Remove Test duplication, e.g. creation of test object IndexLogEntry is highly duplicated #31

Closed

		var signature = ""
		logicalPlan.foreachUp(o => signature += HashingUtils.md5Hex(o.nodeName))

		RulesHelper.getLogicalRelation(project) match {
		case Some(r) =>

		@@ -47,7 +48,10 @@ object RuleUtils {
		.signature(plan)
		signatureMap.put(sourcePlanSignature.provider, signature)

Add signature computation support for arbitrary logical plans #77

Add signature computation support for arbitrary logical plans #77

Conversation

pirz commented Jun 30, 2020 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

apoorvedave1 Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirz Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoorvedave1 Jul 2, 2020 • edited Loading

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

apoorvedave1 Jul 3, 2020 • edited Loading

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

apoorvedave1 left a comment

Choose a reason for hiding this comment

imback82 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

pirz commented Jul 23, 2020

imback82 commented Jul 23, 2020

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

imback82 Jul 23, 2020 • edited Loading

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

imback82 left a comment

Choose a reason for hiding this comment

imback82 commented Jul 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirz Jul 23, 2020 • edited Loading

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

apoorvedave1 left a comment • edited Loading

Choose a reason for hiding this comment

apoorvedave1 left a comment

Choose a reason for hiding this comment

imback82 left a comment

Choose a reason for hiding this comment

pirz commented Jun 30, 2020 •

edited

Loading

apoorvedave1 Jul 2, 2020 •

edited

Loading

pirz Jul 22, 2020 •

edited

Loading

apoorvedave1 Jul 2, 2020 •

edited

Loading

apoorvedave1 Jul 3, 2020 •

edited

Loading

imback82 Jul 23, 2020 •

edited

Loading

pirz Jul 23, 2020 •

edited

Loading

apoorvedave1 left a comment •

edited

Loading