Skip to content

consolidate ValueType and ExprType#10355

Closed
clintropolis wants to merge 4 commits intoapache:masterfrom
clintropolis:expr-types
Closed

consolidate ValueType and ExprType#10355
clintropolis wants to merge 4 commits intoapache:masterfrom
clintropolis:expr-types

Conversation

@clintropolis
Copy link
Copy Markdown
Member

Description

Follow-up to #9638, this PR changes the expression system to operate on ValueType instead of ExprType. There should be no side-effects from this change, even though ExprType was missing FLOAT and COMPLEX, because ExprType was typically used to control explicitly supported types and otherwise explode. Input types are currently detected from the type of object when resolving identifiers, so there isn't risk of these unhandled types leaking into the expression system. A method toExpressionType has been added to convert a ValueType into the types which the expression system does understand (float into double, complex into an exception), for the SQL planning layer to use, (this was pushed down from Expressions utility class).

The only strange-ness introduced in this PR is that when implementing expressions we should ignore these unhandled types, but it seems worth it for having a single type system enum to use in all layers, as well as being able to share the utility methods available on ValueType.


This PR has:

  • been self-reviewed.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

Copy link
Copy Markdown
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

LGTM

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Sep 4, 2020

Because the expression system doesn't support all of the normal Druid ValueTypes, it might actually be better to keep them separate, since it makes things more type-safe. (We know that if we have an ExprType, then that represents a ValueType that the expression system can handle.)

@clintropolis
Copy link
Copy Markdown
Member Author

Because the expression system doesn't support all of the normal Druid ValueTypes, it might actually be better to keep them separate, since it makes things more type-safe. (We know that if we have an ExprType, then that represents a ValueType that the expression system can handle.)

Yeah, I think long term we definitely do want to consolidate these, but maybe I am jumping ahead a bit and we should wait until Expr can handle all ValueType. I will close this PR for now.

@clintropolis clintropolis mentioned this pull request Sep 9, 2020
8 tasks
@clintropolis clintropolis deleted the expr-types branch October 2, 2020 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants