Skip to content

Commit 12f2515

Browse files
wraymowraymo
committed
apply review comment suggestions
1 parent a3b7cb7 commit 12f2515

File tree

12 files changed

+73
-53
lines changed

12 files changed

+73
-53
lines changed

‎presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpConfig.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ public class ClpConfig
2323
public static final Pattern SAFE_SQL_TABLE_NAME_PATTERN = Pattern.compile("^[a-zA-Z0-9_]+$");
2424

2525
private boolean polymorphicTypeEnabled = true;
26+
2627
private MetadataProviderType metadataProviderType = MetadataProviderType.MYSQL;
2728
private String metadataDbUrl;
2829
private String metadataDbName;
@@ -31,6 +32,7 @@ public class ClpConfig
3132
private String metadataTablePrefix;
3233
private long metadataRefreshInterval = 60;
3334
private long metadataExpireInterval = 600;
35+
3436
private SplitProviderType splitProviderType = SplitProviderType.MYSQL;
3537

3638
public boolean isPolymorphicTypeEnabled()

‎presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpConnectorFactory.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,11 +52,11 @@ public Connector create(String catalogName, Map<String, String> config, Connecto
5252
requireNonNull(config, "config is null");
5353
try {
5454
Bootstrap app = new Bootstrap(new JsonModule(), new ClpModule(), binder -> {
55-
binder.bind(TypeManager.class).toInstance(context.getTypeManager());
56-
binder.bind(NodeManager.class).toInstance(context.getNodeManager());
5755
binder.bind(FunctionMetadataManager.class).toInstance(context.getFunctionMetadataManager());
58-
binder.bind(StandardFunctionResolution.class).toInstance(context.getStandardFunctionResolution());
56+
binder.bind(NodeManager.class).toInstance(context.getNodeManager());
5957
binder.bind(RowExpressionService.class).toInstance(context.getRowExpressionService());
58+
binder.bind(StandardFunctionResolution.class).toInstance(context.getStandardFunctionResolution());
59+
binder.bind(TypeManager.class).toInstance(context.getTypeManager());
6060
});
6161

6262
Injector injector = app.doNotInitializeLogging().setRequiredConfigurationProperties(config).initialize();

‎presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpMetadata.java‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@
4040
import java.util.Set;
4141
import java.util.function.Function;
4242

43+
import static com.google.common.collect.ImmutableList.toImmutableList;
44+
import static com.google.common.collect.ImmutableMap.toImmutableMap;
4345
import static java.util.Objects.requireNonNull;
4446
import static java.util.concurrent.TimeUnit.SECONDS;
4547

@@ -92,7 +94,7 @@ public List<SchemaTableName> listTables(ConnectorSession session, Optional<Strin
9294

9395
return listTables(schemaNameValue).stream()
9496
.map(tableHandle -> new SchemaTableName(schemaNameValue, tableHandle.getSchemaTableName().getTableName()))
95-
.collect(ImmutableList.toImmutableList());
97+
.collect(toImmutableList());
9698
}
9799

98100
@Override
@@ -134,7 +136,7 @@ public ConnectorTableMetadata getTableMetadata(ConnectorSession session, Connect
134136
SchemaTableName schemaTableName = clpTableHandle.getSchemaTableName();
135137
List<ColumnMetadata> columns = listColumns(schemaTableName).stream()
136138
.map(ClpColumnHandle::getColumnMetadata)
137-
.collect(ImmutableList.toImmutableList());
139+
.collect(toImmutableList());
138140

139141
return new ConnectorTableMetadata(schemaTableName, columns);
140142
}
@@ -163,7 +165,7 @@ public Map<SchemaTableName, List<ColumnMetadata>> listTableColumns(ConnectorSess
163165
}
164166

165167
return schemaTableNames.stream()
166-
.collect(ImmutableMap.toImmutableMap(
168+
.collect(toImmutableMap(
167169
Function.identity(),
168170
tableName -> getTableMetadata(session, getTableHandle(session, tableName)).getColumns()));
169171
}
@@ -172,10 +174,7 @@ public Map<SchemaTableName, List<ColumnMetadata>> listTableColumns(ConnectorSess
172174
public Map<String, ColumnHandle> getColumnHandles(ConnectorSession session, ConnectorTableHandle tableHandle)
173175
{
174176
ClpTableHandle clpTableHandle = (ClpTableHandle) tableHandle;
175-
return listColumns(clpTableHandle.getSchemaTableName()).stream()
176-
.collect(ImmutableMap.toImmutableMap(
177-
ClpColumnHandle::getColumnName,
178-
column -> column));
177+
return listColumns(clpTableHandle.getSchemaTableName()).stream().collect(toImmutableMap(ClpColumnHandle::getColumnName, column -> column));
179178
}
180179

181180
@Override
@@ -195,13 +194,13 @@ private List<ClpTableHandle> loadTableHandles(String schemaName)
195194
return clpMetadataProvider.listTableHandles(schemaName);
196195
}
197196

198-
private List<ClpTableHandle> listTables(String schemaName)
197+
private List<ClpColumnHandle> listColumns(SchemaTableName schemaTableName)
199198
{
200-
return tableHandleCache.getUnchecked(schemaName);
199+
return columnHandleCache.getUnchecked(schemaTableName);
201200
}
202201

203-
private List<ClpColumnHandle> listColumns(SchemaTableName schemaTableName)
202+
private List<ClpTableHandle> listTables(String schemaName)
204203
{
205-
return columnHandleCache.getUnchecked(schemaTableName);
204+
return tableHandleCache.getUnchecked(schemaName);
206205
}
207206
}

‎presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpModule.java‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,10 @@
2323
import com.google.inject.Scopes;
2424

2525
import static com.facebook.airlift.configuration.ConfigBinder.configBinder;
26+
import static com.facebook.presto.plugin.clp.ClpConfig.MetadataProviderType;
27+
import static com.facebook.presto.plugin.clp.ClpConfig.SplitProviderType;
28+
import static com.facebook.presto.plugin.clp.ClpErrorCode.CLP_UNSUPPORTED_METADATA_SOURCE;
29+
import static com.facebook.presto.plugin.clp.ClpErrorCode.CLP_UNSUPPORTED_SPLIT_SOURCE;
2630

2731
public class ClpModule
2832
extends AbstractConfigurationAwareModule
@@ -37,18 +41,18 @@ protected void setup(Binder binder)
3741
configBinder(binder).bindConfig(ClpConfig.class);
3842

3943
ClpConfig config = buildConfigObject(ClpConfig.class);
40-
if (config.getMetadataProviderType() == ClpConfig.MetadataProviderType.MYSQL) {
44+
if (config.getMetadataProviderType() == MetadataProviderType.MYSQL) {
4145
binder.bind(ClpMetadataProvider.class).to(ClpMySqlMetadataProvider.class).in(Scopes.SINGLETON);
4246
}
4347
else {
44-
throw new PrestoException(ClpErrorCode.CLP_UNSUPPORTED_METADATA_SOURCE, "Unsupported metadata provider type: " + config.getMetadataProviderType());
48+
throw new PrestoException(CLP_UNSUPPORTED_METADATA_SOURCE, "Unsupported metadata provider type: " + config.getMetadataProviderType());
4549
}
4650

47-
if (config.getSplitProviderType() == ClpConfig.SplitProviderType.MYSQL) {
51+
if (config.getSplitProviderType() == SplitProviderType.MYSQL) {
4852
binder.bind(ClpSplitProvider.class).to(ClpMySqlSplitProvider.class).in(Scopes.SINGLETON);
4953
}
5054
else {
51-
throw new PrestoException(ClpErrorCode.CLP_UNSUPPORTED_SPLIT_SOURCE, "Unsupported split provider type: " + config.getSplitProviderType());
55+
throw new PrestoException(CLP_UNSUPPORTED_SPLIT_SOURCE, "Unsupported split provider type: " + config.getSplitProviderType());
5256
}
5357
}
5458
}

‎presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpSplit.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,10 @@
2020
import com.fasterxml.jackson.annotation.JsonCreator;
2121
import com.fasterxml.jackson.annotation.JsonProperty;
2222
import com.google.common.collect.ImmutableList;
23+
import com.google.common.collect.ImmutableMap;
2324

2425
import java.util.List;
26+
import java.util.Map;
2527

2628
import static com.facebook.presto.spi.schedule.NodeSelectionStrategy.NO_PREFERENCE;
2729
import static java.util.Objects.requireNonNull;
@@ -56,8 +58,8 @@ public List<HostAddress> getPreferredNodes(NodeProvider nodeProvider)
5658
}
5759

5860
@Override
59-
public Object getInfo()
61+
public Map<String, String> getInfo()
6062
{
61-
return this;
63+
return ImmutableMap.of("path", path);
6264
}
6365
}

‎presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMySqlMetadataProvider.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import com.facebook.presto.plugin.clp.ClpConfig;
1919
import com.facebook.presto.plugin.clp.ClpTableHandle;
2020
import com.facebook.presto.spi.SchemaTableName;
21+
import com.google.common.collect.ImmutableList;
2122

2223
import javax.inject.Inject;
2324

@@ -27,7 +28,6 @@
2728
import java.sql.ResultSet;
2829
import java.sql.SQLException;
2930
import java.sql.Statement;
30-
import java.util.ArrayList;
3131
import java.util.List;
3232

3333
import static java.lang.String.format;
@@ -95,7 +95,7 @@ public List<ClpColumnHandle> listColumnHandles(SchemaTableName schemaTableName)
9595
@Override
9696
public List<ClpTableHandle> listTableHandles(String schemaName)
9797
{
98-
List<ClpTableHandle> tableHandles = new ArrayList<>();
98+
ImmutableList.Builder<ClpTableHandle> tableHandles = new ImmutableList.Builder<>();
9999
String query = format(SQL_SELECT_DATASETS_TEMPLATE, config.getMetadataTablePrefix());
100100
try (Connection connection = getConnection();
101101
Statement statement = connection.createStatement();
@@ -119,7 +119,7 @@ public List<ClpTableHandle> listTableHandles(String schemaName)
119119
catch (SQLException e) {
120120
log.warn("Failed to load table names: %s", e);
121121
}
122-
return tableHandles;
122+
return tableHandles.build();
123123
}
124124

125125
private Connection getConnection()

‎presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpSchemaTree.java‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,8 @@
1717
import com.facebook.presto.common.type.RowType;
1818
import com.facebook.presto.common.type.Type;
1919
import com.facebook.presto.plugin.clp.ClpColumnHandle;
20-
import com.facebook.presto.plugin.clp.ClpErrorCode;
2120
import com.facebook.presto.spi.PrestoException;
21+
import com.google.common.collect.ImmutableList;
2222

2323
import java.util.ArrayList;
2424
import java.util.Collections;
@@ -33,6 +33,7 @@
3333
import static com.facebook.presto.common.type.BooleanType.BOOLEAN;
3434
import static com.facebook.presto.common.type.DoubleType.DOUBLE;
3535
import static com.facebook.presto.common.type.VarcharType.VARCHAR;
36+
import static com.facebook.presto.plugin.clp.ClpErrorCode.CLP_UNSUPPORTED_TYPE;
3637

3738
/**
3839
* A representation of CLP's schema tree built by turning hierarchical column names (e.g., a.b.c)
@@ -94,7 +95,7 @@ public void addColumn(String fullName, byte type)
9495
*/
9596
public List<ClpColumnHandle> collectColumnHandles()
9697
{
97-
List<ClpColumnHandle> columns = new ArrayList<>();
98+
ImmutableList.Builder<ClpColumnHandle> columns = new ImmutableList.Builder<>();
9899
for (Map.Entry<String, ClpNode> entry : root.children.entrySet()) {
99100
String name = entry.getKey();
100101
ClpNode child = entry.getValue();
@@ -106,7 +107,7 @@ public List<ClpColumnHandle> collectColumnHandles()
106107
columns.add(new ClpColumnHandle(name, child.originalName, rowType, true));
107108
}
108109
}
109-
return columns;
110+
return columns.build();
110111
}
111112

112113
private Type mapColumnType(byte type)
@@ -126,7 +127,7 @@ private Type mapColumnType(byte type)
126127
case Boolean:
127128
return BOOLEAN;
128129
default:
129-
throw new PrestoException(ClpErrorCode.CLP_UNSUPPORTED_TYPE, "Unsupported type: " + type);
130+
throw new PrestoException(CLP_UNSUPPORTED_TYPE, "Unsupported type: " + type);
130131
}
131132
}
132133

‎presto-clp/src/main/java/com/facebook/presto/plugin/clp/split/ClpMySqlSplitProvider.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import com.facebook.presto.plugin.clp.ClpSplit;
1919
import com.facebook.presto.plugin.clp.ClpTableHandle;
2020
import com.facebook.presto.plugin.clp.ClpTableLayoutHandle;
21+
import com.google.common.collect.ImmutableList;
2122

2223
import javax.inject.Inject;
2324

@@ -26,7 +27,6 @@
2627
import java.sql.PreparedStatement;
2728
import java.sql.ResultSet;
2829
import java.sql.SQLException;
29-
import java.util.ArrayList;
3030
import java.util.List;
3131

3232
import static java.lang.String.format;
@@ -63,7 +63,7 @@ public ClpMySqlSplitProvider(ClpConfig config)
6363
@Override
6464
public List<ClpSplit> listSplits(ClpTableLayoutHandle clpTableLayoutHandle)
6565
{
66-
List<ClpSplit> splits = new ArrayList<>();
66+
ImmutableList.Builder<ClpSplit> splits = new ImmutableList.Builder<>();
6767
ClpTableHandle clpTableHandle = clpTableLayoutHandle.getTable();
6868
String tablePath = clpTableHandle.getTablePath();
6969
String tableName = clpTableHandle.getSchemaTableName().getTableName();
@@ -83,7 +83,7 @@ public List<ClpSplit> listSplits(ClpTableLayoutHandle clpTableLayoutHandle)
8383
log.warn("Database error while processing splits for %s: %s", tableName, e);
8484
}
8585

86-
return splits;
86+
return splits.build();
8787
}
8888

8989
private Connection getConnection()

‎presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpMetadataDbSetUp.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@
3030
import java.sql.Statement;
3131
import java.util.List;
3232
import java.util.Map;
33-
import java.util.UUID;
3433

3534
import static com.facebook.presto.plugin.clp.metadata.ClpMySqlMetadataProvider.COLUMN_METADATA_TABLE_COLUMN_NAME;
3635
import static com.facebook.presto.plugin.clp.metadata.ClpMySqlMetadataProvider.COLUMN_METADATA_TABLE_COLUMN_TYPE;
@@ -41,6 +40,7 @@
4140
import static com.facebook.presto.plugin.clp.split.ClpMySqlSplitProvider.ARCHIVES_TABLE_COLUMN_ID;
4241
import static com.facebook.presto.plugin.clp.split.ClpMySqlSplitProvider.ARCHIVE_TABLE_SUFFIX;
4342
import static java.lang.String.format;
43+
import static java.util.UUID.randomUUID;
4444
import static org.testng.Assert.fail;
4545

4646
public final class ClpMetadataDbSetUp
@@ -62,7 +62,7 @@ private ClpMetadataDbSetUp()
6262

6363
public static DbHandle getDbHandle(String dbName)
6464
{
65-
return new DbHandle(format("/tmp/presto-clp-test-%s/%s", UUID.randomUUID(), dbName));
65+
return new DbHandle(format("/tmp/presto-clp-test-%s/%s", randomUUID(), dbName));
6666
}
6767

6868
public static ClpMetadata setupMetadata(DbHandle dbHandle, Map<String, List<Pair<String, ClpSchemaTreeNodeType>>> clpFields)
@@ -216,7 +216,7 @@ public static final class DbHandle
216216
{
217217
public String dbPath;
218218

219-
DbHandle(String dbPath)
219+
private DbHandle(String dbPath)
220220
{
221221
this.dbPath = dbPath;
222222
}

‎presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpMetadata.java‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515

1616
import com.facebook.presto.common.type.ArrayType;
1717
import com.facebook.presto.common.type.RowType;
18-
import com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType;
1918
import com.facebook.presto.spi.ColumnMetadata;
2019
import com.facebook.presto.spi.ConnectorTableMetadata;
2120
import com.facebook.presto.spi.SchemaTableName;
@@ -35,32 +34,41 @@
3534
import static com.facebook.presto.common.type.DoubleType.DOUBLE;
3635
import static com.facebook.presto.common.type.VarcharType.VARCHAR;
3736
import static com.facebook.presto.plugin.clp.ClpMetadata.DEFAULT_SCHEMA_NAME;
37+
import static com.facebook.presto.plugin.clp.ClpMetadataDbSetUp.DbHandle;
38+
import static com.facebook.presto.plugin.clp.ClpMetadataDbSetUp.getDbHandle;
39+
import static com.facebook.presto.plugin.clp.ClpMetadataDbSetUp.setupMetadata;
40+
import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.Boolean;
41+
import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.ClpString;
42+
import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.Float;
43+
import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.Integer;
44+
import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.UnstructuredArray;
45+
import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.VarString;
3846
import static com.facebook.presto.testing.TestingConnectorSession.SESSION;
3947
import static org.testng.Assert.assertEquals;
4048

4149
@Test(singleThreaded = true)
4250
public class TestClpMetadata
4351
{
4452
private static final String TABLE_NAME = "test";
45-
private ClpMetadataDbSetUp.DbHandle dbHandle;
53+
private DbHandle dbHandle;
4654
private ClpMetadata metadata;
4755

4856
@BeforeMethod
4957
public void setUp()
5058
{
51-
dbHandle = ClpMetadataDbSetUp.getDbHandle("metadata_testdb");
52-
metadata = ClpMetadataDbSetUp.setupMetadata(
59+
dbHandle = getDbHandle("metadata_testdb");
60+
metadata = setupMetadata(
5361
dbHandle,
5462
ImmutableMap.of(
5563
TABLE_NAME,
5664
ImmutableList.of(
57-
new Pair<>("a", ClpSchemaTreeNodeType.Integer),
58-
new Pair<>("a", ClpSchemaTreeNodeType.VarString),
59-
new Pair<>("b", ClpSchemaTreeNodeType.Float),
60-
new Pair<>("b", ClpSchemaTreeNodeType.ClpString),
61-
new Pair<>("c.d", ClpSchemaTreeNodeType.Boolean),
62-
new Pair<>("c.e", ClpSchemaTreeNodeType.VarString),
63-
new Pair<>("f.g.h", ClpSchemaTreeNodeType.UnstructuredArray))));
65+
new Pair<>("a", Integer),
66+
new Pair<>("a", ClpString),
67+
new Pair<>("b", Float),
68+
new Pair<>("b", ClpString),
69+
new Pair<>("c.d", Boolean),
70+
new Pair<>("c.e", VarString),
71+
new Pair<>("f.g.h", UnstructuredArray))));
6472
}
6573

6674
@AfterMethod

0 commit comments

Comments
 (0)