Skip to content
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

Add missing unit tests on Faker queries #24540

Closed
wants to merge 3 commits into from
+59 −50
Closed

Add missing unit tests on Faker queries #24540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b92275
Add missing unit tests on Faker queries
sthandassery Dec 20, 2024
a9f0b51
fixup! Add missing unit tests on Faker queries
sthandassery Dec 20, 2024
f24c3a2
fixup! fixup! Add missing unit tests on Faker queries
sthandassery Dec 20, 2024
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
6 changes: 0 additions & 6 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 @@ -249,12 +249,6 @@ void testCommentOnView()
assertUpdate("DROP VIEW " + viewName);
}

private String getTableComment(String tableName)
{
return (String) computeScalar("SELECT comment FROM system.metadata.table_comments " +
"WHERE catalog_name = CURRENT_CATALOG AND schema_name = CURRENT_SCHEMA AND table_name = '" + tableName + "'");
}

@Test
void testFieldLength()
{
Expand Down
7 changes: 0 additions & 7 deletions ...lake/src/test/java/io/trino/plugin/deltalake/BaseDeltaLakeRegisterTableProcedureTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,13 +319,6 @@ protected String getTableLocation(String tableName)
throw new IllegalStateException("Location not found in SHOW CREATE TABLE result");
}

private String getTableComment(String tableName)
{
return (String) computeScalar(format(
"SELECT comment FROM system.metadata.table_comments WHERE catalog_name = CURRENT_CATALOG AND schema_name = CURRENT_SCHEMA AND table_name = '%s'",
tableName));
}

protected HiveMetastore metastore()
{
return TestingDeltaLakeUtils.getConnectorService(getQueryRunner(), HiveMetastoreFactory.class)
Expand Down
25 changes: 15 additions & 10 deletions plugin/trino-faker/src/main/java/io/trino/plugin/faker/FakerMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package io.trino.plugin.faker;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
Copy link
Member

Choose a reason for hiding this comment

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

Add missing unit tests on Faker queries

The commit message is misleading when it changes the product code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with it, because unit tests forced the fixes on product code.

Do you have any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

The commit message should say what behavior was fixed. If there are any additional tests, you could extract them into a separate commit. This could make reviewing easier, but it's optional.

I assume you started with the intention of only adding tests, and this is expressed in the current commit message. Then noticed the code needs to be fixed, so you did that to make the tests passed. You should update the commit message to reflect the end result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the discussion, I think it better to extract it to separate commits. Closing this issue for now.

import com.google.errorprone.annotations.concurrent.GuardedBy;
import io.airlift.slice.Slice;
import io.trino.spi.TrinoException;
Expand Down Expand Up @@ -59,8 +60,6 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static com.google.common.base.Verify.verify;
import static com.google.common.collect.ImmutableList.toImmutableList;
Expand Down Expand Up @@ -223,8 +222,9 @@ public synchronized void renameTable(ConnectorSession session, ConnectorTableHan
FakerTableHandle handle = (FakerTableHandle) tableHandle;
SchemaTableName oldTableName = handle.schemaTableName();

TableInfo oldInfo = tables.get(oldTableName);
tables.remove(oldTableName);
tables.put(newTableName, tables.get(oldTableName));
tables.put(newTableName, oldInfo);
Copy link
Member

Choose a reason for hiding this comment

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

I also fixed this in #24242, please rebase on master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, will rebase

}

@Override
Expand All @@ -234,13 +234,18 @@ public synchronized void setTableProperties(ConnectorSession session, ConnectorT
SchemaTableName tableName = handle.schemaTableName();

TableInfo oldInfo = tables.get(tableName);
Map<String, Object> newProperties = Stream.concat(
oldInfo.properties().entrySet().stream()
.filter(entry -> !properties.containsKey(entry.getKey())),
properties.entrySet().stream()
.filter(entry -> entry.getValue().isPresent()))
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
tables.put(tableName, oldInfo.withProperties(newProperties));
ImmutableMap.Builder updatedProperties = ImmutableMap.<String, Object>builder().putAll(oldInfo.properties());
if (properties.containsKey(TableInfo.NULL_PROBABILITY_PROPERTY)) {
double nullProbability = (double) properties.get(TableInfo.NULL_PROBABILITY_PROPERTY)
.orElseThrow(() -> new IllegalArgumentException("The null_probability property cannot be empty"));
updatedProperties.put(TableInfo.NULL_PROBABILITY_PROPERTY, nullProbability);
}
if (properties.containsKey(TableInfo.DEFAULT_LIMIT_PROPERTY)) {
long defaultLimit = (long) properties.get(TableInfo.DEFAULT_LIMIT_PROPERTY)
.orElseThrow(() -> new IllegalArgumentException("The default_limit property cannot be empty"));
updatedProperties.put(TableInfo.DEFAULT_LIMIT_PROPERTY, defaultLimit);
}
tables.put(tableName, oldInfo.withProperties(updatedProperties.buildOrThrow()));
}

@Override
Expand Down
28 changes: 28 additions & 0 deletions plugin/trino-faker/src/test/java/io/trino/plugin/faker/TestFakerQueries.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -850,4 +850,32 @@ AND rnd_uuid IN (UUID '1fc74d96-0216-449b-a145-455578a9eaa5', UUID '3ee49ede-002

assertUpdate("DROP TABLE faker.default.all_types_in");
}

@Test
void testRenameTable()
{
assertUpdate("CREATE TABLE faker.default.original_table (id INTEGER, name VARCHAR)");
assertUpdate("ALTER TABLE faker.default.original_table RENAME TO faker.default.renamed_table");
assertUpdate("DROP TABLE faker.default.renamed_table");
}

@Test
void testSetTableProperties()
{
try (TestTable table = new TestTable(getQueryRunner()::execute, "set_table_properties", "(id INTEGER, name VARCHAR)")) {
assertUpdate("ALTER TABLE %s SET PROPERTIES default_limit = 100".formatted(table.getName()));
assertQueryFails("ALTER TABLE %s SET PROPERTIES invalid_property = true".formatted(table.getName()), "(?s).*Catalog 'faker' table property 'invalid_property' does not exist");
assertThat((String) computeScalar("SHOW CREATE TABLE " + table.getName()))
.contains("default_limit = 100");
}
}

@Test
void testTableComment()
{
try (TestTable table = new TestTable(getQueryRunner()::execute, "table_comment", "(id INTEGER, name VARCHAR)")) {
assertUpdate("COMMENT ON TABLE %s IS 'this is a test table'".formatted(table.getName()));
assertThat(getTableComment(table.getName())).isEqualTo("this is a test table");
}
}
}
5 changes: 0 additions & 5 deletions ...rino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergRegisterTableProcedure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,11 +575,6 @@ private void dropTableFromMetastore(String tableName)
assertThat(metastore.getTable(getSession().getSchema().orElseThrow(), tableName)).as("Table in metastore should be dropped").isEmpty();
}

private String getTableComment(String tableName)
{
return (String) computeScalar("SELECT comment FROM system.metadata.table_comments WHERE catalog_name = 'iceberg' AND schema_name = '" + getSession().getSchema().orElseThrow() + "' AND table_name = '" + tableName + "'");
}

private String getColumnComment(String tableName, String columnName)
{
return (String) computeScalar("SELECT comment FROM information_schema.columns WHERE table_schema = '" + getSession().getSchema().orElseThrow() + "' AND table_name = '" + tableName + "' AND column_name = '" + columnName + "'");
Expand Down
6 changes: 0 additions & 6 deletions ...io/trino/plugin/iceberg/catalog/rest/TestIcebergPolarisCatalogCaseInsensitiveMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,12 +255,6 @@ void testCaseInsensitiveMatchingForView()
assertQueryFails("SELECT * FROM " + viewName2, ".*'iceberg.%s.%s' does not exist".formatted(LOWERCASE_SCHEMA, lowercaseViewName2));
}

private String getTableComment(String tableName)
{
return (String) computeScalar("SELECT comment FROM system.metadata.table_comments " +
"WHERE catalog_name = 'iceberg' AND schema_name = '" + LOWERCASE_SCHEMA + "' AND table_name = '" + tableName + "'");
}

private String getColumnComment(String tableName, String columnName)
{
return (String) computeScalar("SELECT comment FROM information_schema.columns " +
Expand Down
5 changes: 0 additions & 5 deletions ...-iceberg/src/test/java/io/trino/plugin/iceberg/procedure/TestIcebergMigrateProcedure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,11 +436,6 @@ public void testMigrateTablePreserveComments()
assertUpdate("DROP TABLE " + tableName);
}

private String getTableComment(String tableName)
{
return (String) computeScalar("SELECT comment FROM system.metadata.table_comments WHERE catalog_name = 'iceberg' AND schema_name = 'tpch' AND table_name = '" + tableName + "'");
}

private String getColumnComment(String tableName, String columnName)
{
return (String) computeScalar("SELECT comment FROM information_schema.columns WHERE table_catalog = 'iceberg' AND table_schema = 'tpch' AND table_name = '" + tableName + "' AND column_name = '" + columnName + "'");
Expand Down
16 changes: 16 additions & 0 deletions testing/trino-testing/src/main/java/io/trino/testing/AbstractTestQueryFramework.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,22 @@ private CatalogSchemaTableName getTableName(TableHandle tableHandle)
});
}

protected String getTableComment(String tableName)
{
return getTableComment(getSession(), tableName);
}

protected String getTableComment(Session session, String tableName)
{
return getTableComment(session.getCatalog().orElseThrow(), session.getSchema().orElseThrow(), tableName);
}

protected String getTableComment(String catalogName, String schemaName, String tableName)
{
String sql = format("SELECT comment FROM system.metadata.table_comments WHERE catalog_name = '%s' AND schema_name = '%s' AND table_name = '%s'", catalogName, schemaName, tableName);
return (String) computeScalar(sql);
}

private <T> T inTransaction(Session session, Function<Session, T> transactionSessionConsumer)
{
return transaction(getQueryRunner().getTransactionManager(), getQueryRunner().getPlannerContext().getMetadata(), getQueryRunner().getAccessControl())
Expand Down
5 changes: 0 additions & 5 deletions testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,11 +742,6 @@ public void testCommentMaterializedViewColumn()
}
}

protected String getTableComment(String tableName)
{
return (String) computeScalar(format("SELECT comment FROM system.metadata.table_comments WHERE catalog_name = '%s' AND schema_name = '%s' AND table_name = '%s'", getSession().getCatalog().orElseThrow(), getSession().getSchema().orElseThrow(), tableName));
}

protected String getColumnComment(String tableName, String columnName)
{
return (String) computeScalar(format(
Expand Down
6 changes: 0 additions & 6 deletions testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4252,12 +4252,6 @@ public void testCommentTable()
}
}

protected String getTableComment(String catalogName, String schemaName, String tableName)
{
String sql = format("SELECT comment FROM system.metadata.table_comments WHERE catalog_name = '%s' AND schema_name = '%s' AND table_name = '%s'", catalogName, schemaName, tableName);
return (String) computeScalar(sql);
}

@Test
public void testCommentView()
{
Expand Down
Loading