Adds HoodieGlobalBloomIndex #438

leletan · 2018-08-14T00:49:34Z

WHAT

feature(HoodieGlobalBloomIndex): adds a new type of bloom index to allow global record key lookup

CLAassistant · 2018-08-14T00:49:41Z

All committers have signed the CLA.

n3nash · 2018-08-14T05:26:48Z

hoodie-client/src/main/java/com/uber/hoodie/index/bloom/HoodieGlobalBloomIndex.java

+/**
+ * This filter will only work with hoodie dataset since it will only load partition
+ * with .hoodie_partition_metadata file in it.
+ * Created by jiale.tan on 8/13/18.


Please remove the author names from the file

n3nash · 2018-08-14T05:26:59Z

hoodie-client/src/main/java/com/uber/hoodie/index/bloom/HoodieGlobalBloomIndex.java

@@ -0,0 +1,63 @@
+/*
+ *  Copyright (c) 2017 Uber Technologies, Inc. ([email protected])


Please change the year to 2018

n3nash · 2018-08-14T05:28:41Z

hoodie-client/src/main/java/com/uber/hoodie/index/bloom/HoodieGlobalBloomIndex.java

+ * with .hoodie_partition_metadata file in it.
+ * Created by jiale.tan on 8/13/18.
+ */
+public class HoodieGlobalBloomIndex<T extends HoodieRecordPayload> extends HoodieBloomIndex<T> {


Please override the following method here :

@Override public boolean isGlobal() { return true; }

hoodie-client/src/test/java/com/uber/hoodie/index/bloom/TestHoodieGlobalBloomIndex.java

vinothchandar

This is awesome.. Thanks @leletan :) . Have you been able to test this out with any toy datasets for validation?

hoodie-client/src/test/java/com/uber/hoodie/index/bloom/TestHoodieGlobalBloomIndex.java

n3nash · 2018-08-14T15:50:38Z

In general, very concise PR @leletan, just what was needed!

leletan · 2018-08-14T17:40:46Z

@vinothchandar @n3nash Thanks for the advices, will make change accordingly. Not yet validated with data set, will do it.

leletan · 2018-08-14T21:38:34Z

A quick small data set test indicates sometimes the cross partition deduping is not working, will dig deeper.

leletan · 2018-08-20T04:45:58Z

@vinothchandar wondering if there is any use case I may run some dataset to test this PR, other than cross partition deduping.

vinothchandar · 2018-08-20T14:27:53Z

Thats the main use-case.. Can't think of anything else top of my head.

but there can be several corner cases like

ensuring the partitionpath supplied in the RDD, has no bearing - i.e all keys checked against all files..
generate input such that partition paths in input RDD are a subset of total partitions in dataset (HoodieTestDataGenerator may have them to be same by default)..

vinothchandar · 2018-08-20T14:59:57Z

@leletan @n3nash I think I may know why global de-duping may not be working as expected

/**
   * For each incoming record, produce N output records, 1 each for each file against which the
   * record's key needs to be checked. For datasets, where the keys have a definite insert order
   * (e.g: timestamp as prefix), the number of files to be compared gets cut down a lot from range
   * pruning.
   */
  // sub-partition to ensure the records can be looked up against files & also prune
  // file<=>record comparisons based on recordKey
  // ranges in the index info.
  @VisibleForTesting
  JavaPairRDD<String, Tuple2<String, HoodieKey>> explodeRecordRDDWithFileComparisons(

this method still only joins input records using the partitionpath supplied in the HoodieKey, this needs to be overridden and fixed as well..

@n3nash are you able to take a pass here and provide a path for @leletan ? Also please take a look at usages of HoodieIndex::isGlobal and see if they will still be okay..

n3nash · 2018-08-20T22:58:42Z

@vinothchandar Yes, I can drive this. You are right, JavaPairRDD<String, Tuple2<String, HoodieKey>> explodeRecordRDDWithFileComparisons(, this needs to be overloaded for 2 reasons 1) To be able to autoComputeParallelism correctly and 2) For finding the correct matching files (which in this case should be all).

@leletan Please override that method as well. Also, I took a closer look at HoodieIndex::isGlobal, I think it's best to return false here. There are couple of places in the code which has assumptions about what isGlobal=true means and this implementation does not either satisfy those or does not apply to that assumption. There should be no side-effect of setting it to false.

@vinothchandar We need to remove those assumptions from the code (they are specifically there because of HbaseIndex), that would make isGlobal understanding clean.

n3nash · 2018-08-29T22:31:21Z

@leletan Just checking back here, were you able to make any progress on this ?

leletan · 2018-08-30T06:08:02Z

@n3nash Was still in the progress of making the end-to-end data test work.
First was stuck by this and found a work around in that ticket.

Then I was kind of stuck but just found a way to do a dedup later record here to make sure the cross partition dedup in this PR will keep the record in the old partition in my end-to-end testing.

And next step (likely tomorrow) I am going to continue this work based on your latest input.

But eventually my test will be based on changes on top of this branch which is based on 0.4.2-SNAPSHOT since we are using spark 2.3 in our production.

Wondering if you guys have any suggestions for end-to-end testing for this PR against master or maybe just running the PR against HoodieJavaApp is good enough

n3nash · 2018-08-31T18:04:44Z

@leletan thanks for the update. Definitely run the PR against HoodieJavaApp, also if you can try to run the PR against some dataset internally, that will help vet the PR more. May be run the dataset against the latest release (without your PR) and then with your PR, you can validate if your dataset behaves correctly in both scenarios; that's a good way to vet your code.

We don't have an exhaustive set of end to end testing as such (as we discussed f2f) but unit tests should capture most of the quirks.

leletan · 2018-09-01T00:38:03Z

Made some new modification based on comments and also added some unit tests. Will continue to do some real data testing later.

leletan · 2018-09-10T20:46:14Z

Ran data with HoodieJavaApp - seems working fine as expected. Now will put this into real spark ETL job for a test

n3nash · 2018-09-10T23:13:48Z

@leletan sounds great, let me know how it goes.

vinothchandar · 2018-09-11T05:34:29Z

nicee!

leletan · 2018-09-12T01:30:22Z

I had some changes based on discussion in #441, please let me know if it make sense to add that code into this PR or better to make a separate PR for that.

leletan · 2018-09-12T22:19:31Z

Tested this with our real spark ETL job along with some changes I made for #441, things are working fine. Let me know if I need to checkin those changes here as well or in a separate PR

n3nash · 2018-09-16T16:27:32Z

@leletan Please open a separate PR for changes made for changes talked in the #441. I'll take another pass at this PR later tonight.

hoodie-client/src/main/java/com/uber/hoodie/index/bloom/HoodieBloomIndex.java

n3nash · 2018-09-16T22:34:41Z

hoodie-client/src/main/java/com/uber/hoodie/index/bloom/HoodieGlobalBloomIndex.java

+    return partitionRecordKeyPairRDD.map(partitionRecordKeyPair -> {
+      String recordKey = partitionRecordKeyPair._2();
+
+      List<Tuple2<String, BloomIndexFileInfo>> indexInfos =


We are performing the exact same operation for all recordKey's here. May be move this outside ?

n3nash · 2018-09-16T22:46:53Z

hoodie-client/src/test/java/com/uber/hoodie/index/bloom/TestHoodieGlobalBloomIndex.java

+    return writeParquetFile(partitionPath, filename, records, schema, filter, createCommitTime);
+  }
+
+  private String writeParquetFile(String partitionPath, String filename, List<HoodieRecord> records, Schema schema,


Let's move these methods to HoodieTestUtils. These are reused across TestHoodieBloomIndex..

Good point will do.

n3nash · 2018-09-16T22:47:37Z

@leletan Left a few comments, rest looks ok.

leletan · 2018-09-18T07:18:14Z

Made change according to the comments.
If all things look fine, let me know if a final rebase is needed to have cleaner commit log.

hoodie-client/src/test/java/com/uber/hoodie/common/HoodieClientTestUtils.java

n3nash · 2018-09-18T15:24:02Z

hoodie-client/src/test/java/com/uber/hoodie/common/HoodieClientTestUtils.java

+                                        BloomFilter filter,
+                                        boolean createCommitTime) throws IOException, InterruptedException {
+    Thread.sleep(1000);
+    String commitTime = new SimpleDateFormat("yyyyMMddHHmmss").format(new Date());


There may already be a util in TestUtils to create a new dataFile, use that instead of creating a new file here ?

n3nash · 2018-09-18T15:26:13Z

hoodie-client/src/test/java/com/uber/hoodie/common/HoodieClientTestUtils.java

+
+    if (createCommitTime) {
+      // Also make sure the commit is valid
+      new File(basePath + "/" + HoodieTableMetaClient.METAFOLDER_NAME).mkdirs();


Add utils for these in HoodieTestUtils - createMetadataFolder etc.. and then use them here

n3nash · 2018-09-18T15:27:07Z

@leletan Left a few comments on the TestUtils usage, once that is done please squash your commits to a single commit.

n3nash · 2018-09-21T20:30:52Z

@leletan Please squash the commits and let me know when the diff is ready. I understand the cyclic dependency on HoodieTestUtils so let's avoid that. Please see if there are utils that you can reuse for creating datafiles etc (instead of creating them in the code as you have right now). We can get this diff landed soon.

leletan · 2018-09-22T03:58:41Z

Good point will do

leletan · 2018-09-24T05:37:24Z

Done

vinothchandar · 2018-09-28T02:09:30Z

hoodie-client/src/test/java/com/uber/hoodie/common/HoodieClientTestUtils.java

+                                        Schema schema,
+                                        BloomFilter filter,
+                                        boolean createCommitTime) throws IOException, InterruptedException {
+    Thread.sleep(1000);


@leletan any reason for the sleep?

@vinothchandar nope, this is just copying from https://github.com/uber/hudi/blob/master/hoodie-client/src/test/java/com/uber/hoodie/index/bloom/TestHoodieBloomIndex.java#L496

actually the 2 parquet writing helper functions are mostly just copied from that file

@vinothchandar This was added due to the need to create unique commitTimes since commit Times are at the granularity of a second. We should look at adding a testUtils to create unique commit times since this sleep() is leaking to many classes.

vinothchandar

@n3nash good to go?

n3nash · 2018-09-28T16:25:52Z

hoodie-client/src/main/java/com/uber/hoodie/index/bloom/HoodieGlobalBloomIndex.java

+   * (e.g: timestamp as prefix), the number of files to be compared gets cut down a lot from range
+   * pruning.
+   */
+  // sub-partition to ensure the records can be looked up against files & also prune


This style of commenting doesn't look correct. Could you either move everything here to one java doc style or move some of them inside the method if you want to.

good point, will fix

n3nash · 2018-09-28T16:27:44Z

hoodie-client/src/test/java/com/uber/hoodie/index/bloom/TestHoodieBloomIndex.java

 import com.uber.hoodie.table.HoodieTable;
+


May be try to avoid reshuffle of these imports ?

n3nash · 2018-09-28T16:28:45Z

@leletan Left 2 minor comments after which this is good to merge.

@vinothchandar FYI

…low global record key lookup

leletan · 2018-09-28T21:45:31Z

Done. Also rebased and squashed the commits

n3nash · 2018-09-29T12:18:07Z

@vinothchandar LGTM

vinothchandar · 2018-09-29T14:25:41Z

this is an awesome contribution... thanks @leletan ! merged.. :)

n3nash reviewed Aug 14, 2018

View reviewed changes

hoodie-client/src/test/java/com/uber/hoodie/index/bloom/TestHoodieGlobalBloomIndex.java Show resolved Hide resolved

n3nash assigned leletan Aug 14, 2018

n3nash added the enhancement label Aug 14, 2018

vinothchandar reviewed Aug 14, 2018

View reviewed changes

hoodie-client/src/test/java/com/uber/hoodie/index/bloom/TestHoodieGlobalBloomIndex.java Show resolved Hide resolved

hoodie-client/src/test/java/com/uber/hoodie/index/bloom/TestHoodieGlobalBloomIndex.java Show resolved Hide resolved

leletan changed the title ~~Adds HoodieGlobalBloomIndex~~ [DO NOT MERGE] Adds HoodieGlobalBloomIndex Aug 14, 2018

leletan force-pushed the wip-global-bloomfilter-lookup branch from 9139aa8 to bfe573e Compare September 1, 2018 00:36

vinothchandar changed the title ~~[DO NOT MERGE] Adds HoodieGlobalBloomIndex~~ [WIP] Adds HoodieGlobalBloomIndex Sep 4, 2018

n3nash reviewed Sep 16, 2018

View reviewed changes

hoodie-client/src/main/java/com/uber/hoodie/index/bloom/HoodieBloomIndex.java Show resolved Hide resolved

n3nash reviewed Sep 16, 2018

View reviewed changes

n3nash reviewed Sep 18, 2018

View reviewed changes

hoodie-client/src/test/java/com/uber/hoodie/common/HoodieClientTestUtils.java Show resolved Hide resolved

n3nash reviewed Sep 18, 2018

View reviewed changes

leletan closed this Sep 19, 2018

leletan reopened this Sep 19, 2018

leletan closed this Sep 19, 2018

leletan reopened this Sep 19, 2018

leletan force-pushed the wip-global-bloomfilter-lookup branch from 19ad7e6 to 2b3ee20 Compare September 24, 2018 05:35

vinothchandar reviewed Sep 28, 2018

View reviewed changes

vinothchandar changed the title ~~[WIP] Adds HoodieGlobalBloomIndex~~ Adds HoodieGlobalBloomIndex Sep 28, 2018

n3nash reviewed Sep 28, 2018

View reviewed changes

feature(HoodieGlobalBloomIndex): adds a new type of bloom index to al…

9709e2b

…low global record key lookup

leletan force-pushed the wip-global-bloomfilter-lookup branch from 2b3ee20 to 9709e2b Compare September 28, 2018 21:44

vinothchandar merged commit 98fd97b into apache:master Sep 29, 2018

leletan deleted the wip-global-bloomfilter-lookup branch November 29, 2018 07:33

vinishjail97 pushed a commit to vinishjail97/hudi that referenced this pull request Dec 15, 2023

[ENG-3744] Add log file filter (apache#438)

02b83b9

		@@ -0,0 +1,63 @@
		/*
		* Copyright (c) 2017 Uber Technologies, Inc. ([email protected])

Adds HoodieGlobalBloomIndex #438

Adds HoodieGlobalBloomIndex #438

Conversation

leletan commented Aug 14, 2018

WHAT

CLAassistant commented Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinothchandar left a comment

Choose a reason for hiding this comment

n3nash commented Aug 14, 2018

leletan commented Aug 14, 2018 • edited Loading

leletan commented Aug 14, 2018 • edited Loading

leletan commented Aug 20, 2018

vinothchandar commented Aug 20, 2018

vinothchandar commented Aug 20, 2018

n3nash commented Aug 20, 2018 • edited Loading

n3nash commented Aug 29, 2018 • edited Loading

leletan commented Aug 30, 2018 • edited Loading

n3nash commented Aug 31, 2018

leletan commented Sep 1, 2018

leletan commented Sep 10, 2018

n3nash commented Sep 10, 2018

vinothchandar commented Sep 11, 2018

leletan commented Sep 12, 2018

leletan commented Sep 12, 2018 • edited Loading

n3nash commented Sep 16, 2018

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

n3nash commented Sep 16, 2018

leletan commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n3nash commented Sep 18, 2018

n3nash commented Sep 21, 2018

leletan commented Sep 22, 2018

leletan commented Sep 24, 2018

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

vinothchandar 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

n3nash commented Sep 28, 2018

leletan commented Sep 28, 2018

n3nash commented Sep 29, 2018

vinothchandar commented Sep 29, 2018

CLAassistant commented Aug 14, 2018 •

edited

Loading

leletan commented Aug 14, 2018 •

edited

Loading

leletan commented Aug 14, 2018 •

edited

Loading

n3nash commented Aug 20, 2018 •

edited

Loading

n3nash commented Aug 29, 2018 •

edited

Loading

leletan commented Aug 30, 2018 •

edited

Loading

leletan commented Sep 12, 2018 •

edited

Loading