fix: ignore temp log entries #3423
Conversation
corwinjoy
requested review from
wjones127,
roeap,
rtyler,
hntd187 and
ion-elgreco
as code owners
|
ACTION NEEDED delta-rs follows the Conventional Commits specification for release automation. The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
| @@ -480,6 +480,7 @@ async fn list_log_files_with_checkpoint( | |||
| let version_prefix = format!("{:020}", cp.version); | |||
| let start_from = log_root.child(version_prefix.as_str()); | |||
|
|
|||
| // QUESTION: DOES THIS NEED TO BE FILTERED TO ELIMINATE temp subdirs? | |||
There was a problem hiding this comment.
I think this function, plus the one below may need to be fixed as well to ignore temp subdirectories but I could use some feedback here + pointers to the associated unit test.
| @@ -0,0 +1,2 @@ | |||
| {"commitInfo":{"timestamp":1587968626637,"operation":"WRITE","operationParameters":{"mode":"Overwrite","partitionBy":"[]"},"readVersion":4,"isBlindAppend":true}} | |||
| {"add":{"path":"part-00000-c1777d7d-89d9-4790-b38a-6ee7e24456b1-c001.snappy.parquet","partitionValues":{},"size":262,"modificationTime":1587968626600,"dataChange":true}} | |||
There was a problem hiding this comment.
This is the pending operation in the .tmp subfolder + parquet file below.
Essentially just copied from existing transactions.
corwinjoy
force-pushed
the
ignore_temp_log_entries
branch
from
9ad4e09 to
c319ae4
Compare
|
What about ignoring any Prefixing a file with a |
This fix will already ignore any files or directories beginning with I wonder if it would make sense for |
| @@ -13,6 +15,11 @@ async fn time_travel_by_ds() { | |||
| ("00000000000000000002.json", "2020-05-03T22:47:31-07:00"), | |||
| ("00000000000000000003.json", "2020-05-04T22:47:31-07:00"), | |||
| ("00000000000000000004.json", "2020-05-05T22:47:31-07:00"), | |||
| // Final file is uncommitted by Spark and is in a .tmp subdir | |||
There was a problem hiding this comment.
Since this fix ignores any sub-directory, the test should also contain a testcase of a sub-directory not starting with a dot.
There was a problem hiding this comment.
I don't understand why we need a second test case? The name of the subdirectory does not matter, any and all files in subdirectories are ignored. I only used the name .tmp to clarify what is going on / better match what spark does.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3423 +/- ##
=======================================
Coverage 71.92% 71.93%
=======================================
Files 150 150
Lines 46494 46504 +10
Branches 46494 46504 +10
=======================================
+ Hits 33441 33451 +10
+ Misses 10899 10898 -1
- Partials 2154 2155 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rtyler
force-pushed
the
ignore_temp_log_entries
branch
from
4310411 to
4a6740e
Compare
…directory of delta_log. Signed-off-by: Corwin Joy <[email protected]> Co-authored-by: Adam Reeve <[email protected]>
rtyler
force-pushed
the
ignore_temp_log_entries
branch
from
4a6740e to
f0bf7ed
Compare
|
@rtyler Thanks for reviewing this and merging! |
Description
Fix delta-rs methods so that delta_log entries in subdirectories, representing pending but uncommited transactions, are ignored.
Related Issue(s)
Closes load_with_datetime
Documentation
As per the bug report only root level delta files should be included:
According to the delta protocol:
https://github.com/delta-io/delta/blob/master/PROTOCOL.md#delta-log-entries
"Delta files are stored as JSON in a directory at the root of the table named _delta_log, and together with checkpoints make up the log of all changes that have occurred to a table."