Iceberg test improvements around time travel #13823
Conversation
`getCurrentSnapshotId` and `getLatestSnapshotId` were equivalent.
findepi
added
the
no-release-notes
findepi
requested review from
homar,
losipiuk,
ebyhr,
alexjo2144 and
findinpath
Currently, the way how the system tables are implemented makes it impossible for `getTableHandleForExecute` to be executed on a non-DATA table, so it's just a sanity check.
findepi
force-pushed
the
findepi/prevent-iceberg-table-procedures-on-a-snapshot-7358e5
branch
from
cc1e2e9 to
26a59e1
Compare
| assertThatThrownBy(() -> query(session, "ALTER TABLE \"%s@%d\" EXECUTE OPTIMIZE".formatted(tableName, snapshotId))) | ||
| .hasMessage("Cannot execute table procedure OPTIMIZE on old snapshot " + snapshotId); |
There was a problem hiding this comment.
nit: How about assertQueryFails to ensure TrinoException?
There was a problem hiding this comment.
assertThatThrownBy(() -> query(...)) is a pattern we use very widely.
if we want to deprecate it in favor of assertQueryFails, we should have a discussion about this first.
| assertThat(query("SELECT * FROM " + tableName)) | ||
| .matches("VALUES 11, 22"); |
There was a problem hiding this comment.
nit: No change requested. assertQuery looks better for simple query assertion.
There was a problem hiding this comment.
agree. i think i perhaps did this more for consistency reasons.
| public void testExpireSnapshotsSystemTable() | ||
| { | ||
| assertThatThrownBy(() -> query("ALTER TABLE \"nation$files\" EXECUTE EXPIRE_SNAPSHOTS")) | ||
| .hasMessage("This connector does not support table procedures"); |
There was a problem hiding this comment.
I know it is not in the scope of this PR but I find this error message missleading for this particular case.
This connector actually supports table procedures just not on the system tables.
There was a problem hiding this comment.
Adding io.trino.connector.system.SystemTablesMetadata#getTableHandleForExecute with a more meaningful exception message would be a solution.
| @@ -5117,13 +5117,13 @@ public void testModifyingOldSnapshotIsNotPossible() | |||
| assertUpdate(format("INSERT INTO %s VALUES 4,5,6", tableName), 3); | |||
| assertQuery(sessionWithLegacySyntaxSupport, format("SELECT * FROM \"%s@%d\"", tableName, oldSnapshotId), "VALUES 1,2,3"); | |||
| assertThatThrownBy(() -> query(sessionWithLegacySyntaxSupport, format("INSERT INTO \"%s@%d\" VALUES 7,8,9", tableName, oldSnapshotId))) | |||
There was a problem hiding this comment.
nit: There are a few more ." occurrences in the exception messages within IcebergMetadata
This could lead to unexpected state.
findepi
force-pushed
the
findepi/prevent-iceberg-table-procedures-on-a-snapshot-7358e5
branch
from
26a59e1 to
defbf87
Compare
findepi
deleted the
findepi/prevent-iceberg-table-procedures-on-a-snapshot-7358e5
branch
|
I'm guessing |
|
@alexjo2144 correct. |
Extracted from #13636 where they were added thanks to @alexjo2144 's #13636 (comment)