[WIP] Support for user-defined enums #14691

daniel-ohayon · 2020-06-22T03:17:39Z

The goal of this PR is to enable basic support for enum user-defined types (UDTs) in Presto. Enums already exist in Postgres and MySQL, and this would be a first step toward generic UDT support.

Requirements

Support for both integer-valued (64-bit) and string-valued enums
Enum types should be provided by plugins and kept up-to-date as they change (without the need to restart the server)
Enum literals should be supported, with case-insensitive type names, but case-sensitive keys eg Mood 'HAPPY'
CAST from primitive types to enums should be supported, failing if the provided value is not part of the enum
NULLs are allowed
Both enum keys and values must be unique for a given enum
Enum definition should be forwarded to clients (including the Presto CLI) for custom rendering
The fact that a given column is an enum should be persisted in table schemas
You can compare enum values of the same type (EQUAL, NOT_EQUAL operators)
[?] You can compare an enum value to a literal of the underlying primitive type, but not to a reference, i.e. my_enum_column = 4 is OK, but my_enum_column = my_int_column is not. This would be in line with enum behavior in languages like Hack, and it would facilitate backwards-compatibility of existing queries that do not use enum literals
[?] Support for ephemeral enum definitions forwarded by client session parameters

Implementation

Extend FunctionNamespaceManager to provide types
Cast functions generated on-the-fly for enums

TODO

[ ] Disambiguate key lookup from value lookup with string enums
[ ] Persist enum info to schema (separate PR)
[ ] Support =, != for enums
[ ] Support literal comparison
[ ] Move getTypeDynamically to TypeRegistry

…nager

linux-foundation-easycla · 2020-06-22T03:17:44Z

The committers are authorized under a signed CLA.

✅ Daniel Ohayon (fd40b7b, 6f3beeb, 0df59a1, 8ee5838, 39b3788, 8676ae6, a6a61de, f4d1c62)

daniel-ohayon · 2020-06-22T14:05:51Z

presto-client/src/main/java/com/facebook/presto/client/FixJsonDataUtils.java

@@ -69,7 +69,10 @@ private FixJsonDataUtils() {}
        }
        requireNonNull(columns, "columns is null");
        List<TypeSignature> signatures = columns.stream()
-                .map(column -> parseTypeSignature(column.getType()))
+                .map(column -> {
+                    TypeSignature displayType = column.getTypeMetadata().getDisplayType();


in this file, we use the type signature client-side to determine what kind of data we're dealing with.
Since the client doesn't know about the enum types, if it encounters a String value, it will default to base64 encoding as per:

if (value instanceof String) { return Base64.getDecoder().decode((String) value); }

For string-valued enums, this results in gibberish in the CLI output.
This is why I'm passing this extra info about the display type

daniel-ohayon · 2020-06-22T17:31:54Z

presto-cli/src/main/java/com/facebook/presto/cli/OutputHandler.java

        if (rowBuffer.size() >= MAX_BUFFERED_ROWS) {
            flush(false);
        }
    }

+    private List<?> prettyPrintRow(List<?> row, List<Column> columns)


actually this doesn't work with complex types (row, map, array...)

rebuild switch-case logic here

rongrong · 2020-06-22T17:46:00Z

presto-common/src/main/java/com/facebook/presto/common/type/IntegerEnumType.java

+    @Override
+    public boolean isComparable()
+    {
+        return false;


Enum types are not comparable?

They should be, but I haven't found a way to register the comparison functions dynamically (like we do for CAST) in a way that I'm happy with yet.

daniel-ohayon · 2020-06-22T21:56:51Z

presto-main/src/main/java/com/facebook/presto/metadata/FunctionManager.java

@@ -685,6 +685,16 @@ private boolean isMoreSpecificThan(ApplicableFunction left, ApplicableFunction r
        return boundVariables.isPresent();
    }

+    // Called by TypeRegistry to look up types from FunctionNamespaceManagers


TODO: move to TypeRegistry

rongrong · 2020-07-01T20:26:36Z

presto-common/src/main/java/com/facebook/presto/common/type/IntegerEnumType.java

+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class IntegerEnumType


Should we call it LongEnumType so there's no confusion? Integer can also mean specifically the SQL INTEGER type which is 32-bit (though using 32-bit is probably enough for an enum type so that would be an option as well?)

let's go for LongEnumType!

rongrong · 2020-07-01T20:29:26Z

presto-common/src/main/java/com/facebook/presto/common/type/EnumType.java

+public interface EnumType<T>
+        extends Type
+{
+    Map<String, T> getEntries();


getEntries sounds like it should return a list. Maybe explicitly call it getEnumNameToValueMap or just getEnumMap or getMap.

rongrong · 2020-07-01T20:37:52Z

presto-common/src/main/java/com/facebook/presto/common/type/VarcharType.java

@@ -23,12 +23,12 @@

 import static java.util.Collections.singletonList;

-public final class VarcharType
+public class VarcharType


You probably want to refactor VarcharType into AbstractVarcharType and VarcharType (similar to AbstractLongType and BigintType) and keep this as final. That way you also avoid the weird need to provide a type signature for the constructor.

rongrong · 2020-07-01T20:41:02Z

presto-main/src/main/java/com/facebook/presto/metadata/BuiltInFunctionNamespaceManager.java

@@ -901,6 +904,14 @@ private SpecializedFunctionKey doGetSpecializedFunctionKey(Signature signature)
        Iterable<SqlFunction> candidates = getFunctions(null, signature.getName());
        // search for exact match
        Type returnType = typeManager.getType(signature.getReturnType());
+
+        if (returnType instanceof EnumType && CastType.CAST.getCastName().equals(signature.getName())) {


Why is this logic in BuiltInFunctionNamespaceManager?

I reorganized it a bit in #14728 to have the enum-specific logic only live in EnumOperators (but we still have a call from BuiltinFunctionNamespaceManager - I think it makes sense to have logic there to handle those UDT functions on-the-fly since it's a "core" feature which is independent of any specific UDT).

rongrong · 2020-07-01T20:55:35Z

presto-client/src/main/java/com/facebook/presto/client/Column.java

+                name,
+                type.getTypeSignature().toString(),
+                new ClientTypeSignature(type.getTypeSignature()),
+                type.getTypeMetadata());
    }

    public Column(String name, TypeSignature signature)


I think this is what you can do: remove this constructor (Column(String name, TypeSignature signature)). There's no real use case for this one. Create a Type class in client (I think call it Type is fine, but if it feels too confusing we can call it ClientType), which wraps the string type, client type signature and enum maps.

rongrong · 2020-07-01T20:58:43Z

presto-cli/src/main/java/com/facebook/presto/cli/OutputHandler.java

        if (rowBuffer.size() >= MAX_BUFFERED_ROWS) {
            flush(false);
        }
    }

+    private List<?> prettyPrintRow(List<?> row, List<Column> columns)


I might just call this function processRow or transform.

daniel-ohayon added 7 commits June 21, 2020 19:50

add IntegerEnumType

fd40b7b

add StringEnumType

6f3beeb

dynamic cast functions for string/int -> enum

0df59a1

generate enum cast functions on-the-fly in BuiltinFunctionNamespaceMa…

8ee5838

…nager

provide user-defined types via FunctionNamespaceManager

39b3788

tests for enum casts

8676ae6

expose enum metadata to client, use it for serDe

a6a61de

daniel-ohayon requested a review from rongrong June 22, 2020 03:17

fix cast return type; implement missing getTypeMetadata for enums

f4d1c62

daniel-ohayon commented Jun 22, 2020

View reviewed changes

daniel-ohayon added 3 commits June 22, 2020 07:17

fix checkstyle violation

c3c2619

handle hive serialization

e60ac30

display enum keys in CLI output

2ccd5d1

daniel-ohayon commented Jun 22, 2020

View reviewed changes

rongrong reviewed Jun 22, 2020

View reviewed changes

daniel-ohayon commented Jun 22, 2020

View reviewed changes

enforce distinct values for enums

a47cf26

daniel-ohayon changed the title ~~[WIP] Support for user-defined enums~~ [WIP] Support for user-defined enums [language] Jun 23, 2020

support more standard operators for enums

11ecc02

daniel-ohayon marked this pull request as draft June 26, 2020 00:59

daniel-ohayon changed the title ~~[WIP] Support for user-defined enums [language]~~ [WIP] Support for user-defined enums Jun 26, 2020

daniel-ohayon mentioned this pull request Jun 26, 2020

Enums support #1: base types and operators #14728

Merged

rongrong reviewed Jul 1, 2020

View reviewed changes

daniel-ohayon closed this Nov 11, 2020

daniel-ohayon deleted the dohayon/enums-v4 branch November 11, 2020 23:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Support for user-defined enums #14691

[WIP] Support for user-defined enums #14691

daniel-ohayon commented Jun 22, 2020 •

edited

Loading

linux-foundation-easycla bot commented Jun 22, 2020 •

edited

Loading

daniel-ohayon Jun 22, 2020 •

edited

Loading

daniel-ohayon Jun 22, 2020

daniel-ohayon Jun 22, 2020

rongrong Jun 22, 2020

daniel-ohayon Jun 22, 2020

daniel-ohayon Jun 22, 2020

rongrong Jul 1, 2020

daniel-ohayon Jul 6, 2020

rongrong Jul 1, 2020

rongrong Jul 1, 2020

rongrong Jul 1, 2020

daniel-ohayon Jul 6, 2020

rongrong Jul 1, 2020

rongrong Jul 1, 2020

[WIP] Support for user-defined enums #14691

[WIP] Support for user-defined enums #14691

Conversation

daniel-ohayon commented Jun 22, 2020 • edited Loading

Requirements

Implementation

TODO

linux-foundation-easycla bot commented Jun 22, 2020 • edited Loading

daniel-ohayon Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-ohayon commented Jun 22, 2020 •

edited

Loading

linux-foundation-easycla bot commented Jun 22, 2020 •

edited

Loading

daniel-ohayon Jun 22, 2020 •

edited

Loading