Modify getCandidateIndex for hybrid scan #153

sezruby · 2020-09-11T08:39:35Z

What changes were proposed in this pull request?

lazy val allSourceFileInfos: Set[FileInfo]
- lazy val because this source file list of an index referred several times when applying Rules.
- Set instead of Seq because
  - comparison of 2 Set[FileInfo] is the only use case of allSourceFileInfos now.
  - performance issue
isHybridScanCandidate()
1. get the current file list of the given relation
2. check if there's any common files with index.allSourceFileInfos
- TODO: plan signature comparison Add plan signature validation for Hybrid Scan #158
- TODO: number of common files(or size) threshold [FEATURE REQUEST]: Introduce a similarity threshold for Hybrid Scan #159

Why are the changes needed?

The current signature design is not appropriate for hybrid scan.
For delete support, we need to figure out the list of deleted source files and check the other source files still have the same metadata when index build time or not.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Unit test

- only applicable without threshold

rapoth · 2020-09-11T23:09:05Z

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

+      files.exists(entry.allSourceFileInfos.contains)
+    }
+
    // TODO: the following check only considers indexes in ACTIVE state for usage. Update


I remember there is already an issue for this. If so, can we just link to the issue containing the description. @apoorvedave1 to confirm if there is an issue.

It seems #65; I'll add the link here.

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

imback82

few minor comments, but looking fine to me.

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

imback82 · 2020-09-14T20:35:08Z

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

+  def getCandidateIndexes(
+      indexManager: IndexManager,
+      plan: LogicalPlan,
+      hybridScanEnabled: Boolean = false): Seq[IndexLogEntry] = {


nit: can we remove default value? Rationale: this is an user-facing config, which is set to false by default. If we later change the default to true, we need to remember to update this to true as well.

It's temporary for this PR - I'll remove the default value assignment & use the config value instead in the next pr.

OK. In that case, please leave a review comment on your intention. That helps reviewers.

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

imback82 · 2020-09-14T20:43:00Z

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

-  def getCandidateIndexes(indexManager: IndexManager, plan: LogicalPlan): Seq[IndexLogEntry] = {
+  def getCandidateIndexes(
+      indexManager: IndexManager,
+      plan: LogicalPlan,


Currently this plan is always the result of RuleUtils.getLogicalRelation meaning this plan is expected to be in a fixed shape (linear, etc.). What would be the best way to address this? cc: @apoorvedave1 / @pirz

cc @rapoth I modified #160 a bit for this, but I think we need a new uber-issue for "arbitrary source plan" though it will be handled later.

Good idea! Can you also open an uber issue for that when you get a chance please? (even if it is a work-in-progress issue, let's just capture all the work as we know so far)

Ok I'll 👍

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

imback82 · 2020-09-14T20:48:10Z

src/test/scala/com/microsoft/hyperspace/index/rules/HyperspaceRuleTestSuite.scala

+              Hdfs(Properties(Content(Directory("/")))),
+              "schema",
+              "format",
+              Map())),


Suggestion: #142 (comment)

This is required - empty root directory throws an exception during the test.

src/test/scala/com/microsoft/hyperspace/index/rules/RuleUtilsTest.scala

…date

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

sezruby · 2020-09-15T01:16:50Z

@apoorvedave1 @pirz Could you review this PR? Thanks!

src/test/scala/com/microsoft/hyperspace/index/rules/RuleUtilsTest.scala

imback82

LGTM, thanks @sezruby!

imback82 · 2020-09-15T02:38:55Z

Merged to master. @apoorvedave1 / @pirz Feel free to leave any comments if I missed something. Also, please take a look at #153 (comment). I think we need to follow up on it.

rapoth · 2020-09-15T02:41:09Z

Thanks @sezruby! Can you create a follow-up item for #153 (comment) (and put in the necessary documentation) if it does not already exist?

sezruby added 2 commits September 11, 2020 17:19

Implement getCandidateIndex for hybrid scan

e7fd0bb

Replace intersect.nonEmpty with exists.contains

11aaee6

- only applicable without threshold

sezruby mentioned this pull request Sep 11, 2020

Hybrid Scan for File/Partition Mutable Datasets #150

Closed

7 tasks

sezruby changed the title ~~Implement getCandidateIndex for hybrid scan~~ Modify getCandidateIndex for hybrid scan Sep 11, 2020

rapoth requested review from apoorvedave1 and pirz and removed request for apoorvedave1 September 11, 2020 22:48

rapoth reviewed Sep 11, 2020

View reviewed changes

pirz reviewed Sep 11, 2020

View reviewed changes

src/main/scala/com/microsoft/hyperspace/index/rules/RuleUtils.scala Outdated Show resolved Hide resolved

sezruby mentioned this pull request Sep 14, 2020

Add plan signature validation for Hybrid Scan #158

Open

sezruby added 2 commits September 14, 2020 11:36

Add todo issue links

a519f16

minor fix

cdc5c2d

imback82 reviewed Sep 14, 2020

View reviewed changes

sezruby added 2 commits September 15, 2020 09:06

Review commit

cecacd7

Merge remote-tracking branch 'upstream/master' into hybridscan_3candi…

b2b9605

…date

sezruby requested a review from imback82 September 15, 2020 00:44

imback82 reviewed Sep 15, 2020

View reviewed changes

src/main/scala/com/microsoft/hyperspace/index/rules/RuleUtils.scala Outdated Show resolved Hide resolved

Review commit2

667ce55

imback82 reviewed Sep 15, 2020

View reviewed changes

src/test/scala/com/microsoft/hyperspace/index/rules/RuleUtilsTest.scala Outdated Show resolved Hide resolved

imback82 reviewed Sep 15, 2020

View reviewed changes

src/test/scala/com/microsoft/hyperspace/index/rules/RuleUtilsTest.scala Outdated Show resolved Hide resolved

Review commit3

ea81d3c

imback82 reviewed Sep 15, 2020

View reviewed changes

src/test/scala/com/microsoft/hyperspace/index/rules/RuleUtilsTest.scala Outdated Show resolved Hide resolved

Review commit4

a7f2c3f

imback82 approved these changes Sep 15, 2020

View reviewed changes

imback82 merged commit b7e70af into microsoft:master Sep 15, 2020

sezruby deleted the hybridscan_3candidate branch September 17, 2020 13:29

Modify getCandidateIndex for hybrid scan #153

Modify getCandidateIndex for hybrid scan #153

Uh oh!

Conversation

sezruby commented Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

imback82 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sezruby Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sezruby commented Sep 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

imback82 left a comment

Choose a reason for hiding this comment

Uh oh!

imback82 commented Sep 15, 2020

Uh oh!

rapoth commented Sep 15, 2020

Uh oh!

Uh oh!

sezruby commented Sep 11, 2020 •

edited

Loading

sezruby Sep 15, 2020 •

edited

Loading