Spark-3.3: Handle no-op for rewrite manifests procedure/action #6695
Conversation
|
|
||
| List<Object[]> output = sql("CALL %s.system.rewrite_manifests('%s')", catalogName, tableIdent); | ||
| // should not rewrite any manifests for no-op (output of rewrite is same as before and after) | ||
| assertEquals("Procedure output must match", ImmutableList.of(row(0, 0)), output); |
There was a problem hiding this comment.
the result will be (1,1) without this PR.
ajantha-bhat
force-pushed
the
rewritemanifest
branch
from
b5ea934 to
92da27f
Compare
| @@ -341,6 +341,10 @@ public void testRewriteImportedManifests() throws IOException { | |||
| SparkTableUtil.importSparkTable( | |||
| spark, new TableIdentifier("parquet_table"), table, stagingDir.toString()); | |||
|
|
|||
| // add some more data to create more than one manifest for the rewrite | |||
There was a problem hiding this comment.
It was a 1-to-1 no-op. So, added one more manifest to avoid no-op.
| @@ -431,6 +435,8 @@ public void testRewriteManifestsWithPredicate() throws IOException { | |||
| new ThreeColumnRecord(1, null, "AAAA"), new ThreeColumnRecord(1, "BBBBBBBBBB", "BBBB")); | |||
| writeRecords(records1); | |||
|
|
|||
| writeRecords(records1); | |||
There was a problem hiding this comment.
It was a 1-to-1 no-op. So, added one more manifest to avoid no-op.
| .rewriteIf(manifest -> manifest.path().equals(manifests.get(0).path())) | ||
| .rewriteIf( | ||
| manifest -> | ||
| (manifest.path().equals(manifests.get(0).path()) |
There was a problem hiding this comment.
selecting more than one manifests for rewrite
|
|
||
| List<ThreeColumnRecord> expectedRecords = Lists.newArrayList(); | ||
| expectedRecords.addAll(records1); | ||
| expectedRecords.add(records1.get(0)); |
There was a problem hiding this comment.
sorted order insert
|
I should be able to take a look. |
| @@ -172,6 +172,10 @@ private RewriteManifests.Result doExecute() { | |||
| int targetNumManifests = targetNumManifests(totalSizeBytes); | |||
| int targetNumManifestEntries = targetNumManifestEntries(numEntries, targetNumManifests); | |||
|
|
|||
| if (targetNumManifests == 1 && matchingManifests.size() == 1) { | |||
There was a problem hiding this comment.
I don't think this is safe as the only manifest that may match may be huge, which would be a bottleneck for planning. It is a valid scenario to be able to split one giant manifest into a number of smaller ones.
There was a problem hiding this comment.
I think we can skip this iff the size of the manifest is under the target manifest size.
There was a problem hiding this comment.
We will need tests for both scenarios.
There was a problem hiding this comment.
Rewriting one big manifest into smaller manifests still works with this change and we have testcases for it.
It works because targetNumManifests will be more than one in that case. So, it won't enter the new code.
We already have tests for both scenario's in this file it is called testRewriteLargeManifests
|
The change makes sense but we must be able to rewrite large manifests. |
@aokolnychyi: Rewriting one big manifest into smaller manifests still works with this change and we have testcases for it. |
|
My bad, I overlooked the condition, @ajantha-bhat! |
|
Thanks, @ajantha-bhat! I merged this. Would you mind following up with cherry-picks to other versions? |
Thanks for merging. Today I will work on backporting this PR to other spark versions. |
|
@aokolnychyi: back ported PR details. |
|
Thanks, @ajantha-bhat. Merged them! |
Incase of no-op (when the manifests are already optimized), rewrite manifest can still create one new file from the old file with the same contents. Which is a waste of resources. Hence, handling the no-op scenario.
Examples of no-op scenarios:
a) have one manifest and user calls rewrite manifests.
b) output of previous rewrite has created one smaller manifest (residual manifest after rewriting all the manifest to the target size) and user calls rewrite-manifests again.
The first time calling rewrite will create three 8MB file and one 1MB file.
If the rewrite is called again, no need to write just 1MB file into a new 1MB file.