Skip to content

Fix trino-cli to display plans for EXPLAIN ANALYZE for update statements #13907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
findepi merged 1 commit into from
Sep 27, 2022
Merged

Fix trino-cli to display plans for EXPLAIN ANALYZE for update statements #13907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions client/trino-cli/src/main/java/io/trino/cli/Query.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,12 @@ private boolean renderQueryOutput(Terminal terminal, PrintStream out, PrintStrea
if (client.isRunning() || (client.isFinished() && client.finalStatusInfo().getError() == null)) {
QueryStatusInfo results = client.isRunning() ? client.currentStatusInfo() : client.finalStatusInfo();
if (results.getUpdateType() != null) {
renderUpdate(errorChannel, results);
renderUpdate(errorChannel, results, outputFormat, usePager);
Copy link
Copy Markdown
Member

@nineinchnick nineinchnick Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the server not set the update type in results if it's an explain query?

Copy link
Copy Markdown
Member Author

@homar homar Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this, see my comment below with link to a pr that has links to 2 others :D I did 3 different approaches but none of them seems good enough

}
// TODO once https://github.com/trinodb/trino/issues/14253 is done this else here should be needed
// and should be replaced with just simple:
// if there is updateCount print it
// if there are columns(resultSet) then print it
else if (results.getColumns() == null) {
errorChannel.printf("Query %s has no columns\n", results.getId());
return false;
Expand Down Expand Up @@ -220,14 +224,21 @@ private void processInitialStatusUpdates(WarningsPrinter warningsPrinter)
warningsPrinter.print(warnings, false, true);
}

private void renderUpdate(PrintStream out, QueryStatusInfo results)
private void renderUpdate(PrintStream out, QueryStatusInfo results, OutputFormat outputFormat, boolean usePager)
{
String status = results.getUpdateType();
if (results.getUpdateCount() != null) {
long count = results.getUpdateCount();
status += format(": %s row%s", count, (count != 1) ? "s" : "");
out.println(status);
}
else if (results.getColumns() != null && !results.getColumns().isEmpty()) {
out.println(status);
renderResults(out, outputFormat, usePager, results.getColumns());
Copy link
Copy Markdown
Member

@findepi findepi Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why go here only when updateCount is null?

Copy link
Copy Markdown
Member

@findepi findepi Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for example -- after this change -- for CTAS we print number of rows written:

trino:default> CREATE TABLE t AS TABLE tpch.tiny.nation;
CREATE TABLE: 25 rows

but for EXPLAIN ANALYZE CTAS we print only the operation name "CREATE TABLE", without the row count:

trino:default> EXPLAIN ANALYZE CREATE TABLE t AS TABLE tpch.tiny.nation;
CREATE TABLE
<query plan>

The query plan is correctly printed, but row count should be too

Copy link
Copy Markdown
Member Author

@homar homar Sep 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a code comment, mainly to avoid printing same information twice as in your comment above #13907 (comment)

}
else {
out.println(status);
}
out.println(status);
discardResults();
Copy link
Copy Markdown
Member

@nineinchnick nineinchnick Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should still be called but after the call to both renderUpdate and renderResults, on line 168. If results are not consumed, queries can get stuck in the FINISHING phase. Also, there are no other calls to discardResults.

Copy link
Copy Markdown
Member Author

@homar homar Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm ok, I will give it a look thanks

Copy link
Copy Markdown
Member

@findepi findepi Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nineinchnick after renderResults completes, we shouldn't need to do discardResults, should we?

Copy link
Copy Markdown
Member

@findepi findepi Sep 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nineinchnick thoughts?

Copy link
Copy Markdown
Member

@nineinchnick nineinchnick Sep 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

}

Expand Down
5 changes: 1 addition & 4 deletions core/trino-main/src/main/java/io/trino/server/protocol/Query.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
import io.trino.spi.block.BlockEncodingSerde;
import io.trino.spi.exchange.ExchangeId;
import io.trino.spi.security.SelectedRole;
import io.trino.spi.type.BooleanType;
import io.trino.spi.type.Type;
import io.trino.transaction.TransactionId;

Expand Down Expand Up @@ -507,13 +506,11 @@ private synchronized QueryResults getNextResult(long token, UriInfo uriInfo, Dat

private synchronized QueryResultRows removePagesFromExchange(QueryInfo queryInfo, long targetResultBytes)
{
// For queries with no output, return a fake boolean result for clients that require it.
if (!resultsConsumed && queryInfo.getOutputStage().isEmpty()) {
return queryResultRowsBuilder(session)
.withSingleBooleanValue(createColumn("result", BooleanType.BOOLEAN, supportsParametricDateTime), true)
.withColumnsAndTypes(ImmutableList.of(), ImmutableList.of())
Copy link
Copy Markdown
Member

@nineinchnick nineinchnick Sep 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure, could this affect older versions of Trino CLI or other clients?

Copy link
Copy Markdown
Member

@nineinchnick nineinchnick Sep 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the changes in the Trino CLI don't require these, should they be in separate commits? If they do, would the Trino CLI still work correctly with older Trino servers?

Copy link
Copy Markdown
Member Author

@homar homar Sep 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we run tests against older versions and they work so I think we are fine

Copy link
Copy Markdown
Member

@hashhar hashhar Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seems to affect other clients though probably - see trinodb/trino-python-client#212 (comment)

Copy link
Copy Markdown
Member

@hashhar hashhar Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, PR title could be probably better worded (and release notes as well) to mention we no longer return result sets for queries which don't return output (which is correct behaviour but older code could be relying on this).

Copy link
Copy Markdown
Member

@hashhar hashhar Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. The comment was meant more to the reviewers. Thanks for the offer - if you want to and feel comfortable with that sure but don't feel responsible for it.

Copy link
Copy Markdown
Member

@hashhar hashhar Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @findepi probably

Copy link
Copy Markdown
Member Author

@homar homar Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel responsible but not comfortable, what should I do in such a case ?

Copy link
Copy Markdown
Member

@findepi findepi Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hashhar you know i strive to make PR titles as informative as I can be, but it's also not possible to fit the whole story in that line
the biggest miss is that we broke the Python client, unknowingly.

Would it make sense to have some Python client comaptibility smoke tests within this repo?
This would supplement the ones maintained in the Python client repo. That ones cover current Py client ws current and old servers.
Those new ones would cover current server against older clients.

Copy link
Copy Markdown
Member

@hashhar hashhar Sep 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel responsible but not comfortable, what should I do in such a case ?

No problem. I'd request a review to make sure you can verify the functional changes there match what we did for the JDBC client here. @homar

the biggest miss is that we broke the Python client, unknowingly.

@findepi The Python client isn't my biggest concern - my main concern is this is a user-facing change where if someone wrote code relying on DDL queries as well returning results then they'd be affected (even though it'd be wrong to rely on that in the first place).

We can consider adding a smoke test here that tests current server against current client only so that at least we can ensure that there's at-least 1 working Python (and Go) client for each Trino server release.

I'll create a separate issue about such tests and let's continue here.

.build();
}

// Remove as many pages as possible from the exchange until just greater than DESIRED_RESULT_BYTES
// NOTE: it is critical that query results are created for the pages removed from the exchange
// client while holding the lock because the query may transition to the finished state when the
Expand Down
35 changes: 23 additions & 12 deletions plugin/trino-blackhole/src/test/java/io/trino/plugin/blackhole/TestBlackHoleSmoke.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import io.trino.testing.MaterializedResult;
import io.trino.testing.MaterializedRow;
import io.trino.testing.QueryRunner;
import org.intellij.lang.annotations.Language;
import org.testng.annotations.AfterTest;
import org.testng.annotations.BeforeTest;
import org.testng.annotations.Test;
Expand Down Expand Up @@ -75,8 +76,8 @@ public void testCreateSchema()
assertEquals(queryRunner.execute("SHOW SCHEMAS FROM blackhole").getRowCount(), 3);
assertThatQueryReturnsValue("CREATE TABLE test.test_schema as SELECT * FROM tpch.tiny.region", 5L);

assertThatQueryReturnsValue("DROP TABLE test_schema", true);
assertThatQueryReturnsValue("DROP TABLE test.test_schema", true);
assertThatQueryDoesNotReturnValues("DROP TABLE test_schema");
assertThatQueryDoesNotReturnValues("DROP TABLE test.test_schema");
}

@Test
Expand All @@ -87,7 +88,7 @@ public void createTableWhenTableIsAlreadyCreated()
assertThatThrownBy(() -> queryRunner.execute(createTableSql))
.isInstanceOf(RuntimeException.class)
.hasMessage("line 1:1: Destination table 'blackhole.default.nation' already exists");
assertThatQueryReturnsValue("DROP TABLE nation", true);
assertThatQueryDoesNotReturnValues("DROP TABLE nation");
}

@Test
Expand All @@ -105,7 +106,7 @@ public void blackHoleConnectorUsage()

assertThatQueryReturnsValue("SELECT count(*) FROM nation", 0L);

assertThatQueryReturnsValue("DROP TABLE nation", true);
assertThatQueryDoesNotReturnValues("DROP TABLE nation");
}

@Test
Expand All @@ -125,7 +126,7 @@ public void createTableWithDistribution()
assertThatQueryReturnsValue(
"CREATE TABLE distributed_test WITH ( distributed_on = array['orderkey'] ) AS SELECT * FROM tpch.tiny.orders",
15000L);
assertThatQueryReturnsValue("DROP TABLE distributed_test", true);
assertThatQueryDoesNotReturnValues("DROP TABLE distributed_test");
}

@Test
Expand Down Expand Up @@ -170,7 +171,7 @@ public void dataGenerationUsage()
assertEquals(row.getField(2), 0L);
assertEquals(row.getField(3), "****************");

assertThatQueryReturnsValue("DROP TABLE nation", true);
assertThatQueryDoesNotReturnValues("DROP TABLE nation");
}

@Test
Expand Down Expand Up @@ -201,7 +202,7 @@ public void fieldLength()
assertEquals(row.getField(3), "********");
assertEquals(row.getField(4), "***"); // this one is shorter due to column type being VARCHAR(3)

assertThatQueryReturnsValue("DROP TABLE nation", true);
assertThatQueryDoesNotReturnValues("DROP TABLE nation");
}

@Test
Expand Down Expand Up @@ -261,7 +262,7 @@ public void testSelectWithUnenforcedConstraint()

private void createBlackholeAllTypesTable()
{
assertThatQueryReturnsValue(
assertThatQueryDoesNotReturnValues(
format("CREATE TABLE blackhole_all_types (" +
" _varchar VARCHAR(10)" +
", _bigint BIGINT" +
Expand All @@ -279,13 +280,12 @@ private void createBlackholeAllTypesTable()
") WITH ( %s = 1, %s = 1, %s = 1 ) ",
ROWS_PER_PAGE_PROPERTY,
PAGES_PER_SPLIT_PROPERTY,
SPLIT_COUNT_PROPERTY),
true);
SPLIT_COUNT_PROPERTY));
}

private void dropBlackholeAllTypesTable()
{
assertThatQueryReturnsValue("DROP TABLE IF EXISTS blackhole_all_types", true);
assertThatQueryDoesNotReturnValues("DROP TABLE IF EXISTS blackhole_all_types");
}

@Test
Expand Down Expand Up @@ -318,7 +318,7 @@ public void pageProcessingDelay()
stopwatch.stop();
assertGreaterThan(stopwatch.elapsed(MILLISECONDS), pageProcessingDelay.toMillis());

assertThatQueryReturnsValue("DROP TABLE nation", true);
assertThatQueryDoesNotReturnValues("DROP TABLE nation");
}

private void assertThatNoBlackHoleTableIsCreated()
Expand Down Expand Up @@ -346,4 +346,15 @@ private void assertThatQueryReturnsValue(String sql, Object expected, Session se
assertEquals(value, expected);
assertEquals(Iterables.getOnlyElement(rows).getFieldCount(), 1);
}

private void assertThatQueryDoesNotReturnValues(String sql)
{
assertThatQueryDoesNotReturnValues(queryRunner.getDefaultSession(), sql);
}

private void assertThatQueryDoesNotReturnValues(Session session, @Language("SQL") String sql)
{
MaterializedResult rows = session == null ? queryRunner.execute(sql) : queryRunner.execute(session, sql);
assertEquals(rows.getRowCount(), 0);
}
}
2 changes: 1 addition & 1 deletion plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1026,7 +1026,7 @@ public void testCreatePartitionedTableWithQuotedIdentifierCasing(String columnNa
String tableName = "partitioning_" + randomTableSuffix();
@Language("SQL") String sql = format("CREATE TABLE %s (%s bigint) WITH (partitioning = ARRAY['%s'])", tableName, columnName, partitioningField);
if (success) {
assertThat(query(sql)).matches("VALUES (true)");
assertUpdate(sql);
dropTable(tableName);
}
else {
Expand Down
24 changes: 24 additions & 0 deletions testing/trino-product-tests/src/main/java/io/trino/tests/product/cli/TestTrinoCli.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,30 @@ public void testSetRole()
assertThat(trimLines(trino.readLinesUntilPrompt())).doesNotContain("admin");
}

@Test(groups = CLI, timeOut = TIMEOUT)
public void shouldPrintExplainAnalyzePlan()
throws Exception
{
launchTrinoCliWithServerArgument();
trino.waitForPrompt();
trino.getProcessInput().println("EXPLAIN ANALYZE CREATE TABLE iceberg.default.test_table AS SELECT * FROM hive.default.nation LIMIT 10;");
List<String> lines = trimLines(trino.readLinesUntilPrompt());
// TODO once https://github.com/trinodb/trino/issues/14253 is done this should be assertThat(lines).contains("CREATE TABLE: 1 row", "Query Plan");
assertThat(lines).contains("CREATE TABLE", "Query Plan");
Copy link
Copy Markdown
Member

@findepi findepi Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assertThat(lines).contains("CREATE TABLE", "Query Plan");
assertThat(lines).contains("CREATE TABLE: 10 rows", "Query Plan");

Copy link
Copy Markdown
Member Author

@homar homar Sep 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is hard to do it now due to #14253, but that could be a good enhancement for the future

// TODO once https://github.com/trinodb/trino/issues/14253 is done this should be assertThat(lines).contains("INSERT: 1 row", "Query Plan");
trino.getProcessInput().println("EXPLAIN ANALYZE INSERT INTO iceberg.default.test_table VALUES(100, 'URUGUAY', 3, 'test comment');");
lines = trimLines(trino.readLinesUntilPrompt());
assertThat(lines).contains("INSERT", "Query Plan");
Copy link
Copy Markdown
Member

@findepi findepi Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assertThat(lines).contains("INSERT", "Query Plan");
assertThat(lines).contains("INSERT: 1 row", "Query Plan");

Copy link
Copy Markdown
Member Author

@homar homar Sep 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is hard to do it now due to #14253, but that could be a good enhancement for the future

// TODO once https://github.com/trinodb/trino/issues/14253 is done this should be assertThat(lines).contains("UPDATE: 1 row", "Query Plan");
trino.getProcessInput().println("EXPLAIN ANALYZE UPDATE iceberg.default.test_table SET n_comment = 'testValue 5' WHERE n_nationkey = 100;");
lines = trimLines(trino.readLinesUntilPrompt());
assertThat(lines).contains("UPDATE", "Query Plan");
// TODO once https://github.com/trinodb/trino/issues/14253 is done this should be assertThat(lines).contains("DELETE: 1 row", "Query Plan");
trino.getProcessInput().println("EXPLAIN ANALYZE DELETE FROM iceberg.default.test_table WHERE n_nationkey = 100;");
lines = trimLines(trino.readLinesUntilPrompt());
assertThat(lines).contains("DELETE", "Query Plan");
}

private void launchTrinoCliWithServerArgument(String... arguments)
throws IOException
{
Expand Down
26 changes: 16 additions & 10 deletions testing/trino-testing/src/main/java/io/trino/testing/AbstractTestEngineOnlyQueries.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5315,47 +5315,47 @@ public void testShowSession()
public void testSetSession()
{
MaterializedResult result = computeActual("SET SESSION test_string = 'bar'");
assertTrue((Boolean) getOnlyElement(result).getField(0));
assertNoRelationalResult(result);
assertEquals(result.getSetSessionProperties(), ImmutableMap.of("test_string", "bar"));

result = computeActual(format("SET SESSION %s.connector_long = 999", TESTING_CATALOG));
assertTrue((Boolean) getOnlyElement(result).getField(0));
assertNoRelationalResult(result);
assertEquals(result.getSetSessionProperties(), ImmutableMap.of(TESTING_CATALOG + ".connector_long", "999"));

result = computeActual(format("SET SESSION %s.connector_string = 'baz'", TESTING_CATALOG));
assertTrue((Boolean) getOnlyElement(result).getField(0));
assertNoRelationalResult(result);
assertEquals(result.getSetSessionProperties(), ImmutableMap.of(TESTING_CATALOG + ".connector_string", "baz"));

result = computeActual(format("SET SESSION %s.connector_string = 'ban' || 'ana'", TESTING_CATALOG));
assertTrue((Boolean) getOnlyElement(result).getField(0));
assertNoRelationalResult(result);
assertEquals(result.getSetSessionProperties(), ImmutableMap.of(TESTING_CATALOG + ".connector_string", "banana"));

result = computeActual(format("SET SESSION %s.connector_long = 444", TESTING_CATALOG));
assertTrue((Boolean) getOnlyElement(result).getField(0));
assertNoRelationalResult(result);
assertEquals(result.getSetSessionProperties(), ImmutableMap.of(TESTING_CATALOG + ".connector_long", "444"));

result = computeActual(format("SET SESSION %s.connector_long = 111 + 111", TESTING_CATALOG));
assertTrue((Boolean) getOnlyElement(result).getField(0));
assertNoRelationalResult(result);
assertEquals(result.getSetSessionProperties(), ImmutableMap.of(TESTING_CATALOG + ".connector_long", "222"));

result = computeActual(format("SET SESSION %s.connector_boolean = 111 < 3", TESTING_CATALOG));
assertTrue((Boolean) getOnlyElement(result).getField(0));
assertNoRelationalResult(result);
assertEquals(result.getSetSessionProperties(), ImmutableMap.of(TESTING_CATALOG + ".connector_boolean", "false"));

result = computeActual(format("SET SESSION %s.connector_double = 11.1", TESTING_CATALOG));
assertTrue((Boolean) getOnlyElement(result).getField(0));
assertNoRelationalResult(result);
assertEquals(result.getSetSessionProperties(), ImmutableMap.of(TESTING_CATALOG + ".connector_double", "11.1"));
}

@Test
public void testResetSession()
{
MaterializedResult result = computeActual(getSession(), "RESET SESSION test_string");
assertTrue((Boolean) getOnlyElement(result).getField(0));
assertNoRelationalResult(result);
assertEquals(result.getResetSessionProperties(), ImmutableSet.of("test_string"));

result = computeActual(getSession(), format("RESET SESSION %s.connector_string", TESTING_CATALOG));
assertTrue((Boolean) getOnlyElement(result).getField(0));
assertNoRelationalResult(result);
assertEquals(result.getResetSessionProperties(), ImmutableSet.of(TESTING_CATALOG + ".connector_string"));
}

Expand Down Expand Up @@ -6535,4 +6535,10 @@ private static ZonedDateTime zonedDateTime(String value)
{
return ZONED_DATE_TIME_FORMAT.parse(value, ZonedDateTime::from);
}

private void assertNoRelationalResult(MaterializedResult result)
{
assertThat(result.getMaterializedRows()).isEmpty();
assertThat(result.getTypes()).isEmpty(); // i.e. no columns
}
}
4 changes: 2 additions & 2 deletions testing/trino-tests/src/test/java/io/trino/execution/TestGrantOnSchema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@ public void testValidGrantWithGrantOption(String privilege)
queryRunner.execute(admin, format("GRANT %s ON SCHEMA default TO %s WITH GRANT OPTION", privilege, username));

assertThat(assertions.query(user, "SHOW SCHEMAS FROM local")).matches("VALUES (VARCHAR 'information_schema'), (VARCHAR 'default')");
assertThat(assertions.query(user, format("GRANT %s ON SCHEMA default TO %s", privilege, randomUsername()))).matches("VALUES (BOOLEAN 'TRUE')");
assertThat(assertions.query(user, format("GRANT %s ON SCHEMA default TO %s WITH GRANT OPTION", privilege, randomUsername()))).matches("VALUES (BOOLEAN 'TRUE')");
assertions.query(user, format("GRANT %s ON SCHEMA default TO %s", privilege, randomUsername()));
assertions.query(user, format("GRANT %s ON SCHEMA default TO %s WITH GRANT OPTION", privilege, randomUsername()));
}

@Test(dataProvider = "privileges")
Expand Down
7 changes: 5 additions & 2 deletions testing/trino-tests/src/test/java/io/trino/execution/TestGrantOnTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,12 @@
import org.testng.annotations.Test;

import java.util.EnumSet;
import java.util.Optional;
import java.util.OptionalLong;

import static io.trino.common.Randoms.randomUsername;
import static io.trino.spi.security.PrincipalType.USER;
import static io.trino.testing.QueryAssertions.assertUpdate;
import static io.trino.testing.TestingSession.testSessionBuilder;
import static java.lang.String.format;
import static org.assertj.core.api.Assertions.assertThat;
Expand Down Expand Up @@ -105,8 +108,8 @@ public void testValidGrantWithGrantOption(String privilege)
queryRunner.execute(admin, format("GRANT %s ON TABLE table_one TO %s WITH GRANT OPTION", privilege, username));

assertThat(assertions.query(user, "SHOW TABLES FROM default")).matches("VALUES (VARCHAR 'table_one')");
assertThat(assertions.query(user, format("GRANT %s ON TABLE table_one TO %s", privilege, randomUsername()))).matches("VALUES (BOOLEAN 'TRUE')");
assertThat(assertions.query(user, format("GRANT %s ON TABLE table_one TO %s WITH GRANT OPTION", privilege, randomUsername()))).matches("VALUES (BOOLEAN 'TRUE')");
assertUpdate(queryRunner, user, format("GRANT %s ON TABLE table_one TO %s", privilege, randomUsername()), OptionalLong.empty(), Optional.empty());
assertUpdate(queryRunner, user, format("GRANT %s ON TABLE table_one TO %s WITH GRANT OPTION", privilege, randomUsername()), OptionalLong.empty(), Optional.empty());
}

@Test(dataProvider = "privileges")
Expand Down