Merge append and delete actions on indexes on modified source data #187

apoorvedave1 · 2020-10-07T22:58:43Z

NOTES TO REVIEWERS:

Index metadata changes from another PR Update index log entry for enforce delete during read time #170 by @pirz have been handpicked without which correctness cannot be guaranteed for this PR.
Changes include:

1. add "appended" list to index metadata
1. add "deleted" list to index metadata
  Reviewers are NOT required to review PR Update index log entry for enforce delete during read time #170 to review this PR.

What is the context for this pull request?

Hyperspace currently handles append support and delete support on modifiable source data separately. Users can either allow deletion of some source files and refresh their index, or they can allow adding files to data and refresh the index.

This PR merges these operations which will enable users to have a fully modifyable data source. Users can delete some of the indexed source files and also append some new source files. A refreshIndex(indexName, mode = "incremental") operation will allow users to update their indexes to accommodate for all these changes.

Tracking Issue: Merge incremental mode append and delete actions into index refresh #149 , also Fixes RefreshDelete should update appended files list in metadata #179, RefreshAppend should update appended/deleted files list in metadata #183
Parent Issue: Incremental Index Maintenance for File/Partition Mutable Datasets #136
Dependencies: NA

What changes were proposed in this pull request?

1

Merge the behavior of RefreshDeleteAction and RefreshAppendAction in a single user facing api refreshIndex(indexName, mode = "incremental"). When the user calls this api, hyperspace should update the index by removing records from all the deleted source files, and adding records from the newly added source files.

2 PR #188 MERGED

IndexLogEntry will support storing "appended" and "deleted" files (these changes were picked from #170 for correctness).

3 PR #191 MERGED

Update RefreshDeleteAction and RefreshAppendAction to also update the "appended" and "deleted" files list in index metadata.

4 PR #189 MERGED

If no updates to data source are found, it will NOT throw an exception anymore. This means:

If no deleted files are found, log a warning message and exit the RefreshDeleteAction without throwing exception.
If no appended files are found, log a warning message and exit the RefreshAppendAction without throwing exception.

5 PR #194 MERGED

Bug fix: if 'appended' or 'deleted' files are present in index metadata and hybrid scan is disabled, do NOT use those indexes. Otherwise this will lead to incorrect results.

Does this PR introduce any user-facing change?

Yes.

This PR introduces refreshIndex(indexName, mode = "incremental") for the users. Users can choose this over mode="full" for a faster refresh of index files.
index metadata files now contain "appended" and "deleted" list of files from original source data. These files are those for which index has not been updated.

How was this patch tested?

added unit tests where we delete some files from original indexed data, followed by appending new data to original data. A refresh(indexName, mode = "incremental") will update the indexes such that newly created index files will reflect the most recent version of the data.

…freshAppend

Co-authored-by: Rahul Potharaju <rapoth@microsoft.com>

RefreshIncremental.scala is for the same purpose.

# Conflicts: # src/main/scala/com/microsoft/hyperspace/index/IndexLogEntry.scala # src/test/scala/com/microsoft/hyperspace/index/IndexLogEntryTest.scala

# Conflicts: # src/main/scala/com/microsoft/hyperspace/actions/RefreshAction.scala # src/main/scala/com/microsoft/hyperspace/actions/RefreshActionBase.scala # src/main/scala/com/microsoft/hyperspace/actions/RefreshDeleteAction.scala # src/main/scala/com/microsoft/hyperspace/index/IndexCollectionManager.scala # src/main/scala/com/microsoft/hyperspace/index/IndexConstants.scala # src/test/scala/com/microsoft/hyperspace/index/E2EHyperspaceRulesTests.scala

…s.scala Co-authored-by: Rahul Potharaju <rapoth@microsoft.com>

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

sezruby

Please don't forget to handle this in 0.4: #187 (comment)

src/main/scala/com/microsoft/hyperspace/Hyperspace.scala

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

apoorvedave1 · 2020-10-10T16:52:30Z

Please don't forget to handle this in 0.4: #187 (comment)

Thanks for the reminders. I prefer not to take this in this PR

apoorvedave1 · 2020-10-10T16:57:36Z

Please don't forget to handle this in 0.4: #187 (comment)

Thanks for the reminders. I prefer not to take this in this PR

~~Could you please create an issue?~~
Nvm. I just realized this is already known. I created #196 for tracking and added lineage requirement to the issue but please check:
https://github.com/microsoft/hyperspace/blob/master/src/main/scala/com/microsoft/hyperspace/actions/RefreshAction.scala#L40-L42
and
https://github.com/microsoft/hyperspace/blob/master/src/main/scala/com/microsoft/hyperspace/actions/RefreshAppendAction.scala#L44-L48

apoorvedave1 · 2020-10-10T19:54:29Z

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

  }

-  test("Verify Join Index Rule utilizes indexes correctly after incremental refresh.") {
+  test("Verify JoinIndexRule utilizes indexes correctly after incremental refresh (append-only).") {


NOTE: this is a preexsisting test with only a REFRESH_APPEND_ENABLED flag removed

src/main/scala/com/microsoft/hyperspace/Hyperspace.scala

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

imback82 · 2020-10-12T05:20:50Z

Can you or @thrajput also update the Python binding?

@AFFogarty can help on the C# side once 0.3 is released. (I will ping you @AFFogarty 😄 when it's ready)

src/main/scala/com/microsoft/hyperspace/actions/RefreshAction.scala

src/main/scala/com/microsoft/hyperspace/actions/RefreshAppendAction.scala

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

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

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

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

…anager.scala Co-authored-by: Terry Kim <yuminkim@gmail.com>

imback82

LGTM, thanks @apoorvedave1!

I will wait others to approve as well before merging.

imback82 · 2020-10-13T00:55:59Z

Merging this as no responses from @sezruby and @pirz. Feel free to make comments if you have any.

Pouria Pirzadeh and others added 30 commits August 25, 2020 22:07

add delete support for index refresh

4d002cd

Merge branch 'master' into pouriap/refreshDelete

f8f72f2

Merge branch 'master' into pouriap/refreshDelete

aed6255

add delete support to refresh index

bf98425

add delete support to refresh index

fcb4f95

add delete support to refresh index

ba55f30

add delete support to refresh index

a15b71d

fix index manager test case

70d9416

fix index content refresh

0b26209

Merge branch 'master' into pouriap/refreshDelete

3cc1e0f

Merge branch 'master' into pouriap/refreshDelete

bfa0543

fix merge conflicts

66fa8fb

refactor refresh code and add refresh delete

2a62d1d

check for lineage during refresh delete.

9916c25

fix refresh delete test

d808a13

Merge branch 'master' of github.com:apoorvedave1/hyperspace-1 into re…

096546e

…freshAppend

add merge functionality with test

7e516df

initial commit for refresh incremental support (append only)

ed6d49c

comment fix

2308ca9

Co-authored-by: Rahul Potharaju <rapoth@microsoft.com>

Delete RefreshIncrementalAction.scala

db46723

RefreshIncremental.scala is for the same purpose.

Merge branch 'master' into refreshAppend

bbeacdd

Merge remote-tracking branch 'upstream/master' into refreshAppend

a657556

add unit test to show refresh works fine

3cbfea5

Merge remote-tracking branch 'upstream/master' into refreshAppend

8e8da80

# Conflicts: # src/main/scala/com/microsoft/hyperspace/index/IndexLogEntry.scala # src/test/scala/com/microsoft/hyperspace/index/IndexLogEntryTest.scala

Merge branch 'master' into refreshAppend

c3de8ca

add sort node verification to the test

0c0c376

Update src/test/scala/com/microsoft/hyperspace/index/IndexManagerTest…

eb0c29a

…s.scala Co-authored-by: Rahul Potharaju <rapoth@microsoft.com>

Update src/test/scala/com/microsoft/hyperspace/index/IndexManagerTest…

5e25290

…s.scala Co-authored-by: Rahul Potharaju <rapoth@microsoft.com>

Update src/test/scala/com/microsoft/hyperspace/index/IndexManagerTest…

3347c6d

…s.scala Co-authored-by: Rahul Potharaju <rapoth@microsoft.com>

apoorvedave1 commented Oct 10, 2020

View reviewed changes

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

sezruby reviewed Oct 10, 2020

View reviewed changes

src/main/scala/com/microsoft/hyperspace/Hyperspace.scala Outdated Show resolved Hide resolved

src/test/scala/com/microsoft/hyperspace/index/E2EHyperspaceRulesTests.scala Show resolved Hide resolved

apoorvedave1 added 2 commits October 10, 2020 12:51

added tests as per review comments

91a27ea

review comments

458083e

apoorvedave1 commented Oct 10, 2020

View reviewed changes

apoorvedave1 added 2 commits October 10, 2020 12:57

nit

012c89b

add issue details for a known issue

60585d7

apoorvedave1 requested a review from sezruby October 10, 2020 20:24

imback82 reviewed Oct 12, 2020

View reviewed changes

src/main/scala/com/microsoft/hyperspace/Hyperspace.scala Outdated Show resolved Hide resolved

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

review comments

3b09a59

apoorvedave1 mentioned this pull request Oct 12, 2020

Update the Python binding for new refresh and optimize APIs #203

Closed

apoorvedave1 requested a review from imback82 October 12, 2020 18:07

imback82 reviewed Oct 12, 2020

View reviewed changes

apoorvedave1 and others added 4 commits October 12, 2020 11:51

Update src/main/scala/com/microsoft/hyperspace/index/IndexCollectionM…

f93831a

…anager.scala Co-authored-by: Terry Kim <yuminkim@gmail.com>

Merge remote-tracking branch 'upstream/master' into merge

a9e5493

review comments and test refactoring

87f4a8a

scalastyle

0ba4241

apoorvedave1 requested a review from imback82 October 12, 2020 19:38

imback82 and others added 2 commits October 12, 2020 14:10

cleanup

bdc6620

minor test fix

03be354

imback82 approved these changes Oct 12, 2020

View reviewed changes

apoorvedave1 mentioned this pull request Oct 12, 2020

Optimize Index, with "quick" and "full" modes #166

Merged

imback82 merged commit 5f9a3e5 into microsoft:master Oct 13, 2020

pirz mentioned this pull request Oct 14, 2020

Merge incremental mode append and delete actions into index refresh #149

Closed

3 tasks

apoorvedave1 deleted the merge branch March 16, 2021 22:40

apoorvedave1 restored the merge branch March 16, 2021 22:40

Merge append and delete actions on indexes on modified source data #187

Merge append and delete actions on indexes on modified source data #187

Uh oh!

Conversation

apoorvedave1 commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the context for this pull request?

What changes were proposed in this pull request?

1

2 PR #188 MERGED

3 PR #191 MERGED

4 PR #189 MERGED

5 PR #194 MERGED

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Uh oh!

sezruby left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

apoorvedave1 commented Oct 10, 2020

Uh oh!

apoorvedave1 commented Oct 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoorvedave1 Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

imback82 commented Oct 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

imback82 left a comment

Choose a reason for hiding this comment

Uh oh!

imback82 commented Oct 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

apoorvedave1 commented Oct 7, 2020 •

edited

Loading

apoorvedave1 commented Oct 10, 2020 •

edited

Loading