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

Jump to bottom

DBZ-1413 Support PostgreSQL Domain and Enum types #1079

Merged
merged 13 commits into from Dec 10, 2019
Merged

DBZ-1413 Support PostgreSQL Domain and Enum types #1079

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
d5d0aa5
DBZ-1413 Support PostgreSQL domain types
Naros Oct 15, 2019
774f2ed
DBZ-1413 Align domain type support across all 3 decoders
Naros Nov 5, 2019
222fdf9
DBZ-1413 Delay building type hierarchy after all base types built
Naros Nov 25, 2019
c788b47
DBZ-1413 Updated tests
Naros Nov 25, 2019
3efef48
DBZ-1413 Remove some System.out madness
Naros Nov 25, 2019
36e1b06
DBZ-920 Add support for PostgreSQL enum types
Naros Nov 25, 2019
ad97bd3
DBZ-1413 DBZ-920 Document Domain Type and ENUM support
Naros Nov 25, 2019
630c849
DBZ-1413 Suggested Javadoc changes, added/fix tests, changed how Colu…
Naros Dec 2, 2019
a9a39a5
DBZ-1413 DBZ-920 Updated documentation with suggested changes
Naros Dec 2, 2019
9f43d96
DBZ-1413 Revert change to pom.xml for PostgreSQL driver upgrade.
Naros Dec 4, 2019
4389c16
DBZ-1413 Revert AbstractColumnValue changes in commit 774f2ed
Naros Dec 5, 2019
53939f8
DBZ-1413 Resolve column length/scale when reading table column metada…
Naros Dec 6, 2019
c0482ff
DBZ-1413 Fix format violation
Naros Dec 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
61 changes: 51 additions & 10 deletions debezium-connector-postgres/src/main/java/io/debezium/connector/postgresql/PostgresType.java
View file
Open in desktop
Expand Up @@ -19,25 +19,29 @@
*/
public class PostgresType {

public static final PostgresType UNKNOWN = new PostgresType("unknown", -1, Integer.MIN_VALUE, null);
public static final PostgresType UNKNOWN = new PostgresType("unknown", -1, Integer.MIN_VALUE, null, null, null);

private final String name;
private final int oid;
private final int jdbcId;
private final PostgresType baseType;
private final PostgresType elementType;
private final TypeInfo typeInfo;
private final int modifiers;

public PostgresType(String name, int oid, int jdbcId, TypeInfo typeInfo) {
this(name, oid, jdbcId, typeInfo, null);
public PostgresType(String name, int oid, int jdbcId, TypeInfo typeInfo, PostgresType baseType, PostgresType elementType) {
this(name, oid, jdbcId, TypeRegistry.NO_TYPE_MODIFIER, typeInfo, baseType, elementType);
}

public PostgresType(String name, int oid, int jdbcId, TypeInfo typeInfo, PostgresType elementType) {
public PostgresType(String name, int oid, int jdbcId, int modifiers, TypeInfo typeInfo, PostgresType baseType, PostgresType elementType) {
Naros marked this conversation as resolved.
Show resolved Hide resolved
Objects.requireNonNull(name);
this.name = name;
this.oid = oid;
this.jdbcId = jdbcId;
this.elementType = elementType;
this.typeInfo = typeInfo;
this.baseType = baseType;
this.elementType = elementType;
this.modifiers = modifiers;
}

/**
Expand All @@ -47,6 +51,13 @@ public boolean isArrayType() {
return elementType != null;
}

/**
* @return true if this type is a base type
Copy link
Member

Choose a reason for hiding this comment

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

What is a "base type"? Can you clarify in the JavaDoc?

Copy link
Member Author

@Naros Naros Dec 4, 2019 •
edited

Choose a reason for hiding this comment

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

In the recent commits, I decided to expand on this with the notion that a PostgresType has both a base type, which is documented but also allows for fetching a type's root type, the top most type in the type hierarchy. This proved nice to abstract the iterative nature away where we needed this as part of the type.

*/
public boolean isBaseType() {
return baseType == null;
}

/**
*
* @return symbolic name of the type
Expand Down Expand Up @@ -79,6 +90,14 @@ public PostgresType getElementType() {
return elementType;
}

/**
*
* @return the base postgres type this type is based upon
*/
public PostgresType getBaseType() {
return baseType;
}

/**
*
* @return the default length of the type
Expand All @@ -87,9 +106,23 @@ public int getDefaultLength() {
if (typeInfo == null) {
return TypeRegistry.UNKNOWN_LENGTH;
}
int size = typeInfo.getPrecision(oid, TypeRegistry.NO_TYPE_MODIFIER);
if (baseType != null) {
if (modifiers == TypeRegistry.NO_TYPE_MODIFIER) {
return baseType.getDefaultLength();
}
else {
int size = typeInfo.getPrecision(baseType.getOid(), modifiers);
if (size == 0) {
size = typeInfo.getDisplaySize(baseType.getOid(), modifiers);
}
if (size != 0 && size != Integer.MAX_VALUE) {
return size;
}
}
}
int size = typeInfo.getPrecision(oid, modifiers);
if (size == 0) {
size = typeInfo.getDisplaySize(oid, TypeRegistry.NO_TYPE_MODIFIER);
size = typeInfo.getDisplaySize(oid, modifiers);
}
return size;
}
Expand All @@ -102,7 +135,15 @@ public int getDefaultScale() {
if (typeInfo == null) {
return TypeRegistry.UNKNOWN_LENGTH;
}
return typeInfo.getScale(oid, TypeRegistry.NO_TYPE_MODIFIER);
if (baseType != null) {
if (modifiers == TypeRegistry.NO_TYPE_MODIFIER) {
return baseType.getDefaultScale();
}
else {
return typeInfo.getScale(baseType.getOid(), modifiers);
}
}
return typeInfo.getScale(oid, modifiers);
}

/**
Expand Down Expand Up @@ -179,7 +220,7 @@ public boolean equals(Object obj) {

@Override
public String toString() {
return "PostgresType [name=" + name + ", oid=" + oid + ", jdbcId=" + jdbcId + ", defaultLength=" + getDefaultLength()
+ ", defaultScale=" + getDefaultScale() + ", elementType=" + elementType + "]";
return "PostgresType [name=" + name + ", oid=" + oid + ", jdbcId=" + jdbcId + ", modifiers=" + modifiers + ", defaultLength=" + getDefaultLength()
+ ", defaultScale=" + getDefaultScale() + ", baseType=" + baseType + ", elementType=" + elementType + "]";
}
}
58 changes: 58 additions & 0 deletions ...ector-postgres/src/main/java/io/debezium/connector/postgresql/PostgresValueConverter.java
View file
Open in desktop
Expand Up @@ -151,7 +151,10 @@ protected PostgresValueConverter(Charset databaseCharset, DecimalMode decimalMod
@Override
public SchemaBuilder schemaBuilder(Column column) {
int oidValue = column.nativeType();
return schemaBuilder(oidValue, column);
}

private SchemaBuilder schemaBuilder(int oidValue, Column column) {
switch (oidValue) {
case PgOid.BIT:
case PgOid.BIT_ARRAY:
Expand Down Expand Up @@ -277,6 +280,7 @@ else if (oidValue == typeRegistry.citextArrayOid()) {
else if (oidValue == typeRegistry.ltreeArrayOid()) {
return SchemaBuilder.array(Ltree.builder().optional().build());
}

final SchemaBuilder jdbcSchemaBuilder = super.schemaBuilder(column);
if (jdbcSchemaBuilder == null) {
return includeUnknownDatatypes ? SchemaBuilder.bytes() : null;
Expand Down Expand Up @@ -309,7 +313,10 @@ private SchemaBuilder hstoreSchema() {
@Override
public ValueConverter converter(Column column, Field fieldDefn) {
int oidValue = column.nativeType();
return converter(oidValue, column, fieldDefn);
}

private ValueConverter converter(int oidValue, Column column, Field fieldDefn) {
switch (oidValue) {
case PgOid.BIT:
case PgOid.VARBIT:
Expand Down Expand Up @@ -417,6 +424,7 @@ else if (oidValue == typeRegistry.geometryArrayOid() ||
oidValue == typeRegistry.hstoreArrayOid()) {
return createArrayConverter(column, fieldDefn);
}

final ValueConverter jdbcConverter = super.converter(column, fieldDefn);
if (jdbcConverter == null) {
return includeUnknownDatatypes ? data -> convertBinary(column, fieldDefn, data) : null;
Expand Down Expand Up @@ -907,4 +915,54 @@ protected Object convertString(Column column, Field fieldDefn, Object data) {
}
return super.convertString(column, fieldDefn, data);
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

Let's avoid such empty comments. But could you add one to the method in the base class, describing its purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

By moving associating the right jdbc type and native type to a column at construction time, these Types.DISTINCT specific handler methods were removed as they're no longer needed.

*
* @param column
* @return
*/
@Override
protected SchemaBuilder distinctSchema(Column column) {
return schemaBuilder(getColumnWithDomainJdbcType(column, typeRegistry.get(column.nativeType())));
}

/**
* Provides a ValueConverter that properly resolves the domain type to base type for data of a given column
*
* @param column the column definition; never null
* @param fieldDefn the field definition; never null
* @return the value converter to convert the supplied data
*/
@Override
protected ValueConverter convertDistinct(Column column, Field fieldDefn) {
return converter(getColumnWithDomainJdbcType(column, typeRegistry.get(column.nativeType())), fieldDefn);
}

/**
* For a given column and type, traverse type hierarchy and return a column based on base type's JDBC type
*
* @param column the column
* @param postgresType the column's postgres type
* @return A new {@link Column} instance with appropriate native JDBC type
*/
private static Column getColumnWithDomainJdbcType(Column column, PostgresType postgresType) {
PostgresType baseType = postgresType;
while (!baseType.isBaseType()) {
baseType = baseType.getBaseType();
}

// This is necessary for situations where PostgresValueConverter delegates schema and converter resolution
// to JdbcValueConverters where the resolution is based on the column's jdbcType. For columns that use
// domain alias types, this is the OID to the alias type, not the actual base type.
//
// For example:
// CREATE DOMAIN bool2 bool default false;
// CREATE TABLE (pk serial, data bool2 not null);
//
// This guarantees that when the data column's schema and value converter is resolved, it's based on the
// fact the resolved base type is bool, not some oid that resolves to an unhandled type.
//
// Perhaps there are better ways - TBD.
return column.edit().jdbcType(baseType.getJdbcId()).nativeType(baseType.getOid()).create();
Copy link
Member

Choose a reason for hiding this comment

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

Hum, this side-effect of the get... method is unexpected. Could this be moved to the construction time?

Copy link
Member Author

Choose a reason for hiding this comment

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

Quite possibly, I'll look and let you know my findings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another option I explored as well was to instead make a small change to JdbcValueConverters where we move what the switch statements are based upon to a method argument and then delegate to those as needed, e.g.

    @Override
    public SchemaBuilder schemaBuilder(Column column) {
        return resolveSchemaBuilderByJdbcType(column.jdbcType(), column);
    }

    protected SchemaBuilder resolveSchemaBuilderByJdbcType(int jdbcType, Column column) {
        switch (jdbcType) {
            ...
        }
    }

This allows PostgresValueConverter to actually call those new resolve methods directly and rather than create a new Column instance solely for the sake of controlling the switch argument, we can instead change those methods like so:

    @Override
    protected SchemaBuilder distinctSchema(Column column) {
        final int rootTypeJdbcId = typeRegistry.get(column.nativeType()).getRootType().getJdbcId();
        return resolveSchemaBuilderByJdbcType(rootTypeJdbcId, column);
    }

I am still going to test your suggestion with Column construction to be sure there are no side in the event you don't particularly care for all this method delegation here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to go with your construction approach as it seemed to solve all the corner nuances without introducing any additional side effects.

}
}