Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
13 changes: 13 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2420,6 +2420,19 @@
<version>2.1.0-3</version>
</dependency>

<dependency>
<groupId>com.facebook.presto</groupId>
<artifactId>presto-clp</artifactId>
<version>${project.version}</version>
</dependency>

<dependency>
<groupId>com.facebook.presto</groupId>
<artifactId>presto-clp</artifactId>
<version>${project.version}</version>
<type>test-jar</type>
</dependency>

<!-- force newer version to be used for dependencies -->
<dependency>
<groupId>org.javassist</groupId>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,278 @@
/*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.facebook.presto.plugin.clp.mockdb;

import com.facebook.presto.plugin.clp.mockdb.table.ArchivesTableRows;
import com.facebook.presto.plugin.clp.mockdb.table.ColumnMetadataTableRows;
import com.facebook.presto.plugin.clp.mockdb.table.DatasetsTableRows;
import com.google.common.collect.ImmutableList;

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.List;
import java.util.Map;
import java.util.UUID;

import static java.lang.String.format;
import static java.util.Objects.requireNonNull;
import static org.testng.Assert.assertTrue;
import static org.testng.Assert.fail;

/**
* File-backed H2 mock metadata database for CLP tests. Uses the same schema as the CLP package.
* Provides a builder-driven setup and a single-call teardown that drops all objects and deletes
* files.
*/
public class ClpMockMetadataDatabase
Comment thread
coderabbitai[bot] marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There seems to be a lot of duplication with ClpMetadataDbSetUp?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It is a refactored ClpMetadataDbSetup to provide interface which can be used for E2E testing. I iniitally directly modified ClpMetadataDbSetUp but gave up because it needed to modify the unit tests using it as well. To make this PR clean and don't touch existing unit tests, I rewrote a new one. In the next PR we can use this class to refactor the exsiting unit tests and finally get rid of ClpMetadataDbSetup.

{
private static final String MOCK_METADATA_DB_DEFAULT_USERNAME = "sa";
private static final String MOCK_METADATA_DB_DEFAULT_PASSWORD = "";

private static final String MOCK_METADATA_DB_DEFAULT_TABLE_PREFIX = "clp_";

private static final String MOCK_METADATA_DB_URL_TEMPLATE = "jdbc:h2:file:%s;MODE=MySQL;DATABASE_TO_UPPER=FALSE";

private String url;
private String archiveStorageDirectory;
private String username;
private String password;
private String tablePrefix;

/**
* Creates a new builder instance for constructing {@link ClpMockMetadataDatabase}.
*
* @return a new {@link Builder}
*/
public static Builder builder()
{
return new Builder();
}

/**
* Tears down the mock database by dropping all objects (tables, views, etc.) and deleting the
* backing database file. Any exceptions during cleanup will cause the test to fail.
*/
public void teardown()
{
try (Connection connection = DriverManager.getConnection(url, username, password); Statement stmt = connection.createStatement()) {
stmt.execute("DROP ALL OBJECTS DELETE FILES");
}
catch (SQLException e) {
fail(e.getMessage());
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

public String getUrl()
{
return url;
}

public String getUsername()
{
return username;
}

public String getPassword()
{
return password;
}

public String getTablePrefix()
{
return tablePrefix;
}

private void addTableToDatasetsTableIfNotExist(List<String> tableNames)
{
try (Connection connection = DriverManager.getConnection(url, username, password)) {
ImmutableList.Builder<String> repeatedArchiveStorageDirectory = ImmutableList.builder();
for (String tableName : tableNames) {
ArchivesTableRows.createTableIfNotExist(connection, tablePrefix, tableName);
ColumnMetadataTableRows.createTableIfNotExist(connection, tablePrefix, tableName);
repeatedArchiveStorageDirectory.add(archiveStorageDirectory);
}
DatasetsTableRows datasetsTableRows = new DatasetsTableRows(tableNames, repeatedArchiveStorageDirectory.build());
datasetsTableRows.insertToTable(connection, tablePrefix);
}
Comment thread
anlowee marked this conversation as resolved.
Outdated
catch (SQLException e) {
fail(e.getMessage());
}
Comment thread
anlowee marked this conversation as resolved.
}
Comment on lines +108 to +123

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Normalise archiveStorageDirectory and validate input; improve diagnostics.

Missing trailing “/” yields bad tablePath; add null-check and normalise once.

-    public void addTableToDatasetsTableIfNotExist(List<String> tableNames)
+    public void addTableToDatasetsTableIfNotExist(List<String> tableNames)
     {
         try (Connection connection = DriverManager.getConnection(url, username, password)) {
             ImmutableList.Builder<String> repeatedArchiveStorageDirectory = ImmutableList.builder();
-            for (String tableName : tableNames) {
+            requireNonNull(tableNames, "tableNames is null");
+            final String normalizedArchiveDir = archiveStorageDirectory.endsWith("/") ? archiveStorageDirectory : archiveStorageDirectory + "/";
+            for (String tableName : tableNames) {
                 createArchivesTableIfNotExist(connection, tableName);
                 createColumnMetadataTableIfNotExist(connection, tableName);
-                repeatedArchiveStorageDirectory.add(archiveStorageDirectory);
+                repeatedArchiveStorageDirectory.add(normalizedArchiveDir);
             }
             DatasetsTableRows datasetsTableRows = new DatasetsTableRows(tableNames, repeatedArchiveStorageDirectory.build());
             datasetsTableRows.insertToTable(connection, tablePrefix);
         }
-        catch (SQLException e) {
-            fail(e.getMessage());
-        }
+        catch (SQLException e) {
+            fail(format("Failed to upsert datasets or create per-table structures for %s: %s [SQLState=%s, ErrorCode=%s]",
+                    tableNames, e.getMessage(), e.getSQLState(), e.getErrorCode()));
+        }
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public void addTableToDatasetsTableIfNotExist(List<String> tableNames)
{
try (Connection connection = DriverManager.getConnection(url, username, password)) {
ImmutableList.Builder<String> repeatedArchiveStorageDirectory = ImmutableList.builder();
for (String tableName : tableNames) {
createArchivesTableIfNotExist(connection, tableName);
createColumnMetadataTableIfNotExist(connection, tableName);
repeatedArchiveStorageDirectory.add(archiveStorageDirectory);
}
DatasetsTableRows datasetsTableRows = new DatasetsTableRows(tableNames, repeatedArchiveStorageDirectory.build());
datasetsTableRows.insertToTable(connection, tablePrefix);
}
catch (SQLException e) {
fail(e.getMessage());
}
}
public void addTableToDatasetsTableIfNotExist(List<String> tableNames)
{
try (Connection connection = DriverManager.getConnection(url, username, password)) {
ImmutableList.Builder<String> repeatedArchiveStorageDirectory = ImmutableList.builder();
requireNonNull(tableNames, "tableNames is null");
final String normalizedArchiveDir = archiveStorageDirectory.endsWith("/")
? archiveStorageDirectory
: archiveStorageDirectory + "/";
for (String tableName : tableNames) {
createArchivesTableIfNotExist(connection, tableName);
createColumnMetadataTableIfNotExist(connection, tableName);
repeatedArchiveStorageDirectory.add(normalizedArchiveDir);
}
DatasetsTableRows datasetsTableRows = new DatasetsTableRows(
tableNames,
repeatedArchiveStorageDirectory.build());
datasetsTableRows.insertToTable(connection, tablePrefix);
}
catch (SQLException e) {
fail(format(
"Failed to upsert datasets or create per-table structures for %s: %s [SQLState=%s, ErrorCode=%s]",
tableNames,
e.getMessage(),
e.getSQLState(),
e.getErrorCode()));
}
}
🤖 Prompt for AI Agents
In
presto-clp/src/test/java/com/facebook/presto/plugin/clp/mockdb/ClpMockMetadataDatabase.java
around lines 108 to 123, normalize and validate archiveStorageDirectory before
use: add a null-check (throw or fail early with a diagnostic including the
provided value), trim it once and ensure it ends with a single "/" (append if
missing) prior to the loop so tablePath is built correctly, and improve error
diagnostics by including the archiveStorageDirectory value and full exception
details in the catch/fail call.


private void addColumnMetadata(Map<String, ColumnMetadataTableRows> clpFields)
{
try (Connection connection = DriverManager.getConnection(url, username, password)) {
for (Map.Entry<String, ColumnMetadataTableRows> entry : clpFields.entrySet()) {
entry.getValue().insertToTable(connection, tablePrefix, entry.getKey());
}
}
catch (SQLException e) {
fail(e.getMessage());
}
Comment thread
anlowee marked this conversation as resolved.
}

private void addSplits(Map<String, ArchivesTableRows> splits)
{
try (Connection connection = DriverManager.getConnection(url, username, password)) {
for (Map.Entry<String, ArchivesTableRows> entry : splits.entrySet()) {
entry.getValue().insertToTable(connection, tablePrefix, entry.getKey());
}
}
catch (SQLException e) {
fail(e.getMessage());
}
Comment thread
anlowee marked this conversation as resolved.
}

private ClpMockMetadataDatabase() {}

public static final class Builder
{
private final ClpMockMetadataDatabase mockMetadataDatabase;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think the builder itself shouldn't hold this instance

private boolean isDatasetsTableCreated;

private Builder()
{
mockMetadataDatabase = new ClpMockMetadataDatabase();
setDatabaseUrl(format("/tmp/%s", UUID.randomUUID()));
setUsername(MOCK_METADATA_DB_DEFAULT_USERNAME);
setPassword(MOCK_METADATA_DB_DEFAULT_PASSWORD);
setTablePrefix(MOCK_METADATA_DB_DEFAULT_TABLE_PREFIX);
}
Comment on lines +219 to +223

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we can have use default values for this variables?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

These are already the default;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I mean in the class variable declaration part.


public Builder setDatabaseUrl(String databaseFilePath)
{
mockMetadataDatabase.url = format(MOCK_METADATA_DB_URL_TEMPLATE, databaseFilePath);
return this;
}
Comment thread
anlowee marked this conversation as resolved.
Comment thread
anlowee marked this conversation as resolved.

public Builder setArchiveStorageDirectory(String archiveStorageDirectory)
{
mockMetadataDatabase.archiveStorageDirectory = archiveStorageDirectory;
return this;
}

public Builder setUsername(String username)
{
mockMetadataDatabase.username = username;
return this;
}

public Builder setPassword(String password)
{
mockMetadataDatabase.password = password;
return this;
}

public Builder setTablePrefix(String tablePrefix)
{
mockMetadataDatabase.tablePrefix = tablePrefix;
return this;
}

/**
* Creates the datasets table if it does not already exist. Must be called before invoking
* {@code addTables}, {@code addColumnMetadata}, or {@code addSplits}. Assumes
* {@code setDatabaseUrl}, {@code setUsername}, {@code setPassword}, and
* {@code setTablePrefix} have been set (defaults provided).
*
* @return this builder
*/
public Builder createDatasetsTableIfNotExist()
{
validateDatasetsTableCreationRequirements();
if (!isDatasetsTableCreated) {
DatasetsTableRows.createTableIfNotExist(
mockMetadataDatabase.url,
mockMetadataDatabase.username,
mockMetadataDatabase.password,
mockMetadataDatabase.tablePrefix);
isDatasetsTableCreated = true;
}
return this;
}

/**
* Ensures all table names have been registered in the datasets table and for each table name
* an archives table and a column metadata table have been created.
*
* @param tableNames list of table names to add
* @return this builder
*/
public Builder addTables(List<String> tableNames)
{
validateMockMetadataDatabase();
mockMetadataDatabase.addTableToDatasetsTableIfNotExist(tableNames);
return this;
}
Comment thread
anlowee marked this conversation as resolved.
Outdated

/**
* Inserts column metadata for the given fields into the column metadata table corresponding to
* the given table name.
*
* @param clpFields mapping of table name to {@link ColumnMetadataTableRows}
* @return this builder
*/
public Builder addColumnMetadata(Map<String, ColumnMetadataTableRows> clpFields)
{
validateMockMetadataDatabase();
mockMetadataDatabase.addColumnMetadata(clpFields);
return this;
}

/**
* Inserts split metadata into the archives table corresponding to the given table name.
*
* @param splits mapping of table name to {@link ArchivesTableRows}
* @return this builder
*/
public Builder addSplits(Map<String, ArchivesTableRows> splits)
{
validateMockMetadataDatabase();
mockMetadataDatabase.addSplits(splits);
return this;
}

/**
* Builds and returns the configured {@link ClpMockMetadataDatabase} instance.
*
* @return the constructed {@link ClpMockMetadataDatabase}
*/
public ClpMockMetadataDatabase build()
{
validateMockMetadataDatabase();
return mockMetadataDatabase;
}

/**
* Validates that all required parameters have been set and the datasets table has been
* created.
*/
private void validateMockMetadataDatabase()
{
assertTrue(isDatasetsTableCreated, "createDatasetsTableIfNotExist might not be called");
requireNonNull(mockMetadataDatabase.archiveStorageDirectory, "archiveStorageDirectory is null");
validateDatasetsTableCreationRequirements();
}

private void validateDatasetsTableCreationRequirements()
{
requireNonNull(mockMetadataDatabase.url, "url is null");
requireNonNull(mockMetadataDatabase.username, "username is null");
requireNonNull(mockMetadataDatabase.password, "password is null");
requireNonNull(mockMetadataDatabase.tablePrefix, "tablePrefix is null");
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.facebook.presto.plugin.clp.mockdb.table;

import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.List;

import static com.facebook.presto.plugin.clp.split.ClpMySqlSplitProvider.ARCHIVES_TABLE_SUFFIX;
import static java.lang.String.format;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.fail;

public class ArchivesTableRows

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

also this class

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Same reason as above.

{
private static final String COLUMN_PAGINATION_ID = "pagination_id";
private static final String COLUMN_ID = "id";
private static final String COLUMN_BEGIN_TIMESTAMP = "begin_timestamp";
private static final String COLUMN_END_TIMESTAMP = "end_timestamp";

private final List<String> ids;
private final List<Long> beginTimestamps;
private final List<Long> endTimestamps;
private final int numberOfRows;

public static void createTableIfNotExist(Connection connection, String tablePrefix, String tableName)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

From my understanding, this should be implemented as a method in the place where it’s being called, and the same applies to the other two classes.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Another way is to have an ArchivesTable class and an internal class called Rows

{
final String createTableSql = format(
"CREATE TABLE IF NOT EXISTS `%s` (" +
"`%s` BIGINT AUTO_INCREMENT PRIMARY KEY, " +
"`%s` VARCHAR(64) NOT NULL, " +
"`%s` BIGINT, " +
"`%s` BIGINT)",
format("%s%s%s", tablePrefix, tableName, ARCHIVES_TABLE_SUFFIX),
COLUMN_PAGINATION_ID,
COLUMN_ID,
COLUMN_BEGIN_TIMESTAMP,
COLUMN_END_TIMESTAMP);
try (Statement stmt = connection.createStatement()) {
stmt.execute(createTableSql);
}
catch (SQLException e) {
fail(e.getMessage());
}
Comment thread
anlowee marked this conversation as resolved.
Outdated
}

public void insertToTable(Connection connection, String tablePrefix, String tableName)
{
final String insertSql = format(
"INSERT INTO `%s` (`%s`, `%s`, `%s`) VALUES (?, ?, ?)",
format("%s%s%s", tablePrefix, tableName, ARCHIVES_TABLE_SUFFIX),
COLUMN_ID,
COLUMN_BEGIN_TIMESTAMP,
COLUMN_END_TIMESTAMP);
try (PreparedStatement pstmt = connection.prepareStatement(insertSql)) {
for (int i = 0; i < numberOfRows; ++i) {
pstmt.setString(1, ids.get(i));
pstmt.setLong(2, beginTimestamps.get(i));
pstmt.setLong(3, endTimestamps.get(i));
pstmt.addBatch();
}
pstmt.executeBatch();
}
Comment thread
anlowee marked this conversation as resolved.
catch (SQLException e) {
fail(e.getMessage());
}
}

public ArchivesTableRows(
List<String> ids,
List<Long> beginTimestamps,
List<Long> endTimestamps)
{
assertEquals(ids.size(), beginTimestamps.size());
assertEquals(beginTimestamps.size(), endTimestamps.size());
this.ids = ids;
this.beginTimestamps = beginTimestamps;
this.endTimestamps = endTimestamps;
this.numberOfRows = ids.size();
}
Comment thread
anlowee marked this conversation as resolved.
}
Loading
Loading