-
Notifications
You must be signed in to change notification settings - Fork 129
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
Fix SQLQueryUtils to extract multiple tables #2784
Fix SQLQueryUtils to extract multiple tables #2784
Conversation
d51f211
to
5b9687e
Compare
Signed-off-by: Tomoyuki Morita <moritato@amazon.com>
5b9687e
to
6cba94b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2784 +/- ##
============================================
- Coverage 92.72% 92.72% -0.01%
Complexity 4987 4987
============================================
Files 498 498
Lines 14350 14349 -1
Branches 942 942
============================================
- Hits 13306 13305 -1
Misses 1010 1010
Partials 34 34
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Tomoyuki Morita <moritato@amazon.com>
@@ -32,16 +35,15 @@ | |||
@UtilityClass | |||
public class SQLQueryUtils { | |||
|
|||
// TODO Handle cases where the query has multiple table Names. | |||
public static FullyQualifiedTableName extractFullyQualifiedTableName(String sqlQuery) { | |||
public static List<FullyQualifiedTableName> extractFullyQualifiedTableNames(String sqlQuery) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we show an example SQL query where there is a bug? What is the usecase for multiple table names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bug was in case the identifier has some more elements such as alias, version, etc.
I added test cases for that (ref).
Simplest example is:
SELECT * FROM my_glue.default.http_logs alias;
Where table name was extracted as "http_logsalias".
DQS wants to extract datasource names from the input query and verify them, and that would require all the table names to be extracted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are we solving the use case with multiple table names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this change, we collect table names to a list in the visitor and return all.
I've added test cases for multi table queries.
* Fix SQLQueryUtils to extract multiple tables Signed-off-by: Tomoyuki Morita <moritato@amazon.com> * Improve test coverage Signed-off-by: Tomoyuki Morita <moritato@amazon.com> --------- Signed-off-by: Tomoyuki Morita <moritato@amazon.com> (cherry picked from commit 883cc7e) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* Fix SQLQueryUtils to extract multiple tables * Improve test coverage --------- (cherry picked from commit 883cc7e) Signed-off-by: Tomoyuki Morita <moritato@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
Issues Resolved
n/a
Check List
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.