Skip to content

Improve type mapping docs for Elasticsearch connector#13876

Merged
martint merged 1 commit intotrinodb:masterfrom
tlblessing:tb/elasticsearch-type-mapping
Sep 13, 2022
Merged

Improve type mapping docs for Elasticsearch connector#13876
martint merged 1 commit intotrinodb:masterfrom
tlblessing:tb/elasticsearch-type-mapping

Conversation

@tlblessing
Copy link
Copy Markdown
Member

Description

Improve type mapping documentation for Elasticsearch connector.

Is this change a fix, improvement, new feature, refactoring, or other?

Improvement to Elasticsearch connector documentation

How would you describe this change to a non-technical end user or system administrator?

Added data type mapping for write direction (Trino --> Elasticsearch); reformatted table for better wrapping; added boilerplate headings and verbiage for consistency.

Question for SMEs: I wasn't sure about the new write table to avoid 1:m mapping in the write direction. I assume that text might be preferred over keyword based on https://stackoverflow.com/questions/52845088/difference-between-keyword-and-text-in-elasticsearch. Please advise.

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

@phd3 phd3 changed the title Improve type mapping for Elasticsearch connector Improve type mapping docs for Elasticsearch connector Aug 28, 2022
@tlblessing tlblessing requested review from ebyhr and removed request for bitsondatadev August 29, 2022 15:33
@tlblessing tlblessing force-pushed the tb/elasticsearch-type-mapping branch from 351f5df to 70c0dd4 Compare September 12, 2022 17:53
@tlblessing tlblessing requested a review from martint September 12, 2022 18:17
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does "modifies some types" mean in this context?

Copy link
Copy Markdown
Contributor

@jhlodin jhlodin Sep 12, 2022

Choose a reason for hiding this comment

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

In the context of a read-only connector like this, just that the Elasticsearch data types are not necessarily the types used in Trino when reading, so some modification occurs in the engine/connector.

That ref link goes here, to this general type mapping explanation that just got merged earlier today - https://github.com/trinodb/trino/blob/master/docs/src/main/sphinx/language/types.rst#trino-type-support-and-mapping

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's misleading. The types are not "modified", in the engine or elsewhere. The types from ES get mapped into Trino types, that's all.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@martint and @jhlodin can we change this to "maps some types"?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that works for me. Just make sure whatever wording gets merged here then gets applies to all other connectors.

@tlblessing tlblessing force-pushed the tb/elasticsearch-type-mapping branch 2 times, most recently from 949ca8f to c750c4f Compare September 12, 2022 21:01
@tlblessing tlblessing force-pushed the tb/elasticsearch-type-mapping branch from c750c4f to 2780114 Compare September 13, 2022 19:30
@tlblessing tlblessing requested a review from martint September 13, 2022 19:30
@martint martint merged commit 66b9bad into trinodb:master Sep 13, 2022
@github-actions github-actions bot added this to the 396 milestone Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants