Skip to content

Commit

Permalink
Hide support for PostgreSQL arrays behind a config switch
Browse files Browse the repository at this point in the history
As we want to replace existing mapping with something more suitable, the
first step is to provide a config toggle that will help during
transition period.
  • Loading branch information
findepi authored and electrum committed May 3, 2019
1 parent 69cb7ef commit 7df0eec
Show file tree
Hide file tree
Showing 7 changed files with 118 additions and 4 deletions.
5 changes: 5 additions & 0 deletions presto-postgresql/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@
<artifactId>joda-time</artifactId>
</dependency>

<dependency>
<groupId>javax.validation</groupId>
<artifactId>validation-api</artifactId>
</dependency>

<!-- Presto SPI -->
<dependency>
<groupId>io.prestosql</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import io.airlift.json.ObjectMapperProvider;
import io.airlift.slice.DynamicSliceOutput;
import io.airlift.slice.Slice;
Expand Down Expand Up @@ -97,12 +98,24 @@ public class PostgreSqlClient
private static final String DUPLICATE_TABLE_SQLSTATE = "42P07";

private final Type jsonType;
private final boolean supportArrays;

@Inject
public PostgreSqlClient(BaseJdbcConfig config, TypeManager typeManager)
public PostgreSqlClient(BaseJdbcConfig config, PostgreSqlConfig postgreSqlConfig, TypeManager typeManager)
{
super(config, "\"", new DriverConnectionFactory(new Driver(), config));
this.jsonType = typeManager.getType(new TypeSignature(StandardTypes.JSON));

switch (postgreSqlConfig.getArrayMapping()) {
case DISABLED:
supportArrays = false;
break;
case AS_ARRAY:
supportArrays = true;
break;
default:
throw new IllegalArgumentException("Unsupported ArrayMapping: " + postgreSqlConfig.getArrayMapping());
}
}

@Override
Expand Down Expand Up @@ -197,6 +210,9 @@ public List<JdbcColumnHandle> getColumns(ConnectorSession session, JdbcTableHand
private Map<String, Integer> getArrayColumnDimensions(Connection connection, JdbcTableHandle tableHandle)
throws SQLException
{
if (!supportArrays) {
return ImmutableMap.of();
}
String sql = "" +
"SELECT att.attname, att.attndims " +
"FROM pg_attribute att " +
Expand Down Expand Up @@ -237,7 +253,7 @@ public Optional<ColumnMapping> toPrestoType(ConnectorSession session, JdbcTypeHa
if (typeHandle.getJdbcType() == Types.TIMESTAMP) {
return Optional.of(timestampColumnMapping(session));
}
if (typeHandle.getJdbcType() == Types.ARRAY) {
if (typeHandle.getJdbcType() == Types.ARRAY && supportArrays) {
if (!typeHandle.getArrayDimensions().isPresent()) {
return Optional.empty();
}
Expand Down Expand Up @@ -278,7 +294,7 @@ public WriteMapping toWriteMapping(ConnectorSession session, Type type)
if (type.getTypeSignature().getBase().equals(StandardTypes.JSON)) {
return WriteMapping.sliceMapping("jsonb", typedVarcharWriteFunction("json"));
}
if (type instanceof ArrayType) {
if (type instanceof ArrayType && supportArrays) {
Type elementType = ((ArrayType) type).getElementType();
String elementDataType = toWriteMapping(session, elementType).getDataType();
return WriteMapping.blockMapping(elementDataType + "[]", arrayWriteFunction(session, elementType, getArrayElementPgTypeName(session, this, elementType)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,6 @@ public void configure(Binder binder)
{
binder.bind(JdbcClient.class).to(PostgreSqlClient.class).in(Scopes.SINGLETON);
configBinder(binder).bindConfig(BaseJdbcConfig.class);
configBinder(binder).bindConfig(PostgreSqlConfig.class);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* 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 io.prestosql.plugin.postgresql;

import io.airlift.configuration.Config;

import javax.validation.constraints.NotNull;

public class PostgreSqlConfig
{
private ArrayMapping arrayMapping = ArrayMapping.DISABLED;

public enum ArrayMapping {
DISABLED,
@Deprecated // TODO https://github.com/prestosql/presto/issues/682
AS_ARRAY,
}

@NotNull
public ArrayMapping getArrayMapping()
{
return arrayMapping;
}

@Config("postgresql.experimental.array-mapping")
public PostgreSqlConfig setArrayMapping(ArrayMapping arrayMapping)
{
this.arrayMapping = arrayMapping;
return this;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* 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 io.prestosql.plugin.postgresql;

import com.google.common.collect.ImmutableMap;
import io.airlift.configuration.testing.ConfigAssertions;
import org.testng.annotations.Test;

import java.util.Map;

public class TestPostgreSqlConfig
{
@Test
public void testDefaults()
{
ConfigAssertions.assertRecordedDefaults(ConfigAssertions.recordDefaults(PostgreSqlConfig.class)
.setArrayMapping(PostgreSqlConfig.ArrayMapping.DISABLED));
}

@Test
public void testExplicitPropertyMappings()
{
Map<String, String> properties = new ImmutableMap.Builder<String, String>()
.put("postgresql.experimental.array-mapping", "AS_ARRAY")
.build();

PostgreSqlConfig expected = new PostgreSqlConfig()
.setArrayMapping(PostgreSqlConfig.ArrayMapping.AS_ARRAY);

ConfigAssertions.assertFullMapping(properties, expected);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,13 @@ protected boolean supportsViews()
return false;
}

@Override
protected boolean supportsArrays()
{
// Arrays are supported conditionally. Check the defaults.
return new PostgreSqlConfig().getArrayMapping() != PostgreSqlConfig.ArrayMapping.DISABLED;
}

@Override
public void testCommentTable()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public TestPostgreSqlTypeMapping()

private TestPostgreSqlTypeMapping(TestingPostgreSqlServer postgreSqlServer)
{
super(() -> createPostgreSqlQueryRunner(postgreSqlServer, ImmutableMap.of(), ImmutableList.of()));
super(() -> createPostgreSqlQueryRunner(postgreSqlServer, ImmutableMap.of("postgresql.experimental.array-mapping", "AS_ARRAY"), ImmutableList.of()));
this.postgreSqlServer = postgreSqlServer;
}

Expand Down

0 comments on commit 7df0eec

Please sign in to comment.