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

getSqlEscapedSource: support for databricksRest provider #913

Merged

Conversation

zbigg
Copy link
Contributor

@zbigg zbigg commented Sep 25, 2024

Description

Shortcut: https://app.shortcut.com/cartoteam/story/442095

getSqlEscapedSource & FullyQualifiedName to support databricksRest provider type - which is same as databricks from SQL pov

Type of change

(choose one and remove the others)

  • Fix

Acceptance

Already covered with UT

Basic checklist

  • Good PR name
  • Shortcut link
  • Changelog entry
  • Just one issue per PR
  • GitHub labels
  • Proper status & reviewers
  • Tests
  • Documentation

@zbigg zbigg requested review from padawannn and jmgaya September 25, 2024 08:23
@zbigg zbigg added bug Something isn't working builder labels Sep 25, 2024
@zbigg zbigg force-pushed the bug/sc-442095/support-pinea-prepared-tables-in-databricks branch from 776af9d to 130e6b7 Compare September 25, 2024 08:25
Copy link

github-actions bot commented Sep 25, 2024

Pull Request Test Coverage Report for Build 11029739616

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.009%) to 70.925%

Totals Coverage Status
Change from base Build 10994200388: 0.009%
Covered Lines: 2852
Relevant Lines: 3698

💛 - Coveralls

Copy link

github-actions bot commented Sep 25, 2024

Visit the preview URL for this PR (updated for commit 75ed6ca):

https://cartodb-fb-storybook-react-dev--pr913-bug-sc-442095-su-qa8hsz2m.web.app

(expires Wed, 02 Oct 2024 09:03:39 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 517cc4d31d7e09cf277774e034094b67c301cd4c

Copy link
Contributor

@jmgaya jmgaya left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@@ -26,13 +26,15 @@ const fqnParseRegex = {
`^${databricksIdentifierRegex}(?:\\.${databricksIdentifierRegex})?(?:\\.${databricksIdentifierRegex})?$`
)
};
fqnParseRegex[Provider.DatabricksRest] = fqnParseRegex[Provider.Databricks];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: I'll add this for consistency inside of fqnParseRegex and will extract the Regex, rather than updating the object afterward, increasing (at least, IMHO) this part's readability, e.g.

const databricksRegex = `^${databricksIdentifierRegex}(?:\\.${databricksIdentifierRegex})?(?:\\.${databricksIdentifierRegex})?$`
const fqnParseRegex = {
  ...
  [Provider.Databricks]: databricksRegex,
  [Provider.DatabricksRest]: databricksRegex
}

Same applies to nameNeedsQuotesChecker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied more or less.

@zbigg zbigg merged commit cef6bba into master Sep 25, 2024
2 checks passed
@zbigg zbigg deleted the bug/sc-442095/support-pinea-prepared-tables-in-databricks branch September 25, 2024 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working builder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants