Skip to content

Conversation

@caithagoras
Copy link
Contributor

@caithagoras caithagoras commented Jan 16, 2020

== RELEASE NOTES ==

General Changes
* Add support to show whether functions have variable arity in ``SHOW FUNCTIONS``.
* Add support to show whether functions are built-in in ``SHOW FUNCTIONS``.

@caithagoras caithagoras requested a review from rongrong January 16, 2020 21:10
@caithagoras caithagoras changed the title Expand SHOW FUNCTIONS Extend SHOW FUNCTIONS Jan 16, 2020
@caithagoras caithagoras removed the request for review from rongrong January 16, 2020 21:50
@caithagoras caithagoras added the wip Work In Progress label Jan 16, 2020
@caithagoras caithagoras changed the title Extend SHOW FUNCTIONS [WIP] Extend SHOW FUNCTIONS Jan 16, 2020
@caithagoras caithagoras force-pushed the s3 branch 2 times, most recently from 7a92bcc to 59dffa1 Compare January 16, 2020 22:38
@caithagoras caithagoras changed the title [WIP] Extend SHOW FUNCTIONS Extend SHOW FUNCTIONS Jan 16, 2020
@caithagoras caithagoras removed the wip Work In Progress label Jan 16, 2020
@caithagoras caithagoras requested a review from rongrong January 16, 2020 23:52
@caithagoras caithagoras force-pushed the s3 branch 7 times, most recently from e87a31e to ba6221d Compare January 23, 2020 09:26
@caithagoras
Copy link
Contributor Author

caithagoras commented Jan 23, 2020

@rongrong Removed new session properties and configuration properties.
I think we should kept the column in a relative logical order (starting from more important attributes, to the less important metadata, and description at the end).

Column name suggestions are welcome if needed.

@rongrong
Copy link
Contributor

I think we should kept the column in a relative logical order (starting from more important attributes, to the less important metadata, and description at the end).

Description is quite important (Personally I feel it's actually more important than type parameters and return types when I use SHOW FUNCTIONS). I agree it looks nicer to have all metadata together. My concern is mostly for backward compatibility. Nobody should use column orders though but you never know. For me the look doesn't worth it. But it's your call.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a function derived from RoutineCharacteristics.language. Actually is it better to expose language rather than implementation type?

Copy link
Contributor Author

@caithagoras caithagoras Jan 23, 2020

Choose a reason for hiding this comment

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

Language is an attribute for SqlInvokedFunction only. If we expose this information as an agglomerated enum such as in (BUILTIN, SQL, PYTHON, JS...), I would avoid reuse the name language, maybe function type?

Also, as you mentioned, we may also want to ship SqlInvokedFunctions that are static and packaged within Presto, which type value do we give them? They are both BUILTIN and SQL, so use a new enum BUILTIN_SQL, or PREPACKAGED_SQL?

An alternative I would prefer is to introduce 2 columns instead.

  • boolean built_in:
    • true: hard-coded functions, and pre-packaged SqlInvokedFunctions
    • false: user-defined functions.
  • varchar language:
    • null: hard-coded functions
    • non-null values (SQL, Python, etc...): all SqlInvokedFunctions, including pre-packaged and user-defined.

I'm essentially defining "built_in" as anything that comes in with the presto installation, which I think does make sense.

@caithagoras caithagoras force-pushed the s3 branch 3 times, most recently from 4bc99f6 to 60cfbef Compare January 24, 2020 01:27
@caithagoras
Copy link
Contributor Author

@rongrong Separated into 2 columns: boolean builtin, varchar language

@caithagoras caithagoras force-pushed the s3 branch 8 times, most recently from c5c4fdd to dd684b5 Compare January 24, 2020 21:52
Also, added additional assertions.
@caithagoras caithagoras force-pushed the s3 branch 2 times, most recently from 0ffe51d to 8c0409b Compare January 24, 2020 23:44
@rongrong rongrong merged commit 8a1bb1b into prestodb:master Jan 27, 2020
@caithagoras caithagoras deleted the s3 branch January 27, 2020 22:46
@caithagoras caithagoras mentioned this pull request Feb 20, 2020
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants