-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Write mapping support #25124
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.
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
Write mapping support #25124
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,9 +17,16 @@ | |
| import com.facebook.presto.common.predicate.TupleDomain; | ||
| import com.facebook.presto.common.type.CharType; | ||
| import com.facebook.presto.common.type.DecimalType; | ||
| import com.facebook.presto.common.type.TimestampType; | ||
| import com.facebook.presto.common.type.Type; | ||
| import com.facebook.presto.common.type.UuidType; | ||
| import com.facebook.presto.common.type.VarcharType; | ||
| import com.facebook.presto.plugin.jdbc.mapping.ColumnMapping; | ||
| import com.facebook.presto.plugin.jdbc.mapping.WriteMapping; | ||
| import com.facebook.presto.plugin.jdbc.mapping.functions.BooleanWriteFunction; | ||
| import com.facebook.presto.plugin.jdbc.mapping.functions.DoubleWriteFunction; | ||
| import com.facebook.presto.plugin.jdbc.mapping.functions.LongWriteFunction; | ||
| import com.facebook.presto.plugin.jdbc.mapping.functions.SliceWriteFunction; | ||
| import com.facebook.presto.spi.ColumnHandle; | ||
| import com.facebook.presto.spi.ColumnMetadata; | ||
| import com.facebook.presto.spi.ConnectorSession; | ||
|
|
@@ -57,21 +64,41 @@ | |
|
|
||
| import static com.facebook.presto.common.type.BigintType.BIGINT; | ||
| import static com.facebook.presto.common.type.BooleanType.BOOLEAN; | ||
| import static com.facebook.presto.common.type.CharType.MAX_LENGTH; | ||
| import static com.facebook.presto.common.type.DateType.DATE; | ||
| import static com.facebook.presto.common.type.DoubleType.DOUBLE; | ||
| import static com.facebook.presto.common.type.IntegerType.INTEGER; | ||
| import static com.facebook.presto.common.type.RealType.REAL; | ||
| import static com.facebook.presto.common.type.SmallintType.SMALLINT; | ||
| import static com.facebook.presto.common.type.TimeType.TIME; | ||
| import static com.facebook.presto.common.type.TimeWithTimeZoneType.TIME_WITH_TIME_ZONE; | ||
| import static com.facebook.presto.common.type.TimestampType.TIMESTAMP; | ||
| import static com.facebook.presto.common.type.TimestampWithTimeZoneType.TIMESTAMP_WITH_TIME_ZONE; | ||
| import static com.facebook.presto.common.type.TinyintType.TINYINT; | ||
| import static com.facebook.presto.common.type.VarbinaryType.VARBINARY; | ||
| import static com.facebook.presto.common.type.Varchars.isVarcharType; | ||
| import static com.facebook.presto.plugin.jdbc.JdbcErrorCode.JDBC_ERROR; | ||
| import static com.facebook.presto.plugin.jdbc.JdbcWarningCode.USE_OF_DEPRECATED_CONFIGURATION_PROPERTY; | ||
| import static com.facebook.presto.plugin.jdbc.StandardReadMappings.jdbcTypeToPrestoType; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.bigintColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.booleanColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.charColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.dateColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.decimalColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.doubleColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.integerColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.jdbcTypeToPrestoType; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.realColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.smallintColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.timeColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.timeWithTimeZoneColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.timestampColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.timestampWithTimeZoneColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.tinyintColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.uuidColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.varbinaryColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.varcharColumnMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.WriteMapping.booleanMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.WriteMapping.longMapping; | ||
| import static com.facebook.presto.plugin.jdbc.mapping.WriteMapping.sliceMapping; | ||
| import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND; | ||
| import static com.facebook.presto.spi.StandardErrorCode.NOT_SUPPORTED; | ||
| import static com.google.common.base.MoreObjects.firstNonNull; | ||
|
|
@@ -94,21 +121,21 @@ public class BaseJdbcClient | |
| { | ||
| private static final Logger log = Logger.get(BaseJdbcClient.class); | ||
|
|
||
| private static final Map<Type, String> SQL_TYPES = ImmutableMap.<Type, String>builder() | ||
| .put(BOOLEAN, "boolean") | ||
| .put(BIGINT, "bigint") | ||
| .put(INTEGER, "integer") | ||
| .put(SMALLINT, "smallint") | ||
| .put(TINYINT, "tinyint") | ||
| .put(DOUBLE, "double precision") | ||
| .put(REAL, "real") | ||
| .put(VARBINARY, "varbinary") | ||
| .put(DATE, "date") | ||
| .put(TIME, "time") | ||
| .put(TIME_WITH_TIME_ZONE, "time with timezone") | ||
| .put(TIMESTAMP, "timestamp") | ||
| .put(TIMESTAMP_WITH_TIME_ZONE, "timestamp with timezone") | ||
| .put(UuidType.UUID, "uuid") | ||
| private static final Map<Type, WriteMapping> TYPE_MAPPINGS = ImmutableMap.<Type, WriteMapping>builder() | ||
| .put(BOOLEAN, booleanMapping("boolean", (BooleanWriteFunction) booleanColumnMapping().getWriteFunction())) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these explicit casts to respective writeFunctions needed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes these are the default write functions for these types & will be used when getting a write function for a specific type. |
||
| .put(BIGINT, longMapping("bigint", (LongWriteFunction) bigintColumnMapping().getWriteFunction())) | ||
| .put(INTEGER, longMapping("integer", (LongWriteFunction) integerColumnMapping().getWriteFunction())) | ||
| .put(SMALLINT, longMapping("smallint", (LongWriteFunction) smallintColumnMapping().getWriteFunction())) | ||
| .put(TINYINT, longMapping("tinyint", (LongWriteFunction) tinyintColumnMapping().getWriteFunction())) | ||
| .put(DOUBLE, WriteMapping.doubleMapping("double precision", (DoubleWriteFunction) doubleColumnMapping().getWriteFunction())) | ||
| .put(REAL, longMapping("real", (LongWriteFunction) realColumnMapping().getWriteFunction())) | ||
| .put(VARBINARY, sliceMapping("varbinary", (SliceWriteFunction) varbinaryColumnMapping().getWriteFunction())) | ||
| .put(DATE, longMapping("date", (LongWriteFunction) dateColumnMapping().getWriteFunction())) | ||
| .put(TIME, longMapping("time", (LongWriteFunction) timeColumnMapping().getWriteFunction())) | ||
| .put(UuidType.UUID, sliceMapping("uuid", (SliceWriteFunction) uuidColumnMapping().getWriteFunction())) | ||
pratyakshsharma marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| .put(TIME_WITH_TIME_ZONE, longMapping("time with timezone", (LongWriteFunction) timeWithTimeZoneColumnMapping().getWriteFunction())) | ||
| .put(TIMESTAMP_WITH_TIME_ZONE, longMapping("timestamp with timezone", (LongWriteFunction) timestampWithTimeZoneColumnMapping().getWriteFunction())) | ||
| .build(); | ||
|
|
||
| protected final String connectorId; | ||
|
|
@@ -246,7 +273,7 @@ public List<JdbcColumnHandle> getColumns(ConnectorSession session, JdbcTableHand | |
| resultSet.getString("TYPE_NAME"), | ||
| resultSet.getInt("COLUMN_SIZE"), | ||
| resultSet.getInt("DECIMAL_DIGITS")); | ||
| Optional<ReadMapping> columnMapping = toPrestoType(session, typeHandle); | ||
| Optional<ColumnMapping> columnMapping = toPrestoType(session, typeHandle); | ||
| // skip unsupported column types | ||
| if (columnMapping.isPresent()) { | ||
| String columnName = resultSet.getString("COLUMN_NAME"); | ||
|
|
@@ -272,7 +299,7 @@ public List<JdbcColumnHandle> getColumns(ConnectorSession session, JdbcTableHand | |
| } | ||
|
|
||
| @Override | ||
| public Optional<ReadMapping> toPrestoType(ConnectorSession session, JdbcTypeHandle typeHandle) | ||
| public Optional<ColumnMapping> toPrestoType(ConnectorSession session, JdbcTypeHandle typeHandle) | ||
| { | ||
| return jdbcTypeToPrestoType(typeHandle); | ||
| } | ||
|
|
@@ -408,7 +435,7 @@ private String getColumnString(ColumnMetadata column, String columnName) | |
| StringBuilder sb = new StringBuilder() | ||
| .append(quoted(columnName)) | ||
| .append(" ") | ||
| .append(toSqlType(column.getType())); | ||
| .append(toWriteMapping(column.getType()).getDataType()); | ||
| if (!column.isNullable()) { | ||
| sb.append(" NOT NULL"); | ||
| } | ||
|
|
@@ -752,28 +779,45 @@ protected void execute(Connection connection, String query) | |
| } | ||
| } | ||
|
|
||
| protected String toSqlType(Type type) | ||
| public WriteMapping toWriteMapping(Type type) | ||
| { | ||
| String dataType; | ||
| if (isVarcharType(type)) { | ||
| VarcharType varcharType = (VarcharType) type; | ||
| if (varcharType.isUnbounded()) { | ||
| return "varchar"; | ||
| dataType = "varchar"; | ||
| } | ||
| return "varchar(" + varcharType.getLengthSafe() + ")"; | ||
| else { | ||
| dataType = "varchar(" + varcharType.getLengthSafe() + ")"; | ||
| } | ||
| return sliceMapping(dataType, (SliceWriteFunction) varcharColumnMapping(varcharType).getWriteFunction()); | ||
| } | ||
| else if (type instanceof CharType) { | ||
| CharType charType = (CharType) type; | ||
| if (charType.getLength() == MAX_LENGTH) { | ||
| dataType = "char"; | ||
| } | ||
| else { | ||
| dataType = "char(" + ((CharType) type).getLength() + ")"; | ||
| } | ||
| return sliceMapping(dataType, (SliceWriteFunction) charColumnMapping(charType).getWriteFunction()); | ||
| } | ||
| if (type instanceof CharType) { | ||
| if (((CharType) type).getLength() == CharType.MAX_LENGTH) { | ||
| return "char"; | ||
| else if (type instanceof DecimalType) { | ||
| DecimalType decimalType = (DecimalType) type; | ||
| dataType = format("decimal(%s, %s)", ((DecimalType) type).getPrecision(), ((DecimalType) type).getScale()); | ||
| if (decimalType.isShort()) { | ||
| return longMapping(dataType, (LongWriteFunction) decimalColumnMapping(decimalType).getWriteFunction()); | ||
| } | ||
| else { | ||
| return sliceMapping(dataType, (SliceWriteFunction) decimalColumnMapping(decimalType).getWriteFunction()); | ||
| } | ||
| return "char(" + ((CharType) type).getLength() + ")"; | ||
| } | ||
| if (type instanceof DecimalType) { | ||
| return format("decimal(%s, %s)", ((DecimalType) type).getPrecision(), ((DecimalType) type).getScale()); | ||
| else if (type instanceof TimestampType) { | ||
| return longMapping("timestamp", (LongWriteFunction) timestampColumnMapping((TimestampType) type).getWriteFunction()); | ||
| } | ||
|
|
||
| String sqlType = SQL_TYPES.get(type); | ||
| if (sqlType != null) { | ||
| return sqlType; | ||
| WriteMapping writeMapping = TYPE_MAPPINGS.get(type); | ||
| if (writeMapping != null) { | ||
| return writeMapping; | ||
| } | ||
| throw new PrestoException(NOT_SUPPORTED, "Unsupported column type: " + type.getDisplayName()); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we miss adding support for this since you have added support for TIME, TIME_WITH_TIME_ZONE and TIMESTAMP_WITH_TIME_ZONE.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I didn't see any implementations for them in the original (although I don't think they had proper support back then so it probably was not missed, just not needed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, all of them were not supported back then. Since you added support for some of them now, was just checking to see if you missed TIMESTAMP somehow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see - Timestamp types require some type info to get the correct write function so the implementation for timestamp is here:
https://github.com/prestodb/presto/pull/25124/files#diff-a585323eb9886bcb1569de59ffc9ba78fb29e5da66b43666aa0072a8de747172R816