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: [Docs] Add an ES-SQL column for data types #37529

Merged
merged 2 commits into from
Jan 16, 2019

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Jan 16, 2019

In order to distinguish the ES-SQL type from the standard SQL type
add a new ES-SQL column that will make clear this distingstion,
e.g.: datetime vs TIMSTAMP

Fixes: #37519

In order to distinguish the ES-SQL type from the standard SQL type
add a new ES-SQL column that will make clear this distingstion,
e.g.: datetime vs TIMSTAMP

Fixes: elastic#37519
@matriv matriv added >docs General docs changes v7.0.0 :Analytics/SQL SQL querying v6.7.0 labels Jan 16, 2019
@matriv matriv requested review from costin and astefan January 16, 2019 12:12
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@matriv
Copy link
Contributor Author

matriv commented Jan 16, 2019

Should we backport to earlier versions?

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 one minor comment and one open suggestion.


|===
s|{es} type
s|{es}-SQL type
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not mistaken, I think we have a variable defined for Elasticsearch SQL: {es-sql} and there shouldn't be a need for {es}-SQL.

| <<keyword, `keyword`>> | keyword | VARCHAR | based on <<ignore-above>>
| <<text, `text`>> | text | VARCHAR | 2,147,483,647
| <<binary, `binary`>> | binary | VARBINARY | 2,147,483,647
| <<date, `date`>> | datetime | TIMESTAMP | 24
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the generated HTML, it looks fine, but I'm wondering if we shouldn't make it more clear (in a short sentence before the table) what is the actual difference, because the user will see two columns that look almost the same and only towards the end one notices (if it does notice) date in ES has a datetime correspondent in ES-SQL.

What I'm suggesting is to change the phrase before the table from Most of Elasticsearch data types are available in Elasticsearch SQL, as indicated below:

to something like

Most of Elasticsearch data types are available in Elasticsearch SQL, as indicated below. As one can see, all of Elasticsearch data types are mapped to the data type with the same name in Elasticsearch SQL, with the exception of datedata type which is calleddatetime in Elasticsearch SQL:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the clarification, as I think it's better to have it standing out there apart from the table where the eye can skip it as everything else is the same.

Copy link
Member

Choose a reason for hiding this comment

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

Potentially add an explanation as why there's datetime : in order to not confuse it with the ANSI SQL date (date only) type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, planning to do it with the introduction of date.

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 8932750 into elastic:master Jan 16, 2019
@matriv matriv deleted the mt/fix-37519 branch January 16, 2019 14:16
matriv added a commit that referenced this pull request Jan 16, 2019
In order to distinguish the ES-SQL type from the standard SQL type
add a new ES-SQL column that will make clear this distingstion,
e.g.: datetime vs TIMSTAMP

Fixes: #37519
@matriv
Copy link
Contributor Author

matriv commented Jan 16, 2019

Backported to 6.x with 40af8a3

cshtdd added a commit to cshtdd/elasticsearch that referenced this pull request Jan 16, 2019
* master:
  Deprecate _type from LeafDocLookup (elastic#37491)
  Allow system privilege to execute proxied actions (elastic#37508)
  Update Put Watch to allow unknown fields (elastic#37494)
  AwaitsFix testAddNewReplicas
  SQL: Add protocol tests and remove jdbc_type from drivers response (elastic#37516)
  SQL: [Docs] Add an ES-SQL column for data types (elastic#37529)
  IndexMetaData#mappingOrDefault doesn't need to take a type argument. (elastic#37480)
  Simplify + Cleanup Dead Code in Settings (elastic#37341)
  Reject all requests that have an unconsumed body (elastic#37504)
  [Ml] Prevent config snapshot failure blocking migration (elastic#37493)
  Fix line length for aliases and remove suppression (elastic#37455)
  Add SSL Configuration Library (elastic#37287)
  SQL: Remove slightly used meta commands (elastic#37506)
  Simplify Snapshot Create Request Handling (elastic#37464)
  Remove the use of AbstracLifecycleComponent constructor elastic#37488 (elastic#37488)
  [ML] log minimum diskspace setting if forecast fails due to insufficient d… (elastic#37486)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/SQL SQL querying >docs General docs changes v6.7.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants