You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "amogh-jahagirdar (via GitHub)" <gi...@apache.org> on 2023/10/26 17:56:54 UTC
[PR] Core: Ignore split offsets array when split offset is past file length [iceberg]
amogh-jahagirdar opened a new pull request, #8925:
URL: https://github.com/apache/iceberg/pull/8925
Follow up from a miss in the fix done in https://github.com/apache/iceberg/pull/8860. There's an additional splitOffsetArray method which gets used here https://github.com/apache/iceberg/blob/aa891acf20040d15e7ca59dc503adb3c1e4325b8/core/src/main/java/org/apache/iceberg/BaseContentScanTask.java#L134 .
Alternatively, we can undo the optimization done here: https://github.com/apache/iceberg/pull/8336#discussion_r1296190234 and then the original fix would be sufficient, and be simpler. Since the optimization seemed impactful, this fix currently just fixes both cases explicitly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Ignore split offsets array when split offset is past file length [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #8925:
URL: https://github.com/apache/iceberg/pull/8925#issuecomment-1783496227
> @amogh-jahagirdar, maybe this time we should create a test to validate FileScanTask.split when there are bad split offsets? I think that would have caught this in the last PR.
Done, added a new test which will exercise both `splitOffsetArray` and `splitOffsets`. This will also validate the number of tasks per file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Ignore split offsets array when split offset is past file length [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #8925:
URL: https://github.com/apache/iceberg/pull/8925#issuecomment-1781867237
@amogh-jahagirdar, maybe this time we should create a test to validate `FileScanTask.split` when there are bad split offsets? I think that would have caught this in the last PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Ignore split offsets array when split offset is past file length [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8925:
URL: https://github.com/apache/iceberg/pull/8925#discussion_r1375034993
##########
core/src/test/java/org/apache/iceberg/TestSplitPlanning.java:
##########
@@ -216,6 +217,34 @@ public void testSplitPlanningWithOffsets() {
"We should get one task per row group", 32, Iterables.size(scan.planTasks()));
}
+ @Test
+ public void testSplitPlanningWithCorruptedOffsets() throws IOException {
+ List<DataFile> files16Mb = newFilesWithInvalidOffset(16, 16 * 1024 * 1024, 2);
+ appendFiles(files16Mb);
+
+ // There will be 4 tasks per file, instead of 2 tasks per file, since the offsets are invalid
+ // and will be ignored.
+ TableScan scan =
+ table
+ .newScan()
+ .option(TableProperties.SPLIT_OPEN_FILE_COST, String.valueOf(0))
+ .option(TableProperties.SPLIT_SIZE, String.valueOf(4L * 1024 * 1024));
+
+ Assert.assertEquals(
+ "We should get four tasks per file since the offsets will be ignored",
+ 64,
+ Iterables.size(scan.planTasks()));
+ try (CloseableIterable<CombinedScanTask> tasks = scan.planTasks()) {
+ CombinedScanTask task = tasks.iterator().next().asCombinedScanTask();
+ Collection<FileScanTask> files = task.files();
+ for (FileScanTask fileScanTask : files) {
+ DataFile dataFile = fileScanTask.file();
+ Assert.assertNull(
Review Comment:
same as above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Ignore split offsets array when split offset is past file length [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8925:
URL: https://github.com/apache/iceberg/pull/8925#discussion_r1375034300
##########
core/src/test/java/org/apache/iceberg/TestSplitPlanning.java:
##########
@@ -216,6 +217,34 @@ public void testSplitPlanningWithOffsets() {
"We should get one task per row group", 32, Iterables.size(scan.planTasks()));
}
+ @Test
+ public void testSplitPlanningWithCorruptedOffsets() throws IOException {
+ List<DataFile> files16Mb = newFilesWithInvalidOffset(16, 16 * 1024 * 1024, 2);
+ appendFiles(files16Mb);
+
+ // There will be 4 tasks per file, instead of 2 tasks per file, since the offsets are invalid
+ // and will be ignored.
+ TableScan scan =
+ table
+ .newScan()
+ .option(TableProperties.SPLIT_OPEN_FILE_COST, String.valueOf(0))
+ .option(TableProperties.SPLIT_SIZE, String.valueOf(4L * 1024 * 1024));
+
+ Assert.assertEquals(
Review Comment:
minor: for newly added test code it would be better to use AssertJ-style assertions to make a future migration away from JUnit4 easier
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Ignore split offsets array when split offset is past file length [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8925:
URL: https://github.com/apache/iceberg/pull/8925#discussion_r1375117854
##########
core/src/test/java/org/apache/iceberg/TestSplitPlanning.java:
##########
@@ -216,6 +217,34 @@ public void testSplitPlanningWithOffsets() {
"We should get one task per row group", 32, Iterables.size(scan.planTasks()));
}
+ @Test
+ public void testSplitPlanningWithCorruptedOffsets() throws IOException {
Review Comment:
Discussed offline, this test exercises the path but has poor test hardness since the offsets end up being null anyways.
For proper test hardness we can go through TableScanUtil.planTaskGroups which will call FileScanTask.split and exercise the split offset array path. The new test will fail without the fix since the number of tasks will be different (without the fix, there will be 2 tasks, 1 per split offset but the last task is a bad task. With the fix, the split size of 1 byte will be used for the 10 byte file, and there will be 10 tasks)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Ignore split offsets array when split offset is past file length [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8925:
URL: https://github.com/apache/iceberg/pull/8925#discussion_r1375037491
##########
core/src/test/java/org/apache/iceberg/TestSplitPlanning.java:
##########
@@ -216,6 +217,34 @@ public void testSplitPlanningWithOffsets() {
"We should get one task per row group", 32, Iterables.size(scan.planTasks()));
}
+ @Test
+ public void testSplitPlanningWithCorruptedOffsets() throws IOException {
Review Comment:
unfortunately that test passes for me locally without the new fix in `BaseFile` (I woud have expected it to fail without the fix)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Ignore split offsets array when split offset is past file length [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #8925:
URL: https://github.com/apache/iceberg/pull/8925
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Ignore split offsets array when split offset is past file length [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8925:
URL: https://github.com/apache/iceberg/pull/8925#discussion_r1375037491
##########
core/src/test/java/org/apache/iceberg/TestSplitPlanning.java:
##########
@@ -216,6 +217,34 @@ public void testSplitPlanningWithOffsets() {
"We should get one task per row group", 32, Iterables.size(scan.planTasks()));
}
+ @Test
+ public void testSplitPlanningWithCorruptedOffsets() throws IOException {
Review Comment:
that test passes for me locally without the new fix in `BaseFile` (I woud have expected it to fail without the fix)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Ignore split offsets array when split offset is past file length [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8925:
URL: https://github.com/apache/iceberg/pull/8925#discussion_r1375037491
##########
core/src/test/java/org/apache/iceberg/TestSplitPlanning.java:
##########
@@ -216,6 +217,34 @@ public void testSplitPlanningWithOffsets() {
"We should get one task per row group", 32, Iterables.size(scan.planTasks()));
}
+ @Test
+ public void testSplitPlanningWithCorruptedOffsets() throws IOException {
Review Comment:
unfortunately that test passes for me locally without the new fix in `BaseFile`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Ignore split offsets array when split offset is past file length [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8925:
URL: https://github.com/apache/iceberg/pull/8925#discussion_r1375118142
##########
core/src/test/java/org/apache/iceberg/TestSplitPlanning.java:
##########
@@ -216,6 +217,34 @@ public void testSplitPlanningWithOffsets() {
"We should get one task per row group", 32, Iterables.size(scan.planTasks()));
}
+ @Test
+ public void testSplitPlanningWithCorruptedOffsets() throws IOException {
+ List<DataFile> files16Mb = newFilesWithInvalidOffset(16, 16 * 1024 * 1024, 2);
+ appendFiles(files16Mb);
+
+ // There will be 4 tasks per file, instead of 2 tasks per file, since the offsets are invalid
+ // and will be ignored.
+ TableScan scan =
+ table
+ .newScan()
+ .option(TableProperties.SPLIT_OPEN_FILE_COST, String.valueOf(0))
+ .option(TableProperties.SPLIT_SIZE, String.valueOf(4L * 1024 * 1024));
+
+ Assert.assertEquals(
Review Comment:
Done, this test is no longer applicable but for the new test it's using AssertJ
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org