[HUDI-8570] Use secondary index only for snapshot queries #12322
Conversation
|
also, do we have a similar test for RLI ? if not can we add one |
Yes we have one here - |
| // updated record should still show in time travel view | ||
| assertEquals(1, timeTravelDF.where(s"_row_key = '$recordKeyToUpdate'").count()) | ||
| // rider field (secondary key) should point to previous value | ||
| val secondaryKey = timeTravelDF.where(s"_row_key = '$recordKeyToUpdate'").select("rider").collect().head.getString(0) |
There was a problem hiding this comment.
so, this test will succeed even if there is a bug where we poll sec index for time travel query right?
There was a problem hiding this comment.
can you corrupt one of the data files and ensure time travel fails. we are likely testing the reverse of it.
i.e. if sec index pruning kicked in, the corrupted file will not be read and the query will succeed.
There was a problem hiding this comment.
can you corrupt one of the data files and ensure time travel fails.
I did not get this part. If the data file is corrupt, then it should fail anyway irrespective of whether time travel used sec index or not. What this test checks is that we get valid previous value (as of first commit) and the screenshot in PR description shows that the secondary index pruning did not kick in at all because it does not support time travel query.
There was a problem hiding this comment.
I trust w/ your screen shot that it works today. but the purpose of adding a test case is, tomorrow if someone changes anything the test should fail. i.e. unintentionally if time travel was looking up in secondary index.
So, to account for that. in addition to your existing assertions. can we do the following:
- just go through ur usual test set up
- add assertion that time travel works.
- corrupt one of the data file which should not match if secondary index was looked up.
- now re-execute the time travel query and assert for exception. Our theory here is that, SI lookup should not happen and hence all file slices will be read and hence the corrupt data file will be read and it will be thrown.
here the assertion for exception is important.
So, w/ this test, in future if someone untentionally reverts and ends up looking up in SI for a TT query, this test will fail since it may not throw exception
Change Logs
Use secondary index only for snapshot queries. Skip sec index and fallback to regular query path for query types such as time travel and incremental.
SecondaryIndexSupportsubclassesRecordLevelIndexSupportandsupportsQueryTypeof RLI is reused -hudi/hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/RecordLevelIndexSupport.scala
Line 102 in ae5833e
Added a test with time travel query and filter having column for which secondary index available. Test validates record is as of previous timestamp provided in query, and attached screenshot below where
supportsQueryTypeAPI returns false.Impact
Use secondary index only for snapshot queries.
Risk level (write none, low medium or high below)
low
Documentation Update
Describe any necessary documentation update if there is any new feature, config, or user-facing change. If not, put "none".
ticket number here and follow the instruction to make
changes to the website.
Contributor's checklist