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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+59 −50
Open

Add missing unit tests on Faker queries #24540

Show file tree
Hide file tree
Changes from 1 commit
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
31 changes: 19 additions & 12 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
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.

import com.google.errorprone.annotations.concurrent.GuardedBy;
import io.airlift.slice.Slice;
import io.trino.spi.TrinoException;
Expand Down Expand Up @@ -59,12 +60,12 @@
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;
import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static io.trino.plugin.faker.TableInfo.DEFAULT_LIMIT_PROPERTY;
import static io.trino.plugin.faker.TableInfo.NULL_PROBABILITY_PROPERTY;
import static io.trino.spi.StandardErrorCode.INVALID_COLUMN_PROPERTY;
import static io.trino.spi.StandardErrorCode.INVALID_COLUMN_REFERENCE;
import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED;
Expand Down Expand Up @@ -146,7 +147,7 @@ public synchronized ConnectorTableHandle getTableHandle(ConnectorSession session
return null;
}
long schemaLimit = (long) schema.properties().getOrDefault(SchemaInfo.DEFAULT_LIMIT_PROPERTY, defaultLimit);
long tableLimit = (long) tables.get(tableName).properties().getOrDefault(TableInfo.DEFAULT_LIMIT_PROPERTY, schemaLimit);
long tableLimit = (long) tables.get(tableName).properties().getOrDefault(DEFAULT_LIMIT_PROPERTY, schemaLimit);
sthandassery marked this conversation as resolved.
Show resolved Hide resolved
return new FakerTableHandle(tableName, TupleDomain.all(), tableLimit);
}

Expand Down Expand Up @@ -223,8 +224,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.

}

@Override
Expand All @@ -234,13 +236,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(NULL_PROBABILITY_PROPERTY)) {
double nullProbability = (double) properties.get(NULL_PROBABILITY_PROPERTY)
.orElseThrow(() -> new IllegalArgumentException("The null_probability property cannot be empty"));
updatedProperties.put(NULL_PROBABILITY_PROPERTY, nullProbability);
}
if (properties.containsKey(DEFAULT_LIMIT_PROPERTY)) {
long defaultLimit = (long) properties.get(DEFAULT_LIMIT_PROPERTY)
.orElseThrow(() -> new IllegalArgumentException("The default_limit property cannot be empty"));
updatedProperties.put(DEFAULT_LIMIT_PROPERTY, defaultLimit);
}
tables.put(tableName, oldInfo.withProperties(updatedProperties.buildOrThrow()));
}

@Override
Expand Down Expand Up @@ -295,7 +302,7 @@ public synchronized FakerOutputTableHandle beginCreateTable(ConnectorSession ses
checkTableNotExists(tableMetadata.getTable());

double schemaNullProbability = (double) schema.properties().getOrDefault(SchemaInfo.NULL_PROBABILITY_PROPERTY, nullProbability);
double tableNullProbability = (double) tableMetadata.getProperties().getOrDefault(TableInfo.NULL_PROBABILITY_PROPERTY, schemaNullProbability);
double tableNullProbability = (double) tableMetadata.getProperties().getOrDefault(NULL_PROBABILITY_PROPERTY, schemaNullProbability);

ImmutableList.Builder<ColumnInfo> columns = ImmutableList.builder();
int columnId = 0;
Expand Down
39 changes: 39 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,43 @@ AND rnd_uuid IN (UUID '1fc74d96-0216-449b-a145-455578a9eaa5', UUID '3ee49ede-002

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

@Test
void testRenameTable(){
sthandassery marked this conversation as resolved.
Show resolved Hide resolved
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");
sthandassery marked this conversation as resolved.
Show resolved Hide resolved
}


@Test
void testSetTableProperties(){
try (TestTable table = new TestTable(getQueryRunner()::execute, "table", "(id INTEGER, name VARCHAR)")) {
sthandassery marked this conversation as resolved.
Show resolved Hide resolved
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(computeActual("SHOW CREATE TABLE %s".formatted(table.getName())).getOnlyValue())
sthandassery marked this conversation as resolved.
Show resolved Hide resolved
.isEqualTo("""
CREATE TABLE faker.default.%s (
id integer,
name varchar
)
WITH (
default_limit = 100
)""".formatted(table.getName()));
}
}

@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(computeScalar("SHOW CREATE TABLE %s".formatted(table.getName()))).isEqualTo("""
sthandassery marked this conversation as resolved.
Show resolved Hide resolved
CREATE TABLE faker.default.%s (
id integer,
name varchar
)
COMMENT 'this is a test table'""".formatted(table.getName()));
}
}
}
Loading