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.

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

SQL: Add CAST and CONVERT to SHOW FUNCTIONS #34940

Merged
merged 4 commits into from Oct 29, 2018
Merged

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Oct 26, 2018

Fixes: #34939

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@matriv
Copy link
Contributor Author

matriv commented Oct 26, 2018

retest this please

@matriv
Copy link
Contributor Author

matriv commented Oct 27, 2018

@bpintea I've chosen to show CAST and CONVERT with type: DATA_TYPE_CONVERSION instead of SCALAR as most DBs list CAST in a different section than scalar functions. What do you think? Is it ok for ODBC, or should I list them as SCALAR ?

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

I would stick with SCALAR since essentially that's what they are doing. DATA_TYPE_CONVERSION is a long description (with underscores) and I'm not sure what are the expected grouping names.

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM

@bpintea
Copy link
Contributor

bpintea commented Oct 29, 2018

Is it ok for ODBC, or should I list them as SCALAR ?

AFA ODBC is concerned, the way we list the function is orthogonal to it; i.e. there's no API which maps directly to this catalog call a.t.m.

AFAI can tell, ODBC lists it however as a scalar function, under the type SQL_CONVERT_FUNCTIONS, where the supported conversions (or "functions") are are to be inquired with types provided in the following paragraph.

A while ago we have decided not to obtain this information from ES/SQL dynamically, but to rather hard coded it in the driver. But even if we were to change that decision, in the listing we now have, the type column would not suffice to cover the API call where it could be relevant (SQLGetInfo()), since the supported conversions are not part of that listing.
This is why we have no direct mapping on this catalog call and why the way we do the listing is atm not relevant to the driver.

@matriv
Copy link
Contributor Author

matriv commented Oct 29, 2018

@bpintea Thanks a lot for looking into this! Changing the type to SCALAR.

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM

@matriv matriv merged commit 42b4f23 into elastic:master Oct 29, 2018
@matriv matriv deleted the mt/fix-34939 branch October 29, 2018 15:27
matriv pushed a commit that referenced this pull request Oct 29, 2018
@matriv
Copy link
Contributor Author

matriv commented Oct 29, 2018

Backported to 6.x with 71704fa

matriv pushed a commit that referenced this pull request Oct 29, 2018
@matriv
Copy link
Contributor Author

matriv commented Oct 29, 2018

Backported to 6.5 with 7e995e0

kcm pushed a commit that referenced this pull request Oct 30, 2018
@colings86 colings86 added v6.5.0 and removed v6.5.1 labels Nov 2, 2018
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.

None yet

6 participants