Enums support #1: base types and operators #14728

daniel-ohayon · 2020-06-26T01:07:54Z

This PR is the first in a series that aims to introduce support for user-defined types, and specifically user-defined enums, into Presto (see #14691 for an overview of the work).

This one focuses specifically on introducing the base types and generic operators that apply to enums.
We support

enum literals, eg Mood.HAPPY
cast to and from base types
=, != and IS_DISTINCT
hash operator used for IN (...) and APPROX_DISTINCT()
cast to and from JSON
comparisons and ordering (using the rules of the underlying types)

Implementation notes

Enum literals like Mood.HAPPY are parsed as Dereferences in the AST (like my_table.my_col) and they are then rewritten as EnumLiterals in the TranslationMap's rewriter.
I added a new type bound constraint to express that we want a given type to be a long enum or varchar enum, so that we can have enum operator signatures like <T extends LongEnumType> equals(T, T): bool . This constraint is similar to the orderable and comparable constraint we use to define the type signature of functions like the equal operator on arrays.

In future PRs, I will introduce

the ability to register enum types from plugins
the ability to serialize enum data to the client

daniel-ohayon · 2020-07-07T16:50:19Z

@rongrong I addressed applicable comments from #14691 here

presto-main/src/main/java/com/facebook/presto/type/EnumOperators.java

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

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

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

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

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

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

presto-main/src/main/java/com/facebook/presto/type/EnumOperators.java

kaikalur · 2020-07-17T20:16:00Z

I'm not sure we should allow literals. They should only be a cast like: MOOD('Happy'). We don't want to create C++-like syntax where you need to know if something is a type before parsing. This is a going to create a lot of hassles and bug-prone.

daniel-ohayon · 2020-07-17T22:20:00Z

I'm not sure we should allow literals. They should only be a cast like: MOOD('Happy').

@kaikalur are you suggesting that we introduce a UDF with the same name as each enum type that takes in a varchar key and returns the matching enum object?

We don't want to create C++-like syntax where you need to know if something is a type before parsing. This is a going to create a lot of hassles and bug-prone.

Can you elaborate on this? How is that syntax different from what we already support with eg DATE '2018-01-05' and JSON '{"hello" 1}' ? what new bugs and hassles would it introduce that these two don't?

kaikalur · 2020-07-17T22:22:28Z

I'm not sure we should allow literals. They should only be a cast like: MOOD('Happy').

@kaikalur are you suggesting that we introduce a UDF with the same name as each enum type that takes in a varchar key and returns the matching enum object?

We don't want to create C++-like syntax where you need to know if something is a type before parsing. This is a going to create a lot of hassles and bug-prone.

Can you elaborate on this? How is that syntax different from what we already support with eg DATE '2018-01-05' and JSON '{"hello" 1}' ? what new bugs and hassles would it introduce that these two don't?

DATE literal is actually a thing in the spec. And DATE is a keyword whereas here you will have to allow any enum type to precede the literal which is not very spec-like and not context-free grammar anymore.

daniel-ohayon · 2020-07-17T22:30:29Z

I'm not too attached to any particular syntax, but the generic type literal is already there in the Presto parser, I didn't change that:
https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/sql/planner/LiteralInterpreter.java#L252

We already have the behavior of parsing type literals in a generic way when we encounter them and looking for a cast: varchar -> type so if you don't think that's a good idea, maybe that part should be removed from the parser too.

kaikalur · 2020-07-17T22:40:08Z

Wow! That's a bad idea. I haven't seen it before. Let me look. Get Outlook for Android<https://aka.ms/ghei36>

…

________________________________ From: Daniel Ohayon <notifications@github.com> Sent: Friday, July 17, 2020 3:30:43 PM To: prestodb/presto <presto@noreply.github.com> Cc: Sreeni Viswanadha <viswanadha@fb.com>; Mention <mention@noreply.github.com> Subject: Re: [prestodb/presto] Enums support #1: base types and operators (#14728) I'm not too attached to any particular syntax, but the generic type literal is already there in the Presto parser, I didn't change that: https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/sql/planner/LiteralInterpreter.java#L252 We already have the behavior of parsing type literals in a generic way when we encounter them and looking for a cast: varchar -> type so if you don't think that's a good idea, maybe that part should be removed from the parser too. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#14728 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAPNYAPOCGX7OQVSIALWSWTR4DGJHANCNFSM4OI4EEMQ>.

rongrong

Add long and varchar enum types looks good. Some nits only.

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

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

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

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

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

rongrong

You might want to separate Add enum literals and operators to two commits: Support enum literals and Add enum operators. They are not quite related.

presto-docs/src/main/sphinx/develop/functions.rst

presto-spi/src/main/java/com/facebook/presto/spi/function/TypeParameter.java

presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionAnalyzer.java

presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionTreeUtils.java

rongrong

For Support type bound on @TypeParameter, are you planning to make the signature look like <T extends SomeType>? Please update the commit message to either use the exact term in type signature, or use generic description rather than the approximate Java syntax.

presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionTreeUtils.java

rongrong · 2020-07-31T20:24:21Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionTreeUtils.java

+        String enumKey = qualifiedName.getSuffix().toLowerCase(ENGLISH);
+        Object enumValue = enumType.getEnumMap().get(enumKey);
+        if (enumValue == null) {
+            throw new SemanticException(INVALID_LITERAL, node, format("No key '%s' in enum '%s'", enumKey, enumType.getDisplayName()));


This function is too low level to throw an SemanticException. If you expect this function to be only called when node is a valid enum, it should throw IllegalArgumentException. You can change these logic to a checkArgument.

Is it OK to have errors triggered by a checkArgument surfaced to users?
This is the only place where we check whether the provided key exists in the enum, so it will be very possible that users see this error.

If it's not OK, we should add a catch at the correct level and translate the IllegalArgumentException to a SemanticException.

presto-main/src/main/java/com/facebook/presto/sql/analyzer/SemanticExceptions.java

presto-main/src/main/java/com/facebook/presto/sql/planner/TranslationMap.java

presto-main/src/main/java/com/facebook/presto/sql/planner/sanity/PlanChecker.java

presto-main/src/main/java/com/facebook/presto/sql/planner/TranslationMap.java

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

presto-main/src/main/java/com/facebook/presto/util/JsonUtil.java

daniel-ohayon · 2020-08-18T03:51:12Z

@rongrong could you have another look at Add long and varchar enum types ?
As per our conversation, I implemented full type signatures for enums containing the enum definition (with serde logic).

rongrong

The structure looks good overall. Feedback on some details. Please also add more tests about valid and invalid enum creation / serde.

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

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

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

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

daniel-ohayon · 2020-08-20T23:11:13Z

addressed latest feedback and added extra tests to TestTypeSignature

rongrong

First commit looks good. Only some nits.

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

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

presto-common/src/test/java/com/facebook/presto/common/type/TestTypeSignature.java

presto-main/src/main/java/com/facebook/presto/type/LongEnumParametricType.java

presto-main/src/main/java/com/facebook/presto/type/VarcharEnumParametricType.java

Add an is-a constraint for type variables, so we can describe generic types of a certain kind.

rongrong

Support enum literals in queries looks good. Only one nit.

presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionTreeUtils.java

rongrong

Wow this looks so clean now!

presto-main/src/main/java/com/facebook/presto/util/JsonUtil.java

presto-main/src/main/java/com/facebook/presto/type/VarcharEnumOperators.java

rongrong

LGTM. Please fix the test failure and we are good to go!

presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionTreeUtils.java

Support common operators like `=` on enum types

daniel-ohayon requested a review from rongrong June 26, 2020 01:08

daniel-ohayon mentioned this pull request Jul 6, 2020

[WIP] Support for user-defined enums #14691

Closed

daniel-ohayon mentioned this pull request Jul 10, 2020

Enums support #2: provide enums via FunctionNamespaceManagers #14828

Closed

daniel-ohayon force-pushed the dohayon-enum-types branch 3 times, most recently from 7115366 to 29d596c Compare July 16, 2020 02:23

rongrong reviewed Jul 16, 2020

View reviewed changes

daniel-ohayon force-pushed the dohayon-enum-types branch 3 times, most recently from cf37c7d to cf63560 Compare July 22, 2020 03:20

daniel-ohayon requested a review from rongrong July 22, 2020 05:27

rongrong reviewed Jul 22, 2020

View reviewed changes

daniel-ohayon force-pushed the dohayon-enum-types branch 3 times, most recently from 11636ea to 9833151 Compare July 24, 2020 21:43

tejasapatil reviewed Jul 25, 2020

View reviewed changes

presto-common/src/main/java/com/facebook/presto/common/type/LongEnumType.java Show resolved Hide resolved

daniel-ohayon force-pushed the dohayon-enum-types branch 3 times, most recently from 7da5d10 to 88a0f14 Compare July 27, 2020 02:43

rongrong reviewed Jul 27, 2020

View reviewed changes

daniel-ohayon force-pushed the dohayon-enum-types branch 2 times, most recently from 8faecd0 to 0b8e3e1 Compare July 28, 2020 15:45

daniel-ohayon force-pushed the dohayon-enum-types branch from b56e3ef to d0e29b7 Compare July 31, 2020 00:40

rongrong reviewed Jul 31, 2020

View reviewed changes

daniel-ohayon force-pushed the dohayon-enum-types branch from d0e29b7 to 555ed46 Compare August 18, 2020 03:46

daniel-ohayon force-pushed the dohayon-enum-types branch 4 times, most recently from ba08218 to dfd4d95 Compare August 19, 2020 16:34

rongrong reviewed Aug 19, 2020

View reviewed changes

daniel-ohayon force-pushed the dohayon-enum-types branch from dfd4d95 to 1c07c54 Compare August 20, 2020 23:10

daniel-ohayon force-pushed the dohayon-enum-types branch 2 times, most recently from 90e139c to 74e8a69 Compare August 23, 2020 23:33

rongrong reviewed Aug 24, 2020

View reviewed changes

daniel-ohayon added 2 commits August 24, 2020 17:33

Add long and varchar enum types

507127b

Support type bound in TypeVariableConstraint

1d6ea3b

Add an is-a constraint for type variables, so we can describe generic types of a certain kind.

daniel-ohayon force-pushed the dohayon-enum-types branch from 74e8a69 to 237f05b Compare August 24, 2020 22:35

rongrong reviewed Aug 27, 2020

View reviewed changes

presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionTreeUtils.java Outdated Show resolved Hide resolved

rongrong reviewed Aug 27, 2020

View reviewed changes

presto-main/src/main/java/com/facebook/presto/util/JsonUtil.java Show resolved Hide resolved

rongrong reviewed Aug 27, 2020

View reviewed changes

presto-main/src/main/java/com/facebook/presto/type/VarcharEnumOperators.java Show resolved Hide resolved

daniel-ohayon force-pushed the dohayon-enum-types branch from 237f05b to 252b098 Compare August 28, 2020 16:28

rongrong reviewed Aug 28, 2020

View reviewed changes

presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionTreeUtils.java Outdated Show resolved Hide resolved

daniel-ohayon added 2 commits August 28, 2020 14:48

Support enum literals in queries

db53af7

Add enum operators

4a60a66

Support common operators like `=` on enum types

daniel-ohayon force-pushed the dohayon-enum-types branch from 252b098 to 4a60a66 Compare August 28, 2020 19:48

rongrong approved these changes Aug 31, 2020

View reviewed changes

rongrong merged commit 99e67bc into prestodb:master Aug 31, 2020

caithagoras mentioned this pull request Sep 9, 2020

Add release notes for 0.241 #15143

Merged

3 tasks

rongrong mentioned this pull request Oct 9, 2020

Core support for enums, take #2 #15219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enums support #1: base types and operators #14728

Enums support #1: base types and operators #14728

daniel-ohayon commented Jun 26, 2020 •

edited

Loading

daniel-ohayon commented Jul 7, 2020

kaikalur commented Jul 17, 2020

daniel-ohayon commented Jul 17, 2020

kaikalur commented Jul 17, 2020 •

edited

Loading

daniel-ohayon commented Jul 17, 2020

kaikalur commented Jul 17, 2020 via email

rongrong left a comment

rongrong left a comment

rongrong left a comment

rongrong Jul 31, 2020

daniel-ohayon Aug 10, 2020

rongrong Aug 10, 2020

daniel-ohayon commented Aug 18, 2020

rongrong left a comment

daniel-ohayon commented Aug 20, 2020

rongrong left a comment

rongrong left a comment

rongrong left a comment

rongrong left a comment

Enums support #1: base types and operators #14728

Enums support #1: base types and operators #14728

Conversation

daniel-ohayon commented Jun 26, 2020 • edited Loading

Implementation notes

daniel-ohayon commented Jul 7, 2020

kaikalur commented Jul 17, 2020

daniel-ohayon commented Jul 17, 2020

kaikalur commented Jul 17, 2020 • edited Loading

daniel-ohayon commented Jul 17, 2020

kaikalur commented Jul 17, 2020 via email

rongrong left a comment

Choose a reason for hiding this comment

rongrong left a comment

Choose a reason for hiding this comment

rongrong left a comment

Choose a reason for hiding this comment

rongrong Jul 31, 2020

Choose a reason for hiding this comment

daniel-ohayon Aug 10, 2020

Choose a reason for hiding this comment

rongrong Aug 10, 2020

Choose a reason for hiding this comment

daniel-ohayon commented Aug 18, 2020

rongrong left a comment

Choose a reason for hiding this comment

daniel-ohayon commented Aug 20, 2020

rongrong left a comment

Choose a reason for hiding this comment

rongrong left a comment

Choose a reason for hiding this comment

rongrong left a comment

Choose a reason for hiding this comment

rongrong left a comment

Choose a reason for hiding this comment

daniel-ohayon commented Jun 26, 2020 •

edited

Loading

kaikalur commented Jul 17, 2020 •

edited

Loading