You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Andrew Sherman (Code Review)" <ge...@cloudera.org> on 2022/09/17 00:37:36 UTC
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Andrew Sherman has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19002
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Iceberg table modifications cause new table snapshots to be created;
these snapshots represent an earlier version of the table. The Iceberg
API provides a way to rollback the table to a previous snapshot.
This change add the ability to execute a rollback on Iceberg tables
using the following statements:
- ALTER TABLE <tbl> EXECUTE ROLLBACK(<snapshot id>)
- ALTER TABLE <tbl> EXECUTE ROLLBACK('<timestamp>')
The latter form of the command rolls back to the the latest snapshot
that has a creation timestamp that is older than the specified
timestamp.
Note that when a table is rolled back to a snapshot, a new snapshot is
created with the same snapshot id, but with a new creation timestamp.
Moved the logic that deals with id and timestamps from TimeTravelSpec
into a new parent class 'SnapshotSpec'. This allows the logic to be
reused in a new subclass. A few names were changed to be less specific
to the "AS OF" support.
Testing:
- Added analysis unit tests.
- Added e2e tests.
- Added a Snapshot class to the pytests to encapsulate the output of
DESCRIBE HISTORY.
Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
---
M common/thrift/JniCatalog.thrift
M fe/src/main/cup/sql-parser.cup
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
A fe/src/main/java/org/apache/impala/analysis/RollbackSpec.java
A fe/src/main/java/org/apache/impala/analysis/SnapshotSpec.java
M fe/src/main/java/org/apache/impala/analysis/TableRef.java
M fe/src/main/java/org/apache/impala/analysis/TimeTravelSpec.java
M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java
M fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
M fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/IcebergUtil.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
A fe/src/test/java/org/apache/impala/analysis/SnapshotSpecTest.java
A testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback.test
M tests/common/iceberg_test_suite.py
M tests/query_test/test_iceberg.py
19 files changed, 654 insertions(+), 174 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/19002/3
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Hello Tamas Mate, Gabor Kaszab, Zoltan Borok-Nagy, Gergely Fürnstáhl, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19002
to look at the new patch set (#11).
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Iceberg table modifications cause new table snapshots to be created;
these snapshots represent an earlier version of the table. The Iceberg
API provides a way to rollback the table to a previous snapshot.
This change adds the ability to execute a rollback on Iceberg tables
using the following statements:
- ALTER TABLE <tbl> EXECUTE ROLLBACK(<snapshot id>)
- ALTER TABLE <tbl> EXECUTE ROLLBACK('<timestamp>')
The latter form of the command rolls back to the most recent snapshot
that has a creation timestamp that is older than the specified
timestamp.
Note that when a table is rolled back to a snapshot, a new snapshot is
created with the same snapshot id, but with a new creation timestamp.
Testing:
- Added analysis unit tests.
- Added e2e tests.
- Converted test_time_travel to use get_snapshots() from iceberg_util.
- Add a utility class to allow pytests to create tables with various
iceberg catalogs.
Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
---
M common/thrift/JniCatalog.thrift
M fe/src/main/cup/sql-parser.cup
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteStmt.java
M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java
M fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
A testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback-negative.test
M tests/custom_cluster/test_events_custom_configs.py
M tests/query_test/test_iceberg.py
M tests/util/iceberg_util.py
15 files changed, 675 insertions(+), 203 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/19002/11
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Iceberg table modifications cause new table snapshots to be created;
these snapshots represent an earlier version of the table. The Iceberg
API provides a way to rollback the table to a previous snapshot.
This change add the ability to execute a rollback on Iceberg tables
using the following statements:
- ALTER TABLE <tbl> EXECUTE ROLLBACK(<snapshot id>)
- ALTER TABLE <tbl> EXECUTE ROLLBACK('<timestamp>')
The latter form of the command rolls back to the latest snapshot
that has a creation timestamp that is older than the specified
timestamp.
Note that when a table is rolled back to a snapshot, a new snapshot is
created with the same snapshot id, but with a new creation timestamp.
Testing:
- Added analysis unit tests.
- Added e2e tests.
- Added a Snapshot class to the pytests to encapsulate the output of
DESCRIBE HISTORY.
Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
---
M common/thrift/JniCatalog.thrift
M fe/src/main/cup/sql-parser.cup
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteStmt.java
M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java
M fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
A testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback.test
M tests/common/iceberg_test_suite.py
M tests/query_test/test_iceberg.py
14 files changed, 547 insertions(+), 110 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/19002/7
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/11539/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Oct 2022 18:09:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 9:
I had to rebase to get the upstream iceberg change.
The only actual code change here (beyond some very minor reformatting) is
- ManageSnapshots manageSnapshots = tbl.getIcebergApiTable().manageSnapshots();
+ ManageSnapshots manageSnapshots = iceTxn.manageSnapshots();
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Mar 2023 19:59:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 3:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/11373/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sat, 17 Sep 2022 00:58:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 4:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/11374/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sat, 17 Sep 2022 16:37:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Iceberg table modifications cause new table snapshots to be created;
these snapshots represent an earlier version of the table. The Iceberg
API provides a way to rollback the table to a previous snapshot.
This change adds the ability to execute a rollback on Iceberg tables
using the following statements:
- ALTER TABLE <tbl> EXECUTE ROLLBACK(<snapshot id>)
- ALTER TABLE <tbl> EXECUTE ROLLBACK('<timestamp>')
The latter form of the command rolls back to the most recent snapshot
that has a creation timestamp that is older than the specified
timestamp.
Note that when a table is rolled back to a snapshot, a new snapshot is
created with the same snapshot id, but with a new creation timestamp.
Testing:
- Added analysis unit tests.
- Added e2e tests.
- Converted test_time_travel to use get_snapshots() from iceberg_util.
- Add a utility class to allow pytests to create tables with various
iceberg catalogs.
Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Reviewed-on: http://gerrit.cloudera.org:8080/19002
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M common/thrift/JniCatalog.thrift
M fe/src/main/cup/sql-parser.cup
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteStmt.java
M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
A testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback-negative.test
M tests/custom_cluster/test_events_custom_configs.py
M tests/query_test/test_iceberg.py
M tests/util/iceberg_util.py
14 files changed, 675 insertions(+), 202 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 13
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Iceberg table modifications cause new table snapshots to be created;
these snapshots represent an earlier version of the table. The Iceberg
API provides a way to rollback the table to a previous snapshot.
This change add the ability to execute a rollback on Iceberg tables
using the following statements:
- ALTER TABLE <tbl> EXECUTE ROLLBACK(<snapshot id>)
- ALTER TABLE <tbl> EXECUTE ROLLBACK('<timestamp>')
The latter form of the command rolls back to the latest snapshot
that has a creation timestamp that is older than the specified
timestamp.
Note that when a table is rolled back to a snapshot, a new snapshot is
created with the same snapshot id, but with a new creation timestamp.
Testing:
- Added analysis unit tests.
- Added e2e tests.
- Added a Snapshot class to the pytests to encapsulate the output of
DESCRIBE HISTORY.
Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
---
M common/thrift/JniCatalog.thrift
M fe/src/main/cup/sql-parser.cup
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteStmt.java
M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java
M fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
A testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback.test
M tests/common/iceberg_test_suite.py
M tests/query_test/test_iceberg.py
14 files changed, 545 insertions(+), 110 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/19002/6
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Iceberg table modifications cause new table snapshots to be created;
these snapshots represent an earlier version of the table. The Iceberg
API provides a way to rollback the table to a previous snapshot.
This change add the ability to execute a rollback on Iceberg tables
using the following statements:
- ALTER TABLE <tbl> EXECUTE ROLLBACK(<snapshot id>)
- ALTER TABLE <tbl> EXECUTE ROLLBACK('<timestamp>')
The latter form of the command rolls back to the latest snapshot
that has a creation timestamp that is older than the specified
timestamp.
Note that when a table is rolled back to a snapshot, a new snapshot is
created with the same snapshot id, but with a new creation timestamp.
Testing:
- Added analysis unit tests.
- Added e2e tests.
- Added a Snapshot class to the pytests to encapsulate the output of
DESCRIBE HISTORY.
Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
---
M common/thrift/JniCatalog.thrift
M fe/src/main/cup/sql-parser.cup
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteStmt.java
M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java
M fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
A testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback.test
M tests/common/iceberg_test_suite.py
M tests/query_test/test_iceberg.py
14 files changed, 613 insertions(+), 131 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/19002/8
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 9:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/12522/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Mar 2023 20:11:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 9:
(9 comments)
Thanks for re-activating the work on this, Andrew!
http://gerrit.cloudera.org:8080/#/c/19002/9/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:
http://gerrit.cloudera.org:8080/#/c/19002/9/common/thrift/JniCatalog.thrift@431
PS9, Line 431: 1: required TRollbackType kind
nit: this could work without the 'kind' member by simply checking which one of 'timestamp_millis' or 'snapshot_id' is set.
http://gerrit.cloudera.org:8080/#/c/19002/9/common/thrift/JniCatalog.thrift@434
PS9, Line 434: 2: optional i64 timestamp_millis
Kind of unrelated and I might be totally off here, but the other day I saw in the Impala docs that a timestamp is represented on 16-bytes (also checked in the query plan that it takes 16 bytes in the 'row-size') but here we use an 8-byte variable to pass it.
https://impala.apache.org/docs/build/html/topics/impala_timestamp.html
http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java:
http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java@36
PS9, Line 36: This uses the ExpireSnapshot API to
: * expire snapshots, it calls the ExpireSnapshot.expireOlderThan(timestampMillis) method.
: * TableProperties.MIN_SNAPSHOTS_TO_KEEP table property manages how many snapshots
: * should be retained even when all snapshots are selected by expireOlderThan().
this comment describes the code that is rather in IcebergCatalogOpExecutor and not here in the analyser, right? I'm wondering if it makes sense to keep it here. In case the logic there changes for some reason, there is a risk that this comment will become obsolete.
http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java:
http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java@58
PS9, Line 58: super.analyze(analyzer);
: FeTable table = getTargetTable();
: if (!(table instanceof FeIcebergTable)) {
: throw new AnalysisException("ALTER TABLE EXECUTE ROLLBACK is only supported "
: + "for Iceberg tables: " + table.getTableName());
: }
: analyzeFunctionCallExpr(analyzer, USAGE);
: analyzeParameter(analyzer);
AlterTableExecuteRollbackStmt.analyze() and AlterTableExecuteExpireSnapshotsStmt.analyze() seems pretty similar to me except they call a different function at the end to analyze their parameter.
I'm thinking of a solution where this analyze could be moved to the parent class (AlterTableExecuteStmt) and then the children would implement an abstract function 'analyzeParameter()'. Would this make sense?
http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@199
PS9, Line 199: long snapshotId = params.getSnapshot_id();
: long timestampMillis = params.getTimestamp_millis();
These are optional members of TAlterTableExecuteRollbackParams. The values might be off if not set.
http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@215
PS9, Line 215: manageSnapshots.rollbackTo(snapshotId);
just to be on the safe side let me ask: apart from IllegalStateException this could throw ValidationException as well. We take care it somehow to detect if the given snapshot ID is not an ancestor, right?
http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py
File tests/query_test/test_iceberg.py:
http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py@245
PS9, Line 245: def test_execute_rollback(self, vector, unique_database):
Another use case that might be worth testing is when we set the timezone to something else then do a rollback.
http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py@247
PS9, Line 247: self.run_test_case('QueryTest/iceberg-rollback', vector, use_db=unique_database)
I'd execute these in a separate test, e.g. test_execute_rollback_negative
http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py@271
PS9, Line 271: # We cannot roll back to a snapshot that is not a current ancestor.
This just made me think if there is a way to un-do a rollback if we realise for instance that we rolled back to a wrong snapshot ID.
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Mar 2023 14:46:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 12:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9128/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 12
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Mar 2023 16:44:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 3:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/19002/3/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java:
http://gerrit.cloudera.org:8080/#/c/19002/3/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java@43
PS3, Line 43: TAlterTableExecuteRollbackParams executeParams = new TAlterTableExecuteRollbackParams();
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/19002/3/fe/src/main/java/org/apache/impala/analysis/RollbackSpec.java
File fe/src/main/java/org/apache/impala/analysis/RollbackSpec.java:
http://gerrit.cloudera.org:8080/#/c/19002/3/fe/src/main/java/org/apache/impala/analysis/RollbackSpec.java@41
PS3, Line 41: case TIME: analyzeTimeBased(analyzer, "EXECUTE ROLLBACK(<expression>):"); break; // FIXME how to pass other paren
line too long (119 > 90)
http://gerrit.cloudera.org:8080/#/c/19002/3/fe/src/main/java/org/apache/impala/analysis/RollbackSpec.java@42
PS3, Line 42: case VERSION: analyzeVersionBased(analyzer, "EXECUTE ROLLBACK(<expression>):"); break;
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/19002/3/fe/src/main/java/org/apache/impala/analysis/SnapshotSpec.java
File fe/src/main/java/org/apache/impala/analysis/SnapshotSpec.java:
http://gerrit.cloudera.org:8080/#/c/19002/3/fe/src/main/java/org/apache/impala/analysis/SnapshotSpec.java@58
PS3, Line 58: protected void analyzeTimeBased(Analyzer analyzer, String systemTimeSql) throws AnalysisException {
line too long (101 > 90)
http://gerrit.cloudera.org:8080/#/c/19002/3/fe/src/main/java/org/apache/impala/analysis/SnapshotSpec.java@94
PS3, Line 94: protected void analyzeVersionBased(Analyzer analyzer, String versionSql) throws AnalysisException {
line too long (101 > 90)
http://gerrit.cloudera.org:8080/#/c/19002/3/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/19002/3/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@199
PS3, Line 199: LOG.info(MessageFormat.format("Execute rollback kind={0} snapshotId={1} timestamp={2}",
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/19002/3/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@211
PS3, Line 211: LOG.debug(MessageFormat.format("Trying to rollback iceberg table to snapshot id {0}",
line too long (93 > 90)
http://gerrit.cloudera.org:8080/#/c/19002/3/tests/common/iceberg_test_suite.py
File tests/common/iceberg_test_suite.py:
http://gerrit.cloudera.org:8080/#/c/19002/3/tests/common/iceberg_test_suite.py@44
PS3, Line 44: i
flake8: E501 line too long (115 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19002/3/tests/common/iceberg_test_suite.py@48
PS3, Line 48: u
flake8: E501 line too long (100 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19002/3/tests/common/iceberg_test_suite.py@49
PS3, Line 49: n
flake8: E501 line too long (93 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19002/3/tests/common/iceberg_test_suite.py@56
PS3, Line 56: s
flake8: E501 line too long (106 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19002/3/tests/common/iceberg_test_suite.py@58
PS3, Line 58: D
flake8: E501 line too long (107 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19002/3/tests/common/iceberg_test_suite.py@59
PS3, Line 59: O
flake8: E501 line too long (102 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19002/3/tests/common/iceberg_test_suite.py@71
PS3, Line 71:
flake8: W291 trailing whitespace
http://gerrit.cloudera.org:8080/#/c/19002/3/tests/common/iceberg_test_suite.py@71
PS3, Line 71: assert len(rows.data) == expected_result_size, """got unexpected number of
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/19002/3/tests/common/iceberg_test_suite.py@117
PS3, Line 117: (
flake8: E501 line too long (91 > 90 characters)
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sat, 17 Sep 2022 00:38:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19002/6/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/19002/6/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@73
PS6, Line 73: public static final Logger LOG = LoggerFactory.getLogger(IcebergCatalogOpExecutor.class);
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/19002/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java:
http://gerrit.cloudera.org:8080/#/c/19002/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@4188
PS6, Line 4188: "be an integer type or a timestamp, but is 'DECIMAL(3,2)': EXECUTE rollback(3.14)");
line too long (92 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Sep 2022 18:57:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] chnage zoltan's test 3/3 IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: chnage zoltan's test 3/3 IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 10:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19002/10/tests/util/iceberg_util.py
File tests/util/iceberg_util.py:
http://gerrit.cloudera.org:8080/#/c/19002/10/tests/util/iceberg_util.py@111
PS10, Line 111: class IcebergCatalogs:
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/19002/10/tests/util/iceberg_util.py@126
PS10, Line 126: +
flake8: W504 line break after binary operator
http://gerrit.cloudera.org:8080/#/c/19002/10/tests/util/iceberg_util.py@139
PS10, Line 139:
flake8: W292 no newline at end of file
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Mar 2023 01:49:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 9:
(9 comments)
Comments ahead of next patch (coming soon)
http://gerrit.cloudera.org:8080/#/c/19002/9/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:
http://gerrit.cloudera.org:8080/#/c/19002/9/common/thrift/JniCatalog.thrift@431
PS9, Line 431: 1: required TRollbackType kind
> nit: this could work without the 'kind' member by simply checking which one
Thanks, you're right, but this seems robust and extensible.
http://gerrit.cloudera.org:8080/#/c/19002/9/common/thrift/JniCatalog.thrift@434
PS9, Line 434: 2: optional i64 timestamp_millis
> Kind of unrelated and I might be totally off here, but the other day I saw
Yes, if I understand it right, the 'timestamp" here is a really milliseconds, that Impala timestamp is nanoseconds and so needs more space.
http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java:
http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java@36
PS9, Line 36: This uses the ExpireSnapshot API to
: * expire snapshots, it calls the ExpireSnapshot.expireOlderThan(timestampMillis) method.
: * TableProperties.MIN_SNAPSHOTS_TO_KEEP table property manages how many snapshots
: * should be retained even when all snapshots are selected by expireOlderThan().
> this comment describes the code that is rather in IcebergCatalogOpExecutor
Done in next patch
http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java:
http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java@58
PS9, Line 58: super.analyze(analyzer);
: FeTable table = getTargetTable();
: if (!(table instanceof FeIcebergTable)) {
: throw new AnalysisException("ALTER TABLE EXECUTE ROLLBACK is only supported "
: + "for Iceberg tables: " + table.getTableName());
: }
: analyzeFunctionCallExpr(analyzer, USAGE);
: analyzeParameter(analyzer);
> AlterTableExecuteRollbackStmt.analyze() and AlterTableExecuteExpireSnapshot
Nice idea, that *is* the purpose of AlterTableExecuteStmt. I tried this, but the differing USAGE strings made it untidy.
So maybe keep it like this, we'll know better when there is a third EXECUTE statement.
http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@199
PS9, Line 199: long snapshotId = params.getSnapshot_id();
: long timestampMillis = params.getTimestamp_millis();
> These are optional members of TAlterTableExecuteRollbackParams. The values
I added some Preconditions.check() calls in the next patch
http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@215
PS9, Line 215: manageSnapshots.rollbackTo(snapshotId);
> just to be on the safe side let me ask: apart from IllegalStateException th
This is tested in test_execute_rollback, the output we get is:
Query aborted:ValidationException: Cannot roll back to snapshot, not an ancestor of the current state: 3922115053442881239
Is that sufficient do you think?
http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py
File tests/query_test/test_iceberg.py:
http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py@245
PS9, Line 245: def test_execute_rollback(self, vector, unique_database):
> Another use case that might be worth testing is when we set the timezone to
There is an example in iceberg_rollback.test
http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py@247
PS9, Line 247: self.run_test_case('QueryTest/iceberg-rollback', vector, use_db=unique_database)
> I'd execute these in a separate test, e.g. test_execute_rollback_negative
Done in next patch set.
http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py@271
PS9, Line 271: # We cannot roll back to a snapshot that is not a current ancestor.
> This just made me think if there is a way to un-do a rollback if we realise
I think you cannot do this.
This is case where having a branch would help, if you remember to make the branch before doing the rollback.
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Mar 2023 23:56:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Gabor Kaszab (Code Review)" <ge...@cloudera.org>.
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 11: Code-Review+2
(1 comment)
Thanks again for taking care of this, Andrew!
http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@215
PS9, Line 215: case VERSION_ID:
> This is tested in test_execute_rollback, the output we get is:
sure, thanks!
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Mar 2023 11:58:36 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 8:
(7 comments)
Thank for the useful comments
http://gerrit.cloudera.org:8080/#/c/19002/7/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:
http://gerrit.cloudera.org:8080/#/c/19002/7/common/thrift/JniCatalog.thrift@416
PS7, Line 416: TAlterTableExecuteExpireSnaps
> I think this should be
Thanks, that is better
http://gerrit.cloudera.org:8080/#/c/19002/7/common/thrift/JniCatalog.thrift@505
PS7, Line 505: // Parameters for ALTER TABLE EXECUTE operations
> nit: // Parameters for ALTER TABLE EXECUTE ... operations.
Thanks
http://gerrit.cloudera.org:8080/#/c/19002/7/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java:
http://gerrit.cloudera.org:8080/#/c/19002/7/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java@74
PS7, Line 74: USAGE + " <expression> must be a constant expression: EXECUTE " + toSql());
: }
: if ((fnParamValue_ instanceof LiteralExpr)
: && (fnParamValue_.getType().isIntegerType())) {
: // Parameter is a snapshot id
: kind_ = TRollbackType.VERSION_ID;
: snapshotVersion_ = fnParamValue_.evalToInteger(analyzer, USAGE);
: if (snapshotVersion_ < 0) {
: throw new AnalysisException("Invalid version number has been given to " + USAGE
: + ": " + snapshotVersion_);
: }
: LOG.debug(USAGE + " version: " + snapshotVersion_);
: } else {
: Expr timestampEpr = getParamConvertibleToTimestamp();
: if (timestampEpr != null) {
: // Parameter is a timestamp.
: kind_ = TRollbackType.TIME_ID;
: try {
: olderThanMillis_ =
: ExprUtil.localTimestampT
> The control flow is kinda messy here. Is being decimal important at all? Fa
Thanks for the thoughtful comment.
Yes this is better
http://gerrit.cloudera.org:8080/#/c/19002/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/19002/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1338
PS7, Line 1338:
> Shouldn't be this method be modified to alterTableExecuteExpireSnapshots to
yes, thanks
http://gerrit.cloudera.org:8080/#/c/19002/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1343
PS7, Line 1343:
> nit: Maybe this is a bit more telling for debugging purposes.
That is better thanks
http://gerrit.cloudera.org:8080/#/c/19002/7/testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback.test
File testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback.test:
http://gerrit.cloudera.org:8080/#/c/19002/7/testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback.test@18
PS7, Line 18: ROLLBACK('1111')
: ---- CATCH
: An invalid TIMESTAMP express
> This is what I meant with weird error message, if I want to rollback to my
Thanks I have made it understandable by humans I hope
http://gerrit.cloudera.org:8080/#/c/19002/7/tests/common/iceberg_test_suite.py
File tests/common/iceberg_test_suite.py:
http://gerrit.cloudera.org:8080/#/c/19002/7/tests/common/iceberg_test_suite.py@105
PS7, Line 105: """Construct a snapshot from a
> Could you add here the expected format of the raw string in a comment?
Done
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Oct 2022 17:48:45 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 9:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py
File tests/query_test/test_iceberg.py:
http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py@41
PS9, Line 41: from tests.util.iceberg_util import cast_ts, quote, parse_timestamp, get_snapshots
flake8: F401 'tests.util.iceberg_util.parse_timestamp' imported but unused
http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py@231
PS9, Line 231: r
flake8: F841 local variable 'result' is assigned to but never used
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Mar 2023 19:59:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/11506/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Sep 2022 19:27:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Hello Tamas Mate, Gabor Kaszab, Zoltan Borok-Nagy, Gergely Fürnstáhl, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19002
to look at the new patch set (#5).
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Iceberg table modifications cause new table snapshots to be created;
these snapshots represent an earlier version of the table. The Iceberg
API provides a way to rollback the table to a previous snapshot.
This change add the ability to execute a rollback on Iceberg tables
using the following statements:
- ALTER TABLE <tbl> EXECUTE ROLLBACK(<snapshot id>)
- ALTER TABLE <tbl> EXECUTE ROLLBACK('<timestamp>')
The latter form of the command rolls back to the the latest snapshot
that has a creation timestamp that is older than the specified
timestamp.
Note that when a table is rolled back to a snapshot, a new snapshot is
created with the same snapshot id, but with a new creation timestamp.
Moved the logic that deals with id and timestamps from TimeTravelSpec
into a new parent class 'SnapshotSpec'. This allows the logic to be
reused in a new subclass. A few names were changed to be less specific
to the "AS OF" support.
Testing:
- Added analysis unit tests.
- Added e2e tests.
- Added a Snapshot class to the pytests to encapsulate the output of
DESCRIBE HISTORY.
Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
---
M common/thrift/JniCatalog.thrift
M fe/src/main/cup/sql-parser.cup
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
A fe/src/main/java/org/apache/impala/analysis/RollbackSpec.java
A fe/src/main/java/org/apache/impala/analysis/SnapshotSpec.java
M fe/src/main/java/org/apache/impala/analysis/TableRef.java
M fe/src/main/java/org/apache/impala/analysis/TimeTravelSpec.java
M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java
M fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
M fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/IcebergUtil.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
A fe/src/test/java/org/apache/impala/analysis/SnapshotSpecTest.java
A testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback.test
M tests/common/iceberg_test_suite.py
M tests/query_test/test_iceberg.py
19 files changed, 684 insertions(+), 174 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/19002/5
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/11375/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sat, 17 Sep 2022 18:15:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] chnage zoltan's test 3/3 IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Hello Tamas Mate, Gabor Kaszab, Zoltan Borok-Nagy, Gergely Fürnstáhl, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19002
to look at the new patch set (#10).
Change subject: chnage zoltan's test 3/3 IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
chnage zoltan's test 3/3
IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Iceberg table modifications cause new table snapshots to be created;
these snapshots represent an earlier version of the table. The Iceberg
API provides a way to rollback the table to a previous snapshot.
This change adds the ability to execute a rollback on Iceberg tables
using the following statements:
- ALTER TABLE <tbl> EXECUTE ROLLBACK(<snapshot id>)
- ALTER TABLE <tbl> EXECUTE ROLLBACK('<timestamp>')
The latter form of the command rolls back to the most recent snapshot
that has a creation timestamp that is older than the specified
timestamp.
Note that when a table is rolled back to a snapshot, a new snapshot is
created with the same snapshot id, but with a new creation timestamp.
Testing:
- Added analysis unit tests.
- Added e2e tests.
- Converted test_time_travel to use get_snapshots() from iceberg_util.
- Add a utility class to allow pytests to create tables with various
iceberg catalogs.
Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
---
M common/thrift/JniCatalog.thrift
M fe/src/main/cup/sql-parser.cup
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteStmt.java
M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java
M fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
A testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback-negative.test
M tests/custom_cluster/test_events_custom_configs.py
M tests/query_test/test_iceberg.py
M tests/util/iceberg_util.py
15 files changed, 676 insertions(+), 203 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/19002/10
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 9:
(1 comment)
The big change here is that I set
needsToUpdateHms = false;
for the EXECUTE cases in alterIcebergTable().
This is because otherwise if the catalog is not a a hive catalog we will fail trying to update hms. I added some test cases for this that use different catalogs, copying the pattern from test_iceberg_self_event().
http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py
File tests/query_test/test_iceberg.py:
http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py@245
PS9, Line 245: def test_execute_rollback(self, vector, unique_database):
> There is an example in iceberg_rollback.test
But you are right I needed a better test, I have added this in the latest PS
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Mar 2023 02:01:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 12: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 12
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Mar 2023 22:00:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19002/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/19002/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1336
PS8, Line 1336: String expireSummary = IcebergCatalogOpExecutor.alterTableExecuteExpireSnapshots(
line too long (93 > 90)
http://gerrit.cloudera.org:8080/#/c/19002/8/tests/common/iceberg_test_suite.py
File tests/common/iceberg_test_suite.py:
http://gerrit.cloudera.org:8080/#/c/19002/8/tests/common/iceberg_test_suite.py@109
PS8, Line 109: 2020-08-30 22:58:08.440000000 8270633197658268308 NULL TRUE
tab used for whitespace
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 05 Oct 2022 17:49:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 6:
(7 comments)
Thanks for the review comments.
This change is blocked waiting for https://github.com/apache/iceberg/issues/5882 which we need to include the rollback in our iceberg transaction.
http://gerrit.cloudera.org:8080/#/c/19002/5/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:
http://gerrit.cloudera.org:8080/#/c/19002/5/common/thrift/JniCatalog.thrift@428
PS5, Line 428: // Is rollback to a date or snapshot id.
: 1: required TRollbackType kind
:
: // If kind is TIME_ID this is the date to rollback to.
: 2: optional i64 timestamp_millis
:
: // If kind is VERSION_ID this is the id to rollback to.
: 3: optional i64 snapshot_id
: }
:
: /
> Just an idea: can we embed this under TAlterTableExecuteParams?
Thanks, I had to rework but this does make the change better and will allow 'EXECUTE' functions to be added more easily.
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/cup/sql-parser.cup@1323
PS5, Line 1323:
> And maybe this could be 'AlterTableExecuteStmt.createExecuteStmt(table, exp
Done
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java:
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java@34
PS5, Line 34:
> nit: shouldn't we name it to 'rollbackSpec'?
yes!
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/analysis/TableRef.java
File fe/src/main/java/org/apache/impala/analysis/TableRef.java:
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/analysis/TableRef.java@478
PS5, Line 478:
> nit: indentation is off
Done
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1336
PS5, Line 1336:
> This will have the same problem as https://gerrit.cloudera.org/#/c/19036/
This not done. I believe we need https://github.com/apache/iceberg/issues/5882 to do this.
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@199
PS5, Line 199: "Execute rollback "
> Do we need this MessagaFormat.format here?
Thanks, yes I have to change
LOG = Logger.getLogger to
LOG = LoggerFactory.getLogger
to get this to work (with {})
http://gerrit.cloudera.org:8080/#/c/19002/5/tests/query_test/test_iceberg.py
File tests/query_test/test_iceberg.py:
http://gerrit.cloudera.org:8080/#/c/19002/5/tests/query_test/test_iceberg.py@258
PS5, Line 258: # We rolled back, but that creates a new snapshot, so now there are 4.
: snapshots = self.get_snapshots(self.client, tbl_name, expected_result_size=4)
> It's interesting that non-ancestor snapshots are still in the output of DES
This is just what Iceberg does.
But it makes sense to me as it makes the rollback non-destructive
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Sep 2022 18:57:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/11505/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Sep 2022 19:18:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 5:
(6 comments)
Looks great! Left a couple of comments.
http://gerrit.cloudera.org:8080/#/c/19002/5/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:
http://gerrit.cloudera.org:8080/#/c/19002/5/common/thrift/JniCatalog.thrift@428
PS5, Line 428: // Parameters for ALTER TABLE EXECUTE ROLLBACK operations.
: struct TAlterTableExecuteRollbackParams {
: // Is rollback to a date or snapshot id.
: 1: required TRollbackType kind
:
: // If kind is TIME_ID this is the date to rollback to.
: 2: optional i64 timestamp_millis
:
: // If kind is VERSION_ID this is the id to rollback to.
: 3: optional i64 snapshot_id
: }
Just an idea: can we embed this under TAlterTableExecuteParams?
E.g. TAlterTableExecuteParams could look like:
struct TAlterTableExecuteParams {
1: optional TExecuteExpireSnapshotsParams
2: optional TExecuteRollbackParams
}
This way we wouldn't need to add a new field to TAlterTableParams whenever we invent a new EXECUTE command.
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/cup/sql-parser.cup@1323
PS5, Line 1323: new AlterTableExecuteStmt(table, expr);
And maybe this could be 'AlterTableExecuteStmt.createExecuteStmt(table, expr);'
And it could return either AlterTableExecuteRollbackStmt or AlterTableExecuteExpireSnapshotsStmt based on the name of the function. This way we also don't need KW_ROLLBACK.
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java:
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java@34
PS5, Line 34: timeTravelSpec
nit: shouldn't we name it to 'rollbackSpec'?
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/analysis/TableRef.java
File fe/src/main/java/org/apache/impala/analysis/TableRef.java:
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/analysis/TableRef.java@478
PS5, Line 478:
nit: indentation is off
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@199
PS5, Line 199: MessageFormat.format(
Do we need this MessagaFormat.format here?
log4j.logger already provides these methods with formatting capabilities, i.e. it could be just:
LOG.info("Execute rollback " +
"kind={0} snapshotId={1} timestamp={2}", kind, snapshotId, timestampMillis);
http://gerrit.cloudera.org:8080/#/c/19002/5/tests/query_test/test_iceberg.py
File tests/query_test/test_iceberg.py:
http://gerrit.cloudera.org:8080/#/c/19002/5/tests/query_test/test_iceberg.py@258
PS5, Line 258: # We rolled back, but that creates a new snapshot, so now there are 4.
: snapshots = self.get_snapshots(self.client, tbl_name, expected_result_size=4)
It's interesting that non-ancestor snapshots are still in the output of DESCRIBE HISTORY, but we cannot rollback to them, not even by snapshot id.
I checked Hive and it works the same so it should be OK.
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Sep 2022 11:50:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12577/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Mar 2023 02:18:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1336
PS5, Line 1336: tbl
This will have the same problem as https://gerrit.cloudera.org/#/c/19036/
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Sep 2022 14:50:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Gergely Fürnstáhl (Code Review)" <ge...@cloudera.org>.
Gergely Fürnstáhl has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 8: Code-Review+1
Thanks for working on this, looks good to me!
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Oct 2022 10:23:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Gergely Fürnstáhl (Code Review)" <ge...@cloudera.org>.
Gergely Fürnstáhl has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 7:
(7 comments)
Generally looks good to me, added a few small comments
http://gerrit.cloudera.org:8080/#/c/19002/7/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:
http://gerrit.cloudera.org:8080/#/c/19002/7/common/thrift/JniCatalog.thrift@416
PS7, Line 416: TExecuteExpireSnapshotsParams
I think this should be
TAlterTableExecuteExpireSnapshotsParams
http://gerrit.cloudera.org:8080/#/c/19002/7/common/thrift/JniCatalog.thrift@505
PS7, Line 505: // Parameters for ALTER TABLE EXECUTE EXPIRE_SNAPSHOTS
nit: // Parameters for ALTER TABLE EXECUTE ... operations.
http://gerrit.cloudera.org:8080/#/c/19002/7/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java:
http://gerrit.cloudera.org:8080/#/c/19002/7/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java@74
PS7, Line 74: if (fnParamValue_.getType().isIntegerType()) {
: // Parameter is a snapshot id
: kind_ = TRollbackType.VERSION_ID;
: snapshotVersion_ = fnParamValue_.evalToInteger(analyzer, USAGE);
: if (snapshotVersion_ < 0) {
: throw new AnalysisException("Invalid version number has been given to " + USAGE
: + ": " + snapshotVersion_);
: }
: LOG.debug(USAGE + " version: " + snapshotVersion_);
: return;
: } else if (fnParamValue_.getType().isDecimal()) {
: throw new AnalysisException(USAGE
: + " <expression> must be an integer type or a timestamp, but is '"
: + fnParamValue_.getType() + "': EXECUTE " + toSql());
: }
: }
:
: // Parameter is a timestamp
: kind_ = TRollbackType.TIME_ID;
: analyzeOlderThan(analyzer, USAGE);
The control flow is kinda messy here. Is being decimal important at all? Falling back to parsing it as timestamp could produce hard to understand error messages too.
Maybe something like this would be easier to follow/maintain/extend it later:
if(literal && integer) { ... }
else if(covertibleToTimestamp()) { ... } // this could be extracted from analyzeOlderThan
else { ... } // not supported, must be integer or timestamp
http://gerrit.cloudera.org:8080/#/c/19002/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/19002/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1338
PS7, Line 1338: alterTableExecute
Shouldn't be this method be modified to alterTableExecuteExpireSnapshots too?
http://gerrit.cloudera.org:8080/#/c/19002/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1343
PS7, Line 1343: Alter table execute has new format.
nit: Maybe this is a bit more telling for debugging purposes.
"Alter table execute statement is not implemented."
http://gerrit.cloudera.org:8080/#/c/19002/7/testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback.test
File testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback.test:
http://gerrit.cloudera.org:8080/#/c/19002/7/testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback.test@18
PS7, Line 18: ROLLBACK('1111')
: ---- CATCH
: Invalid TIMESTAMP expression
This is what I meant with weird error message, if I want to rollback to my id=1111 snapshot, but Impala is telling its not a valid timestamp, hard to find that the mistake is the added ' '.
http://gerrit.cloudera.org:8080/#/c/19002/7/tests/common/iceberg_test_suite.py
File tests/common/iceberg_test_suite.py:
http://gerrit.cloudera.org:8080/#/c/19002/7/tests/common/iceberg_test_suite.py@105
PS7, Line 105: parts = raw_string.split("\t")
Could you add here the expected format of the raw string in a comment?
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 03 Oct 2022 13:53:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 10:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/12576/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Mar 2023 02:08:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
Patch Set 12: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 12
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 Mar 2023 16:44:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Iceberg table modifications cause new table snapshots to be created;
these snapshots represent an earlier version of the table. The Iceberg
API provides a way to rollback the table to a previous snapshot.
This change adds the ability to execute a rollback on Iceberg tables
using the following statements:
- ALTER TABLE <tbl> EXECUTE ROLLBACK(<snapshot id>)
- ALTER TABLE <tbl> EXECUTE ROLLBACK('<timestamp>')
The latter form of the command rolls back to the most recent snapshot
that has a creation timestamp that is older than the specified
timestamp.
Note that when a table is rolled back to a snapshot, a new snapshot is
created with the same snapshot id, but with a new creation timestamp.
Testing:
- Added analysis unit tests.
- Added e2e tests.
- Converted test_time_travel to use get_snapshots() from iceberg_util.
Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
---
M common/thrift/JniCatalog.thrift
M fe/src/main/cup/sql-parser.cup
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteStmt.java
M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java
M fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
A testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback.test
M tests/query_test/test_iceberg.py
13 files changed, 540 insertions(+), 141 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/19002/9
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Hello Tamas Mate, Gabor Kaszab, Zoltan Borok-Nagy, Gergely Fürnstáhl, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19002
to look at the new patch set (#4).
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
......................................................................
IMPALA-11482: Alter Table Execute Rollback for Iceberg tables.
Iceberg table modifications cause new table snapshots to be created;
these snapshots represent an earlier version of the table. The Iceberg
API provides a way to rollback the table to a previous snapshot.
This change add the ability to execute a rollback on Iceberg tables
using the following statements:
- ALTER TABLE <tbl> EXECUTE ROLLBACK(<snapshot id>)
- ALTER TABLE <tbl> EXECUTE ROLLBACK('<timestamp>')
The latter form of the command rolls back to the the latest snapshot
that has a creation timestamp that is older than the specified
timestamp.
Note that when a table is rolled back to a snapshot, a new snapshot is
created with the same snapshot id, but with a new creation timestamp.
Moved the logic that deals with id and timestamps from TimeTravelSpec
into a new parent class 'SnapshotSpec'. This allows the logic to be
reused in a new subclass. A few names were changed to be less specific
to the "AS OF" support.
Testing:
- Added analysis unit tests.
- Added e2e tests.
- Added a Snapshot class to the pytests to encapsulate the output of
DESCRIBE HISTORY.
Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
---
M common/thrift/JniCatalog.thrift
M fe/src/main/cup/sql-parser.cup
A fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
A fe/src/main/java/org/apache/impala/analysis/RollbackSpec.java
A fe/src/main/java/org/apache/impala/analysis/SnapshotSpec.java
M fe/src/main/java/org/apache/impala/analysis/TableRef.java
M fe/src/main/java/org/apache/impala/analysis/TimeTravelSpec.java
M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java
M fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java
M fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/IcebergUtil.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
A fe/src/test/java/org/apache/impala/analysis/SnapshotSpecTest.java
A testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback.test
M tests/common/iceberg_test_suite.py
M tests/query_test/test_iceberg.py
19 files changed, 667 insertions(+), 174 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/19002/4
--
To view, visit http://gerrit.cloudera.org:8080/19002
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9
Gerrit-Change-Number: 19002
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>