You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "chenjian2664 (via GitHub)" <gi...@apache.org> on 2025/06/12 08:47:24 UTC
[PR] Use time-travel schema when resolving partition spec in scan [iceberg]
chenjian2664 opened a new pull request, #13301:
URL: https://github.com/apache/iceberg/pull/13301
(no comment)
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#issuecomment-3437788572
> @nastra: Any more comments?
I'll take another look tomorrow/monday with fresh eyes
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2446774254
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,26 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema()
+ || snapshotId() == null
+ || table().currentSnapshot() == null
+ || snapshotId().equals(table().currentSnapshot().snapshotId())) {
+ return specs;
+ }
+
+ // this is a time travel request
+ Schema snapshotSchema = tableSchema();
+ Map<Integer, PartitionSpec> newSpecs = Maps.newHashMapWithExpectedSize(specs.size());
Review Comment:
We could use the ImmutableMap Builder to avoid the extra variable
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#issuecomment-3162007065
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2407171679
##########
core/src/test/java/org/apache/iceberg/TestScansAndSchemaEvolution.java:
##########
@@ -113,4 +118,48 @@ public void testPartitionSourceRename() throws IOException {
assertThat(tasks).hasSize(1);
}
+
+ @TestTemplate
+ public void testColumnRenameOrDrop() throws IOException {
+ File location = Files.createTempDirectory(temp, "junit").toFile();
+ assertThat(location.delete()).isTrue(); // should be created by table create
+
+ Table table = TestTables.create(location, "test", SCHEMA, SPEC, formatVersion);
+
+ DataFile fileOne = createDataFile("one");
+ DataFile fileTwo = createDataFile("two");
+
+ table.newAppend().appendFile(fileOne).appendFile(fileTwo).commit();
+ long firstSnapshotId = table.currentSnapshot().snapshotId();
+
+ table.updateSchema().renameColumn("data", "renamed_data").commit();
Review Comment:
also added a drop case for partition
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#issuecomment-3414138139
@pvary updated, and tested on my local
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#issuecomment-3401176175
@chenjian2664: Could you please check the failing test?
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2174188811
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,22 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema() || snapshotId() == null) {
+ return specs;
+ }
+
Review Comment:
updated
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #13301: Core: Use time-travel schema when resolving partition spec in scan
URL: https://github.com/apache/iceberg/pull/13301
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2452655714
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -21,6 +21,7 @@
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
+import org.apache.curator.shaded.com.google.common.collect.ImmutableMap;
Review Comment:
nit: use the iceberg shaded import
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2451661047
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,26 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema()
+ || snapshotId() == null
+ || table().currentSnapshot() == null
+ || snapshotId().equals(table().currentSnapshot().snapshotId())) {
+ return specs;
+ }
+
+ // this is a time travel request
+ Schema snapshotSchema = tableSchema();
+ Map<Integer, PartitionSpec> newSpecs = Maps.newHashMapWithExpectedSize(specs.size());
Review Comment:
updated to use `ImmutableMap`
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#issuecomment-3437750246
@nastra: Any more comments?
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "findinpath (via GitHub)" <gi...@apache.org>.
findinpath commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2142706082
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,22 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema() || snapshotId() == null) {
+ return specs;
+ }
+
Review Comment:
can we have a test showcasing the functionality fixed by this contribution?
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2439768674
##########
core/src/test/java/org/apache/iceberg/TestScansAndSchemaEvolution.java:
##########
@@ -111,6 +118,133 @@ public void testPartitionSourceRename() throws IOException {
tasks = Lists.newArrayList(table.newScan().filter(Expressions.equal("p", "one")).planFiles());
assertThat(tasks).hasSize(1);
+
+ // create a new commit
+ table.newAppend().appendFile(createDataFile("three")).commit();
+
+ // use fiter with previous partition name
+ tasks =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
+ .filter(Expressions.equal("part", "one"))
+ .planFiles());
+
+ assertThat(tasks).hasSize(1);
+ }
+
+ @TestTemplate
+ public void testPartitionSourceDrop() throws IOException {
+ Table table = TestTables.create(temp, "test", SCHEMA, SPEC, formatVersion);
+
+ DataFile fileOne = createDataFile("one");
+ DataFile fileTwo = createDataFile("two");
+
+ table.newAppend().appendFile(fileOne).appendFile(fileTwo).commit();
+ long firstSnapshotId = table.currentSnapshot().snapshotId();
+
+ table.updateSpec().addField("id").commit();
+
+ List<FileScanTask> tasks =
+ Lists.newArrayList(
+ table.newScan().filter(Expressions.not(Expressions.isNull("id"))).planFiles());
+
+ assertThat(tasks).hasSize(2);
+
+ DataFile fileThree = createDataFile("three", table.schema(), table.spec());
+ table.newAppend().appendFile(fileThree).commit();
+
+ // remove one field from spec and drop the column
+ table.updateSpec().removeField("id").commit();
+ table.updateSchema().deleteColumn("id").commit();
+
+ List<FileScanTask> tasksAtFirstSnapshotId =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
+ .filter(Expressions.not(Expressions.isNull("id")))
+ .planFiles());
+
+ assertThat(
+ tasksAtFirstSnapshotId.stream()
+ .map(ContentScanTask::file)
+ .map(ContentFile::location)
+ .collect(Collectors.toList()))
+ .isEqualTo(
+ tasks.stream()
+ .map(ContentScanTask::file)
+ .map(ContentFile::location)
+ .collect(Collectors.toList()));
+ }
+
+ @TestTemplate
+ public void testColumnRename() throws IOException {
+ Table table = TestTables.create(temp, "test", SCHEMA, SPEC, formatVersion);
+
+ DataFile fileOne = createDataFile("one");
+ DataFile fileTwo = createDataFile("two");
+
+ table.newAppend().appendFile(fileOne).appendFile(fileTwo).commit();
+ long firstSnapshotId = table.currentSnapshot().snapshotId();
+
+ table.updateSchema().renameColumn("data", "renamed_data").commit();
+
+ DataFile fileThree = createDataFile("three", table.schema(), table.spec());
+ table.newAppend().appendFile(fileThree).commit();
+ long secondSnapshotId = table.currentSnapshot().snapshotId();
+
+ // generate a new commit
+ DataFile fileFour = createDataFile("four", table.schema(), table.spec());
+ table.newAppend().appendFile(fileFour).commit();
+
+ // running successfully with the new filter on previous column name
+ List<FileScanTask> tasks =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
Review Comment:
Yes, it's intentional that testing time travel works correctly when the predicate exists only in earlier snapshots.
But I can add a case to exercise the current snapshot if you think it is necessary
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#issuecomment-3421899173
Thanks @chenjian2664.
LGTM.
Let's give others some time and see if someone else has some comments.
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2450871979
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,26 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema()
+ || snapshotId() == null
+ || table().currentSnapshot() == null
+ || snapshotId().equals(table().currentSnapshot().snapshotId())) {
+ return specs;
+ }
+
+ // this is a time travel request
+ Schema snapshotSchema = tableSchema();
+ Map<Integer, PartitionSpec> newSpecs = Maps.newHashMapWithExpectedSize(specs.size());
Review Comment:
I would prefer to use `ImmutableMap`, like:
```suggestion
ImmutableMap.Builder<Integer, PartitionSpec> newSpecs = ImmutableMap.builder();
```
We use `ImmutableMap` whenever it is viable.
See:
```
➜ g grep ImmutableMap|wc -l
3492
➜ g grep Collections.unmodifiableMap|wc -l
14
```
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2189091923
##########
core/src/main/java/org/apache/iceberg/AllManifestsTable.java:
##########
@@ -119,7 +119,7 @@ protected TableScan newRefinedScan(Table table, Schema schema, TableScanContext
@Override
protected CloseableIterable<FileScanTask> doPlanFiles() {
FileIO io = table().io();
- Map<Integer, PartitionSpec> specs = Maps.newHashMap(table().specs());
+ Map<Integer, PartitionSpec> specs = Maps.newHashMap(specs());
Review Comment:
yes, removed.
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2438588025
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,25 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema()
+ || snapshotId() == null
+ || snapshotId().equals(table().currentSnapshot().snapshotId())) {
+ return specs;
+ }
+
+ // this is a time travel request
+ Schema snapshotSchema = tableSchema();
+ Map<Integer, PartitionSpec> newSpecs = Maps.newHashMapWithExpectedSize(specs.size());
+ for (Map.Entry<Integer, PartitionSpec> entry : specs.entrySet()) {
+ newSpecs.put(entry.getKey(), entry.getValue().toUnbound().bindUnchecked(snapshotSchema));
Review Comment:
Update to use `bind` instead
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2439328299
##########
core/src/test/java/org/apache/iceberg/TestScansAndSchemaEvolution.java:
##########
@@ -111,6 +118,133 @@ public void testPartitionSourceRename() throws IOException {
tasks = Lists.newArrayList(table.newScan().filter(Expressions.equal("p", "one")).planFiles());
assertThat(tasks).hasSize(1);
+
+ // create a new commit
+ table.newAppend().appendFile(createDataFile("three")).commit();
+
+ // use fiter with previous partition name
+ tasks =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
+ .filter(Expressions.equal("part", "one"))
+ .planFiles());
+
+ assertThat(tasks).hasSize(1);
+ }
+
+ @TestTemplate
+ public void testPartitionSourceDrop() throws IOException {
+ Table table = TestTables.create(temp, "test", SCHEMA, SPEC, formatVersion);
+
+ DataFile fileOne = createDataFile("one");
+ DataFile fileTwo = createDataFile("two");
+
+ table.newAppend().appendFile(fileOne).appendFile(fileTwo).commit();
+ long firstSnapshotId = table.currentSnapshot().snapshotId();
+
+ table.updateSpec().addField("id").commit();
+
+ List<FileScanTask> tasks =
+ Lists.newArrayList(
+ table.newScan().filter(Expressions.not(Expressions.isNull("id"))).planFiles());
+
+ assertThat(tasks).hasSize(2);
+
+ DataFile fileThree = createDataFile("three", table.schema(), table.spec());
+ table.newAppend().appendFile(fileThree).commit();
+
+ // remove one field from spec and drop the column
+ table.updateSpec().removeField("id").commit();
+ table.updateSchema().deleteColumn("id").commit();
+
+ List<FileScanTask> tasksAtFirstSnapshotId =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
+ .filter(Expressions.not(Expressions.isNull("id")))
+ .planFiles());
+
+ assertThat(
+ tasksAtFirstSnapshotId.stream()
+ .map(ContentScanTask::file)
+ .map(ContentFile::location)
+ .collect(Collectors.toList()))
+ .isEqualTo(
+ tasks.stream()
+ .map(ContentScanTask::file)
+ .map(ContentFile::location)
+ .collect(Collectors.toList()));
+ }
+
+ @TestTemplate
+ public void testColumnRename() throws IOException {
+ Table table = TestTables.create(temp, "test", SCHEMA, SPEC, formatVersion);
+
+ DataFile fileOne = createDataFile("one");
+ DataFile fileTwo = createDataFile("two");
+
+ table.newAppend().appendFile(fileOne).appendFile(fileTwo).commit();
+ long firstSnapshotId = table.currentSnapshot().snapshotId();
+
+ table.updateSchema().renameColumn("data", "renamed_data").commit();
+
+ DataFile fileThree = createDataFile("three", table.schema(), table.spec());
+ table.newAppend().appendFile(fileThree).commit();
+ long secondSnapshotId = table.currentSnapshot().snapshotId();
+
+ // generate a new commit
+ DataFile fileFour = createDataFile("four", table.schema(), table.spec());
+ table.newAppend().appendFile(fileFour).commit();
+
+ // running successfully with the new filter on previous column name
+ List<FileScanTask> tasks =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
Review Comment:
Is it intentional that we don't have assertions in this testcase where we plan for the current snapshot?
Otherwise looks good to me
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2446435217
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,26 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema()
+ || snapshotId() == null
+ || table().currentSnapshot() == null
+ || snapshotId().equals(table().currentSnapshot().snapshotId())) {
+ return specs;
+ }
+
+ // this is a time travel request
+ Schema snapshotSchema = tableSchema();
+ Map<Integer, PartitionSpec> newSpecs = Maps.newHashMapWithExpectedSize(specs.size());
Review Comment:
Are we returning a mutable map intentionally? How about using `ImmutableMap.toImmutableMap` or `Collections.unmodifiableMap(newSpecs)`? The former is less common in this repository, but we can avoid a local variable.
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2368170825
##########
core/src/test/java/org/apache/iceberg/TestScansAndSchemaEvolution.java:
##########
@@ -113,4 +118,48 @@ public void testPartitionSourceRename() throws IOException {
assertThat(tasks).hasSize(1);
}
+
+ @TestTemplate
+ public void testColumnRenameOrDrop() throws IOException {
+ File location = Files.createTempDirectory(temp, "junit").toFile();
+ assertThat(location.delete()).isTrue(); // should be created by table create
+
+ Table table = TestTables.create(location, "test", SCHEMA, SPEC, formatVersion);
+
+ DataFile fileOne = createDataFile("one");
+ DataFile fileTwo = createDataFile("two");
+
+ table.newAppend().appendFile(fileOne).appendFile(fileTwo).commit();
+ long firstSnapshotId = table.currentSnapshot().snapshotId();
+
+ table.updateSchema().renameColumn("data", "renamed_data").commit();
Review Comment:
Why is this comment closed?
I think @ebyhr suggestion is actually a valid one
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2439896307
##########
core/src/test/java/org/apache/iceberg/TestScansAndSchemaEvolution.java:
##########
@@ -111,6 +118,133 @@ public void testPartitionSourceRename() throws IOException {
tasks = Lists.newArrayList(table.newScan().filter(Expressions.equal("p", "one")).planFiles());
assertThat(tasks).hasSize(1);
+
+ // create a new commit
+ table.newAppend().appendFile(createDataFile("three")).commit();
+
+ // use fiter with previous partition name
+ tasks =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
+ .filter(Expressions.equal("part", "one"))
+ .planFiles());
+
+ assertThat(tasks).hasSize(1);
+ }
+
+ @TestTemplate
+ public void testPartitionSourceDrop() throws IOException {
+ Table table = TestTables.create(temp, "test", SCHEMA, SPEC, formatVersion);
+
+ DataFile fileOne = createDataFile("one");
+ DataFile fileTwo = createDataFile("two");
+
+ table.newAppend().appendFile(fileOne).appendFile(fileTwo).commit();
+ long firstSnapshotId = table.currentSnapshot().snapshotId();
+
+ table.updateSpec().addField("id").commit();
+
+ List<FileScanTask> tasks =
+ Lists.newArrayList(
+ table.newScan().filter(Expressions.not(Expressions.isNull("id"))).planFiles());
+
+ assertThat(tasks).hasSize(2);
+
+ DataFile fileThree = createDataFile("three", table.schema(), table.spec());
+ table.newAppend().appendFile(fileThree).commit();
+
+ // remove one field from spec and drop the column
+ table.updateSpec().removeField("id").commit();
+ table.updateSchema().deleteColumn("id").commit();
+
+ List<FileScanTask> tasksAtFirstSnapshotId =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
+ .filter(Expressions.not(Expressions.isNull("id")))
+ .planFiles());
+
+ assertThat(
+ tasksAtFirstSnapshotId.stream()
+ .map(ContentScanTask::file)
+ .map(ContentFile::location)
+ .collect(Collectors.toList()))
+ .isEqualTo(
+ tasks.stream()
+ .map(ContentScanTask::file)
+ .map(ContentFile::location)
+ .collect(Collectors.toList()));
+ }
+
+ @TestTemplate
+ public void testColumnRename() throws IOException {
+ Table table = TestTables.create(temp, "test", SCHEMA, SPEC, formatVersion);
+
+ DataFile fileOne = createDataFile("one");
+ DataFile fileTwo = createDataFile("two");
+
+ table.newAppend().appendFile(fileOne).appendFile(fileTwo).commit();
+ long firstSnapshotId = table.currentSnapshot().snapshotId();
+
+ table.updateSchema().renameColumn("data", "renamed_data").commit();
+
+ DataFile fileThree = createDataFile("three", table.schema(), table.spec());
+ table.newAppend().appendFile(fileThree).commit();
+ long secondSnapshotId = table.currentSnapshot().snapshotId();
+
+ // generate a new commit
+ DataFile fileFour = createDataFile("four", table.schema(), table.spec());
+ table.newAppend().appendFile(fileFour).commit();
+
+ // running successfully with the new filter on previous column name
+ List<FileScanTask> tasks =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
Review Comment:
Thanks @chenjian2664!
It would be nice. All of the other tests you checked that the planning works correctly without specifying the snapshot. It would be good to do it here too.
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2451646029
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,26 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema()
+ || snapshotId() == null
+ || table().currentSnapshot() == null
+ || snapshotId().equals(table().currentSnapshot().snapshotId())) {
+ return specs;
+ }
+
+ // this is a time travel request
+ Schema snapshotSchema = tableSchema();
+ Map<Integer, PartitionSpec> newSpecs = Maps.newHashMapWithExpectedSize(specs.size());
Review Comment:
Thanks.
Interesting part of creation of map:
```
g grep Maps.newHashMap | wc -l
1485
```
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2368166726
##########
core/src/test/java/org/apache/iceberg/TestScansAndSchemaEvolution.java:
##########
@@ -111,6 +117,61 @@ public void testPartitionSourceRename() throws IOException {
tasks = Lists.newArrayList(table.newScan().filter(Expressions.equal("p", "one")).planFiles());
assertThat(tasks).hasSize(1);
+
+ // create a new commit
+ table.newAppend().appendFile(createDataFile("three")).commit();
+
+ // use fiter with previous partition name
+ tasks =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
+ .filter(Expressions.equal("part", "one"))
+ .planFiles());
+
+ assertThat(tasks).hasSize(1);
+ }
+
+ @TestTemplate
+ public void testColumnRenameOrDrop() throws IOException {
Review Comment:
nit: could we split this 2 test case to 2 methods?
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#issuecomment-3451728536
Thank you @pvary @ebyhr @nastra !
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary merged PR #13301:
URL: https://github.com/apache/iceberg/pull/13301
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#issuecomment-3276997769
@manuzhang Any updates?
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2144080808
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,22 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema() || snapshotId() == null) {
+ return specs;
+ }
+
Review Comment:
I am looking for the proper place to add the test, but I’m not sure where it should go. would appreciate some guidance
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2375840532
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,22 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema() || snapshotId() == null) {
+ return specs;
+ }
+
+ // this is a time travel request
+ Schema snapshotSchema = tableSchema();
+ Map<Integer, PartitionSpec> newSpecs = Maps.newHashMapWithExpectedSize(specs.size());
+ for (Map.Entry<Integer, PartitionSpec> entry : specs.entrySet()) {
+ newSpecs.put(entry.getKey(), entry.getValue().toUnbound().bindUnchecked(snapshotSchema));
+ }
+ return newSpecs;
Review Comment:
The coding style guide states:
> always place a newline after control blocks (if, for, while, switch, etc.)
See: https://iceberg.apache.org/contribute/#block-spacing
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2442415133
##########
core/src/test/java/org/apache/iceberg/TestScansAndSchemaEvolution.java:
##########
@@ -111,6 +118,133 @@ public void testPartitionSourceRename() throws IOException {
tasks = Lists.newArrayList(table.newScan().filter(Expressions.equal("p", "one")).planFiles());
assertThat(tasks).hasSize(1);
+
+ // create a new commit
+ table.newAppend().appendFile(createDataFile("three")).commit();
+
+ // use fiter with previous partition name
+ tasks =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
+ .filter(Expressions.equal("part", "one"))
+ .planFiles());
+
+ assertThat(tasks).hasSize(1);
+ }
+
+ @TestTemplate
+ public void testPartitionSourceDrop() throws IOException {
+ Table table = TestTables.create(temp, "test", SCHEMA, SPEC, formatVersion);
+
+ DataFile fileOne = createDataFile("one");
+ DataFile fileTwo = createDataFile("two");
+
+ table.newAppend().appendFile(fileOne).appendFile(fileTwo).commit();
+ long firstSnapshotId = table.currentSnapshot().snapshotId();
+
+ table.updateSpec().addField("id").commit();
+
+ List<FileScanTask> tasks =
+ Lists.newArrayList(
+ table.newScan().filter(Expressions.not(Expressions.isNull("id"))).planFiles());
+
+ assertThat(tasks).hasSize(2);
+
+ DataFile fileThree = createDataFile("three", table.schema(), table.spec());
+ table.newAppend().appendFile(fileThree).commit();
+
+ // remove one field from spec and drop the column
+ table.updateSpec().removeField("id").commit();
+ table.updateSchema().deleteColumn("id").commit();
+
+ List<FileScanTask> tasksAtFirstSnapshotId =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
+ .filter(Expressions.not(Expressions.isNull("id")))
+ .planFiles());
+
+ assertThat(
+ tasksAtFirstSnapshotId.stream()
+ .map(ContentScanTask::file)
+ .map(ContentFile::location)
+ .collect(Collectors.toList()))
+ .isEqualTo(
+ tasks.stream()
+ .map(ContentScanTask::file)
+ .map(ContentFile::location)
+ .collect(Collectors.toList()));
+ }
+
+ @TestTemplate
+ public void testColumnRename() throws IOException {
+ Table table = TestTables.create(temp, "test", SCHEMA, SPEC, formatVersion);
+
+ DataFile fileOne = createDataFile("one");
+ DataFile fileTwo = createDataFile("two");
+
+ table.newAppend().appendFile(fileOne).appendFile(fileTwo).commit();
+ long firstSnapshotId = table.currentSnapshot().snapshotId();
+
+ table.updateSchema().renameColumn("data", "renamed_data").commit();
+
+ DataFile fileThree = createDataFile("three", table.schema(), table.spec());
+ table.newAppend().appendFile(fileThree).commit();
+ long secondSnapshotId = table.currentSnapshot().snapshotId();
+
+ // generate a new commit
+ DataFile fileFour = createDataFile("four", table.schema(), table.spec());
+ table.newAppend().appendFile(fileFour).commit();
+
+ // running successfully with the new filter on previous column name
+ List<FileScanTask> tasks =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
Review Comment:
Added case without specifying the snapshot in all the tests:
For the renamed tests, uses the renamed column as the filter, for the dropped column, uses the `part`(one of the remaining) column instead.
##########
core/src/test/java/org/apache/iceberg/TestScansAndSchemaEvolution.java:
##########
@@ -111,6 +118,133 @@ public void testPartitionSourceRename() throws IOException {
tasks = Lists.newArrayList(table.newScan().filter(Expressions.equal("p", "one")).planFiles());
assertThat(tasks).hasSize(1);
+
+ // create a new commit
+ table.newAppend().appendFile(createDataFile("three")).commit();
+
+ // use fiter with previous partition name
+ tasks =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
+ .filter(Expressions.equal("part", "one"))
+ .planFiles());
+
+ assertThat(tasks).hasSize(1);
+ }
+
+ @TestTemplate
+ public void testPartitionSourceDrop() throws IOException {
+ Table table = TestTables.create(temp, "test", SCHEMA, SPEC, formatVersion);
+
+ DataFile fileOne = createDataFile("one");
+ DataFile fileTwo = createDataFile("two");
+
+ table.newAppend().appendFile(fileOne).appendFile(fileTwo).commit();
+ long firstSnapshotId = table.currentSnapshot().snapshotId();
+
+ table.updateSpec().addField("id").commit();
+
+ List<FileScanTask> tasks =
+ Lists.newArrayList(
+ table.newScan().filter(Expressions.not(Expressions.isNull("id"))).planFiles());
+
+ assertThat(tasks).hasSize(2);
+
+ DataFile fileThree = createDataFile("three", table.schema(), table.spec());
+ table.newAppend().appendFile(fileThree).commit();
+
+ // remove one field from spec and drop the column
+ table.updateSpec().removeField("id").commit();
+ table.updateSchema().deleteColumn("id").commit();
+
+ List<FileScanTask> tasksAtFirstSnapshotId =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
+ .filter(Expressions.not(Expressions.isNull("id")))
+ .planFiles());
+
+ assertThat(
+ tasksAtFirstSnapshotId.stream()
+ .map(ContentScanTask::file)
+ .map(ContentFile::location)
+ .collect(Collectors.toList()))
+ .isEqualTo(
+ tasks.stream()
+ .map(ContentScanTask::file)
+ .map(ContentFile::location)
+ .collect(Collectors.toList()));
+ }
+
+ @TestTemplate
+ public void testColumnRename() throws IOException {
+ Table table = TestTables.create(temp, "test", SCHEMA, SPEC, formatVersion);
+
+ DataFile fileOne = createDataFile("one");
+ DataFile fileTwo = createDataFile("two");
+
+ table.newAppend().appendFile(fileOne).appendFile(fileTwo).commit();
+ long firstSnapshotId = table.currentSnapshot().snapshotId();
+
+ table.updateSchema().renameColumn("data", "renamed_data").commit();
+
+ DataFile fileThree = createDataFile("three", table.schema(), table.spec());
+ table.newAppend().appendFile(fileThree).commit();
+ long secondSnapshotId = table.currentSnapshot().snapshotId();
+
+ // generate a new commit
+ DataFile fileFour = createDataFile("four", table.schema(), table.spec());
+ table.newAppend().appendFile(fileFour).commit();
+
+ // running successfully with the new filter on previous column name
+ List<FileScanTask> tasks =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
Review Comment:
Added case without specifying the snapshot in all the tests:
For the renamed tests, uses the renamed column as the filter, for the dropped column tests, uses the `part`(one of the remaining) column instead.
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2442415133
##########
core/src/test/java/org/apache/iceberg/TestScansAndSchemaEvolution.java:
##########
@@ -111,6 +118,133 @@ public void testPartitionSourceRename() throws IOException {
tasks = Lists.newArrayList(table.newScan().filter(Expressions.equal("p", "one")).planFiles());
assertThat(tasks).hasSize(1);
+
+ // create a new commit
+ table.newAppend().appendFile(createDataFile("three")).commit();
+
+ // use fiter with previous partition name
+ tasks =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
+ .filter(Expressions.equal("part", "one"))
+ .planFiles());
+
+ assertThat(tasks).hasSize(1);
+ }
+
+ @TestTemplate
+ public void testPartitionSourceDrop() throws IOException {
+ Table table = TestTables.create(temp, "test", SCHEMA, SPEC, formatVersion);
+
+ DataFile fileOne = createDataFile("one");
+ DataFile fileTwo = createDataFile("two");
+
+ table.newAppend().appendFile(fileOne).appendFile(fileTwo).commit();
+ long firstSnapshotId = table.currentSnapshot().snapshotId();
+
+ table.updateSpec().addField("id").commit();
+
+ List<FileScanTask> tasks =
+ Lists.newArrayList(
+ table.newScan().filter(Expressions.not(Expressions.isNull("id"))).planFiles());
+
+ assertThat(tasks).hasSize(2);
+
+ DataFile fileThree = createDataFile("three", table.schema(), table.spec());
+ table.newAppend().appendFile(fileThree).commit();
+
+ // remove one field from spec and drop the column
+ table.updateSpec().removeField("id").commit();
+ table.updateSchema().deleteColumn("id").commit();
+
+ List<FileScanTask> tasksAtFirstSnapshotId =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
+ .filter(Expressions.not(Expressions.isNull("id")))
+ .planFiles());
+
+ assertThat(
+ tasksAtFirstSnapshotId.stream()
+ .map(ContentScanTask::file)
+ .map(ContentFile::location)
+ .collect(Collectors.toList()))
+ .isEqualTo(
+ tasks.stream()
+ .map(ContentScanTask::file)
+ .map(ContentFile::location)
+ .collect(Collectors.toList()));
+ }
+
+ @TestTemplate
+ public void testColumnRename() throws IOException {
+ Table table = TestTables.create(temp, "test", SCHEMA, SPEC, formatVersion);
+
+ DataFile fileOne = createDataFile("one");
+ DataFile fileTwo = createDataFile("two");
+
+ table.newAppend().appendFile(fileOne).appendFile(fileTwo).commit();
+ long firstSnapshotId = table.currentSnapshot().snapshotId();
+
+ table.updateSchema().renameColumn("data", "renamed_data").commit();
+
+ DataFile fileThree = createDataFile("three", table.schema(), table.spec());
+ table.newAppend().appendFile(fileThree).commit();
+ long secondSnapshotId = table.currentSnapshot().snapshotId();
+
+ // generate a new commit
+ DataFile fileFour = createDataFile("four", table.schema(), table.spec());
+ table.newAppend().appendFile(fileFour).commit();
+
+ // running successfully with the new filter on previous column name
+ List<FileScanTask> tasks =
+ Lists.newArrayList(
+ table
+ .newScan()
+ .useSnapshot(firstSnapshotId)
Review Comment:
Added case without specifying the snapshot in all the tests:
For the renamed test, it uses the renamed column as the filter, for the dropped column, it uses the `part`(one of the remaining) column instead.
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2450033662
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,26 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema()
+ || snapshotId() == null
+ || table().currentSnapshot() == null
+ || snapshotId().equals(table().currentSnapshot().snapshotId())) {
+ return specs;
+ }
+
+ // this is a time travel request
+ Schema snapshotSchema = tableSchema();
+ Map<Integer, PartitionSpec> newSpecs = Maps.newHashMapWithExpectedSize(specs.size());
Review Comment:
Thanks @ebyhr @pvary
I updated to use `Collections.unmodifiableMap(newSpecs)` for consistency with other usages
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2446435217
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,26 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema()
+ || snapshotId() == null
+ || table().currentSnapshot() == null
+ || snapshotId().equals(table().currentSnapshot().snapshotId())) {
+ return specs;
+ }
+
+ // this is a time travel request
+ Schema snapshotSchema = tableSchema();
+ Map<Integer, PartitionSpec> newSpecs = Maps.newHashMapWithExpectedSize(specs.size());
Review Comment:
Are we returning a mutable map intentionally? How about using `ImmutableMap.toImmutableMap` (less common in this repository, but we can avoid a local variable) or `Collections.unmodifiableMap(newSpecs)`?
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2450871979
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,26 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema()
+ || snapshotId() == null
+ || table().currentSnapshot() == null
+ || snapshotId().equals(table().currentSnapshot().snapshotId())) {
+ return specs;
+ }
+
+ // this is a time travel request
+ Schema snapshotSchema = tableSchema();
+ Map<Integer, PartitionSpec> newSpecs = Maps.newHashMapWithExpectedSize(specs.size());
Review Comment:
I would prefer to use `ImmutableMap`, like:
```suggestion
ImmutableMap.Builder<Integer, PartitionSpec> newSpecs = ImmutableMap.builder();
```
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2375598709
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,22 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema() || snapshotId() == null) {
+ return specs;
+ }
+
+ // this is a time travel request
+ Schema snapshotSchema = tableSchema();
+ Map<Integer, PartitionSpec> newSpecs = Maps.newHashMapWithExpectedSize(specs.size());
+ for (Map.Entry<Integer, PartitionSpec> entry : specs.entrySet()) {
+ newSpecs.put(entry.getKey(), entry.getValue().toUnbound().bindUnchecked(snapshotSchema));
+ }
+ return newSpecs;
Review Comment:
Sorry, I didn't get it, do you mean add a new line at line 95?
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#issuecomment-3190264679
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#issuecomment-3400305029
@pvary @manuzhang Would you mind to have a look ?
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2429701421
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,25 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema()
+ || snapshotId() == null
+ || snapshotId().equals(table().currentSnapshot().snapshotId())) {
+ return specs;
+ }
+
+ // this is a time travel request
+ Schema snapshotSchema = tableSchema();
+ Map<Integer, PartitionSpec> newSpecs = Maps.newHashMapWithExpectedSize(specs.size());
+ for (Map.Entry<Integer, PartitionSpec> entry : specs.entrySet()) {
+ newSpecs.put(entry.getKey(), entry.getValue().toUnbound().bindUnchecked(snapshotSchema));
Review Comment:
For my education, why do we use `bindUnchecked` instead of `bind`?
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#issuecomment-3247459140
@manuzhang Thank you for helping reopen it!
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2184640499
##########
core/src/main/java/org/apache/iceberg/AllManifestsTable.java:
##########
@@ -119,7 +119,7 @@ protected TableScan newRefinedScan(Table table, Schema schema, TableScanContext
@Override
protected CloseableIterable<FileScanTask> doPlanFiles() {
FileIO io = table().io();
- Map<Integer, PartitionSpec> specs = Maps.newHashMap(table().specs());
+ Map<Integer, PartitionSpec> specs = Maps.newHashMap(specs());
Review Comment:
The new test passes without this change, right?
##########
core/src/test/java/org/apache/iceberg/TestScansAndSchemaEvolution.java:
##########
@@ -113,4 +118,48 @@ public void testPartitionSourceRename() throws IOException {
assertThat(tasks).hasSize(1);
}
+
+ @TestTemplate
+ public void testColumnRenameOrDrop() throws IOException {
+ File location = Files.createTempDirectory(temp, "junit").toFile();
+ assertThat(location.delete()).isTrue(); // should be created by table create
+
+ Table table = TestTables.create(location, "test", SCHEMA, SPEC, formatVersion);
+
+ DataFile fileOne = createDataFile("one");
+ DataFile fileTwo = createDataFile("two");
+
+ table.newAppend().appendFile(fileOne).appendFile(fileTwo).commit();
+ long firstSnapshotId = table.currentSnapshot().snapshotId();
+
+ table.updateSchema().renameColumn("data", "renamed_data").commit();
Review Comment:
How about testing time travel after partition evolution too?
##########
core/src/test/java/org/apache/iceberg/TestScansAndSchemaEvolution.java:
##########
@@ -113,4 +118,48 @@ public void testPartitionSourceRename() throws IOException {
assertThat(tasks).hasSize(1);
}
+
+ @TestTemplate
+ public void testColumnRenameOrDrop() throws IOException {
+ File location = Files.createTempDirectory(temp, "junit").toFile();
+ assertThat(location.delete()).isTrue(); // should be created by table create
Review Comment:
nit: We could change `@TempDir` Path to File so we can remove this logic.
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2368124593
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,22 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema() || snapshotId() == null) {
Review Comment:
Do we want to return the spec immediately `snapshotId == currentSnapshot`?
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2368079456
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,22 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema() || snapshotId() == null) {
+ return specs;
+ }
+
+ // this is a time travel request
+ Schema snapshotSchema = tableSchema();
+ Map<Integer, PartitionSpec> newSpecs = Maps.newHashMapWithExpectedSize(specs.size());
+ for (Map.Entry<Integer, PartitionSpec> entry : specs.entrySet()) {
+ newSpecs.put(entry.getKey(), entry.getValue().toUnbound().bindUnchecked(snapshotSchema));
+ }
+ return newSpecs;
Review Comment:
nit: newline
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2375663207
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,22 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema() || snapshotId() == null) {
Review Comment:
added a check for early stopping here
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#issuecomment-3451346117
Merged to main.
Thanks @chenjian2664 for the PR and @ebyhr, @nastra for the review!
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2446435217
##########
core/src/main/java/org/apache/iceberg/SnapshotScan.java:
##########
@@ -79,6 +79,26 @@ protected ScanMetrics scanMetrics() {
return scanMetrics;
}
+ protected Map<Integer, PartitionSpec> specs() {
+ Map<Integer, PartitionSpec> specs = table().specs();
+ // requires latest schema
+ if (!useSnapshotSchema()
+ || snapshotId() == null
+ || table().currentSnapshot() == null
+ || snapshotId().equals(table().currentSnapshot().snapshotId())) {
+ return specs;
+ }
+
+ // this is a time travel request
+ Schema snapshotSchema = tableSchema();
+ Map<Integer, PartitionSpec> newSpecs = Maps.newHashMapWithExpectedSize(specs.size());
Review Comment:
Are we returning a mutable map intentionally? How about using `ImmutableMap.toImmutableMap` or `Collections.unmodifiableMap(newSpecs)`?
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on code in PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#discussion_r2406824589
##########
core/src/test/java/org/apache/iceberg/TestScansAndSchemaEvolution.java:
##########
@@ -113,4 +118,48 @@ public void testPartitionSourceRename() throws IOException {
assertThat(tasks).hasSize(1);
}
+
+ @TestTemplate
+ public void testColumnRenameOrDrop() throws IOException {
+ File location = Files.createTempDirectory(temp, "junit").toFile();
+ assertThat(location.delete()).isTrue(); // should be created by table create
+
+ Table table = TestTables.create(location, "test", SCHEMA, SPEC, formatVersion);
+
+ DataFile fileOne = createDataFile("one");
+ DataFile fileTwo = createDataFile("two");
+
+ table.newAppend().appendFile(fileOne).appendFile(fileTwo).commit();
+ long firstSnapshotId = table.currentSnapshot().snapshotId();
+
+ table.updateSchema().renameColumn("data", "renamed_data").commit();
Review Comment:
see `testPartitionSourceRename`, added the test case for it
--
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: Use time-travel schema when resolving partition spec in scan [iceberg]
Posted by "chenjian2664 (via GitHub)" <gi...@apache.org>.
chenjian2664 commented on PR #13301:
URL: https://github.com/apache/iceberg/pull/13301#issuecomment-3290841214
gentle reminder @aokolnychyi @szehon-ho
--
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