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

Catalog to Datasource changes #1086

Merged

Conversation

vamsi-amazon
Copy link
Member

@vamsi-amazon vamsi-amazon commented Nov 17, 2022

Signed-off-by: vamsi-amazon reddyvam@amazon.com

Description

This CR changes internal catalog code references to datasource references.

Issues Resolved

#1028

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.

@vamsi-amazon vamsi-amazon linked an issue Nov 17, 2022 that may be closed by this pull request
@vamsi-amazon vamsi-amazon force-pushed the catalog-to-datasource branch 3 times, most recently from c59e04c to 5a9b51f Compare November 17, 2022 21:27
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2022

Codecov Report

Merging #1086 (81c9285) into 2.x (81c9285) will not change coverage.
The diff coverage is n/a.

❗ Current head 81c9285 differs from pull request most recent head fefaa1e. Consider uploading reports for the commit fefaa1e to get more accurate results

@@            Coverage Diff            @@
##                2.x    #1086   +/-   ##
=========================================
  Coverage     98.27%   98.27%           
  Complexity     3434     3434           
=========================================
  Files           343      343           
  Lines          8540     8540           
  Branches        544      544           
=========================================
  Hits           8393     8393           
  Misses          142      142           
  Partials          5        5           
Flag Coverage Δ
sql-engine 98.27% <0.00%> (ø)

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

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@vamsi-amazon vamsi-amazon force-pushed the catalog-to-datasource branch 8 times, most recently from e295d9c to ab3ae58 Compare November 17, 2022 22:06
@vamsi-amazon vamsi-amazon marked this pull request as ready for review November 17, 2022 22:12
@vamsi-amazon vamsi-amazon requested a review from a team as a code owner November 17, 2022 22:12
@vamsi-amazon vamsi-amazon force-pushed the catalog-to-datasource branch 2 times, most recently from 393e449 to fefaa1e Compare November 17, 2022 22:27
@vamsi-amazon vamsi-amazon added maintenance Improves code quality, but not the product backport 2.4 labels Nov 17, 2022
LinkedHashMap<String, ExprValue> valueMap = new LinkedHashMap<>();
valueMap.put("TABLE_CATALOG", stringValue(catalogSchemaName.getCatalogName()));
valueMap.put("TABLE_SCHEMA", stringValue(catalogSchemaName.getSchemaName()));
valueMap.put("TABLE_CATALOG", stringValue(dataSourceSchemaName.getDataSourceName()));
Copy link
Member

Choose a reason for hiding this comment

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

is this table_catalog notion differs from the datasource ?

Copy link
Member Author

Choose a reason for hiding this comment

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

both are same.

TABLE_CATALOG is pre-existing response column when we query metadata about opensearch indices.
I am following the same convention right now in order to be consistent with opensearch metadata queries.
Need to re-think if we can remove from both the places.

Will do it in a separate PR because we need to think about backward compatibility of existing queries.

May be we can change TABLE_CATALOG to TABLE_DATASOURCE in prometheus.

Signed-off-by: vamsi-amazon <reddyvam@amazon.com>
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.

Thanks.

Copy link
Member

@joshuali925 joshuali925 left a comment

Choose a reason for hiding this comment

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

should logs also be updated?

Prometheus Catalog doesn't multiple aggregations in stats command

@vamsi-amazon vamsi-amazon merged commit fef20f8 into opensearch-project:2.x Nov 18, 2022
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.4 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.4 2.4
# Navigate to the new working tree
cd .worktrees/backport-2.4
# Create a new branch
git switch --create backport/backport-1086-to-2.4
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 fef20f86b3eeffe3f16697e52df7eea3bd31b762
# Push it to GitHub
git push --set-upstream origin backport/backport-1086-to-2.4
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.4

Then, create a pull request where the base branch is 2.4 and the compare/head branch is backport/backport-1086-to-2.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.4 maintenance Improves code quality, but not the product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SQL] change catalog references to datasource.
6 participants