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.

Sign up for GitHub

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

Jump to bottom

SQL: Implement CONVERT, an alternative to CAST #34660

Merged
merged 9 commits into from
Oct 23, 2018
Merged

SQL: Implement CONVERT, an alternative to CAST #34660

merged 9 commits into from
Oct 23, 2018

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Oct 19, 2018

CONVERT works exactly like cast with slightly different syntax:
CONVERT(<value>, <data_type) as opposed to CAST(<value> AS <data_type>)

Moreover it support format of the MS-SQL data types SQL_<type>,
e.g.: SQL_INTEGER

Closes: #34513

@matriv
SQL: Implement CONVERT, an alternative to CAST
f93811d 
`CONVERT` works exactly like cast with slightly different syntax:
`CONVERT(<value>, <data_type)` as opposed to `CAST(<value> AS <data_type>)`

Moreover it support format of the MS-SQL data types `SQL_<type>`,
e.g.: `SQL_INTEGER`

Closes: elastic#34513
@matriv matriv requested review from costin and astefan October 19, 2018 22:21
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@matriv
Copy link
Contributor Author

matriv commented Oct 19, 2018

@costin @astefan Instead of null here: https://github.com/elastic/elasticsearch/pull/34660/files#diff-eff884b6c6b53d7e75ebac8b6a270830R92 we can also put UNSUPPORTED.

In this case, the user won't get a ParsingException but a TypeResolution coming from Cast when tries to do the conversion. Do you think this is preferable?

@astefan
Copy link
Contributor

astefan commented Oct 21, 2018

Definitely UNSUPPORTED from usability point of view. It is not an invalid data type, but rather a valid one we do not support yet.

costin
costin approved these changes Oct 21, 2018
View reviewed changes
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.
+1 on using UNSUPPORTED instead of null.

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java Outdated
@@ -44,12 +45,63 @@
DATE( JDBCType.TIMESTAMP, Timestamp.class, Long.BYTES, 24, 24);
// @formatter:on

public static final String MSSQL_DATATYPE_PREFIX = "SQL_";
Copy link
Member

Choose a reason for hiding this comment

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

It's not so much MSSQL as ODBC spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, replaced all MSSQL with ODBC.

astefan
astefan approved these changes Oct 21, 2018
View reviewed changes
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. Left just one minor comment related to docs.

x-pack/qa/sql/src/main/resources/docs.csv-spec Outdated
;


conversionStringToIntConvertESDataType
Copy link
Contributor

Choose a reason for hiding this comment

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

I would put this example first and the odbc one second, in order of familiarity/usage?

@matriv
Copy link
Contributor Author

matriv commented Oct 22, 2018

retest this please

1 similar comment
@matriv
Copy link
Contributor Author

matriv commented Oct 22, 2018

retest this please

@costin
Copy link
Member

costin commented Oct 22, 2018

Try merging master again - hopefully it fixes the external failure.

@matriv matriv merged commit e9e1407 into elastic:master Oct 23, 2018
matriv pushed a commit that referenced this pull request Oct 23, 2018
@matriv
SQL: Implement CONVERT, an alternative to CAST (#34660)
3a3c95c 
`CONVERT` works exactly like cast with slightly different syntax:
`CONVERT(<value>, <data_type)` as opposed to `CAST(<value> AS <data_type>)`

Moreover it support format of the MS-SQL data types `SQL_<type>`,
e.g.: `SQL_INTEGER`

Closes: #34513
@matriv matriv deleted the mt/impl-34513 branch October 23, 2018 09:27
@matriv
Copy link
Contributor Author

matriv commented Oct 23, 2018

Backported to 6.x with 3a3c95c

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Oct 23, 2018
@jasontedor
Merge branch 'master' into ccr-docs
f7bbdd1 
* master: (24 commits)
  ingest: better support for conditionals with simulate?verbose (elastic#34155)
  [Rollup] Job deletion should be invoked on the allocated task (elastic#34574)
  [DOCS] .Security index is never auto created (elastic#34589)
  CCR: Requires soft-deletes on the follower (elastic#34725)
  re-enable bwc tests (elastic#34743)
  Empty GetAliases authorization fix (elastic#34444)
  INGEST: Document Processor Conditional (elastic#33388)
  [CCR] Add total fetch time leader stat (elastic#34577)
  SQL: Support pattern against compatible indices (elastic#34718)
  [CCR] Auto follow pattern APIs adjustments (elastic#34518)
  [Test] Remove dead code from ExceptionSerializationTests (elastic#34713)
  A small typo in migration-assistance doc (elastic#34704)
  ingest: processor stats (elastic#34724)
  SQL: Implement IN(value1, value2, ...) expression. (elastic#34581)
  Tests: Add checks to GeoDistanceQueryBuilderTests (elastic#34273)
  INGEST: Rename Pipeline Processor Param. (elastic#34733)
  Core: Move IndexNameExpressionResolver to java time (elastic#34507)
  [DOCS] Force Merge: clarify execution and storage requirements (elastic#33882)
  TESTING.asciidoc fix examples using forbidden annotation (elastic#34515)
  SQL: Implement `CONVERT`, an alternative to `CAST` (elastic#34660)
  ...
kcm pushed a commit that referenced this pull request Oct 30, 2018
@kcm
SQL: Implement CONVERT, an alternative to CAST (#34660)
9ebbdf0 
`CONVERT` works exactly like cast with slightly different syntax:
`CONVERT(<value>, <data_type)` as opposed to `CAST(<value> AS <data_type>)`

Moreover it support format of the MS-SQL data types `SQL_<type>`,
e.g.: `SQL_INTEGER`

Closes: #34513
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astefan astefan astefan approved these changes

@costin costin costin approved these changes

Assignees
No one assigned
Labels
:Analytics/SQL SQL querying >enhancement >feature v6.5.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@matriv @elasticmachine @astefan @costin @colings86