Describe Table with catalog name. by vamsimanohar · Pull Request #989 · opensearch-project/sql · GitHub
Skip to content

Describe Table with catalog name.#989

Merged
vamsimanohar merged 1 commit into
opensearch-project:2.xfrom
vamsimanohar:des-table-prom
Nov 1, 2022
Merged

Describe Table with catalog name.#989
vamsimanohar merged 1 commit into
opensearch-project:2.xfrom
vamsimanohar:des-table-prom

Conversation

@vamsimanohar

@vamsimanohar vamsimanohar commented Oct 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Description

  • Describe command for prometheus table.
  • Documentation and Integration tests.

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@vamsimanohar vamsimanohar added v2.4.0 'Issues and PRs related to version v2.4.0' enhancement New feature or request labels Oct 28, 2022
@codecov-commenter

codecov-commenter commented Oct 28, 2022 •
edited by codecov Bot
Loading

Copy link
Copy Markdown

@vamsimanohar vamsimanohar force-pushed the des-table-prom branch 6 times, most recently from 255b300 to 4f34e0d Compare October 29, 2022 07:51
@vamsimanohar vamsimanohar marked this pull request as ready for review October 29, 2022 19:51
@vamsimanohar vamsimanohar requested a review from a team as a code owner October 29, 2022 19:51
dai-chen
dai-chen reviewed Oct 31, 2022
View reviewed changes
Comment thread docs/user/ppl/cmd/describe.rst Outdated
Comment thread prometheus/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java
Comment thread ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java
Comment on lines +127 to +133

@dai-chen dai-chen Oct 31, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sorry what does this mean? No UT changes required to cover this?

@vamsimanohar vamsimanohar Oct 31, 2022

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.

Will make comments and also include UTs.

@penghuo penghuo Oct 31, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should it be handled by catalog resolver?

@vamsimanohar vamsimanohar Oct 31, 2022 •
edited
Loading

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.

yes, CatalogSchemaIdentifierNameResolver will handle this.
For

describe prometheus.requests_total command

the table name will get converted to below in the above code.

prometheus.requests_total.MAPPING_ODFE_SYS_TABLE

So that all the system tables are recognized the way it works currently.

@vamsimanohar vamsimanohar Oct 31, 2022 •
edited
Loading

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.

Future Design
describe prometheus.requests_total --> source = prometheus.information_schema.columns.

For information_schema, there will be a separate information_schema internal connector which can interact with all the storage engines to get column and table details.

Storage Engine will have more methods in interface to expose tables, columns, schema

@vamsimanohar vamsimanohar force-pushed the des-table-prom branch 13 times, most recently from bc92c1d to e752f96 Compare October 31, 2022 23:52
YANG-DB
YANG-DB previously approved these changes Nov 1, 2022
View reviewed changes
Comment thread docs/user/general/identifiers.rst Outdated
@vamsimanohar
Describe Table with catalog name.
7ec3af0 
Signed-off-by: Vamsi Manohar <reddyvam@amazon.com>
penghuo
penghuo approved these changes Nov 1, 2022
View reviewed changes

@penghuo penghuo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@vamsimanohar vamsimanohar merged commit 40d8d9f into opensearch-project:2.x Nov 1, 2022
@opensearch-trigger-bot opensearch-trigger-bot Bot mentioned this pull request Nov 1, 2022
Merged
opensearch-trigger-bot Bot pushed a commit that referenced this pull request Nov 1, 2022
@vamsimanohar @github-actions
Describe Table with catalog name. (#989)
7aaf5da 
Signed-off-by: Vamsi Manohar <reddyvam@amazon.com>

Signed-off-by: Vamsi Manohar <reddyvam@amazon.com>
(cherry picked from commit 40d8d9f)
vamsimanohar pushed a commit that referenced this pull request Nov 1, 2022
@opensearch-trigger-bot
Describe Table with catalog name. (#989) (#1004)
344aada 
Signed-off-by: vamsi-amazon <reddyvam@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dai-chen dai-chen dai-chen left review comments
@penghuo penghuo penghuo approved these changes
@joshuali925 joshuali925 joshuali925 approved these changes
+1 more reviewer
@YANG-DB YANG-DB YANG-DB left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport 2.4 enhancement New feature or request v2.4.0 'Issues and PRs related to version v2.4.0'

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vamsimanohar @codecov-commenter @penghuo @joshuali925 @dai-chen @YANG-DB