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

Describe Table with catalog name. #989

Merged
merged 1 commit into from
Nov 1, 2022
Merged

Describe Table with catalog name. #989

merged 1 commit into from
Nov 1, 2022

Conversation

vmmusings
Copy link
Member

@vmmusings vmmusings commented Oct 28, 2022 •
edited
Loading

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.

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

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

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.51%. Comparing base (e3fe086) to head (7ec3af0).
Report is 477 commits behind head on 2.x.

Additional details and impacted files 
@@             Coverage Diff              @@
##                2.x     #989      +/-   ##
============================================
- Coverage     97.60%   95.51%   -2.10%     
- Complexity     3185     3224      +39     
============================================
  Files           308      320      +12     
  Lines          7983     8720     +737     
  Branches        520      644     +124     
============================================
+ Hits           7792     8329     +537     
- Misses          190      334     +144     
- Partials          1       57      +56
Flag Coverage Δ
query-workbench 62.76% <ø> (?)
sql-engine 98.18% <100.00%> (+0.58%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vmmusings vmmusings force-pushed the des-table-prom branch 6 times, most recently from 255b300 to 4f34e0d Compare October 29, 2022 07:51
@vmmusings vmmusings marked this pull request as ready for review October 29, 2022 19:51
@vmmusings vmmusings 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
docs/user/ppl/cmd/describe.rst Outdated Show resolved Hide resolved
prometheus/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java Show resolved Hide resolved
ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java
Comment on lines +127 to +133
ArrayList<String> parts = new ArrayList<>(tableQualifiedName.getParts());
parts.set(parts.size() - 1, mappingTable(parts.get(parts.size() - 1)));
Copy link
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?

Copy link
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.

Copy link
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?

Copy link
Member Author

@vmmusings vmmusings Oct 31, 2022 •
edited
Loading

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.

Copy link
Member Author

@vmmusings vmmusings Oct 31, 2022 •
edited
Loading

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

@vmmusings vmmusings 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
docs/user/general/identifiers.rst Outdated Show resolved Hide resolved
penghuo
penghuo approved these changes Nov 1, 2022
View reviewed changes
Copy link
Collaborator

@penghuo penghuo left a comment

Choose a reason for hiding this comment

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

LGTM

@vmmusings vmmusings 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
@vmmusings @github-actions
Describe Table with catalog name. (#989)
7aaf5da 
Signed-off-by: Vamsi Manohar <[email protected]>

Signed-off-by: Vamsi Manohar <[email protected]>
(cherry picked from commit 40d8d9f)
vmmusings 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 <[email protected]>
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

@joshuali925 joshuali925 joshuali925 approved these changes

@penghuo penghuo penghuo approved these changes

@YANG-DB YANG-DB YANG-DB left review comments

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
@vmmusings @codecov-commenter @penghuo @joshuali925 @dai-chen @YANG-DB