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

fix(datastore): Switch to BFS in Join Builder for Alias, fixes - #2488 #2693

Merged
merged 18 commits into from
Feb 19, 2024

Conversation

ankpshah
Copy link
Contributor

@ankpshah ankpshah commented Jan 29, 2024

  • PR title and description conform to Pull Request guidelines.

Issue #, if available:
#2488
Description of changes:
Replace DFS Join Builder with BFS to Improve Alias Management in Joins

  • Replace the recursiveBuildJoins method (DFS-based) with buildJoinsUsingBFS to address issues with incorrect table alias usage in WHERE conditions.
  • The BFS approach ensures that all tables directly related to the root table are assigned their original names without any numeric suffixes (alias), enhancing the readability and correctness of SQL queries, especially in WHERE clauses involving intermediate tables in multilevel joins.
  • Implement a tracking mechanism for visited (table, FK) combinations to support multiple foreign keys and prevent infinite loops in cyclic relationships.
  • This change aims to improve the reliability of SQL query construction, particularly by ensuring that WHERE conditions on intermediate tables in multilevel joins are more intuitive and less error-prone.

How did you test these changes?
(Please add a line here how the changes were tested)

Documentation update required?

  • No
  • Yes (Please include a PR link for the documentation update)

General Checklist

  • Added Unit Tests
  • Added Integration Tests
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

- Replace the recursiveBuildJoins method (DFS-based) with buildJoinsUsingBFS to address issues with incorrect table alias usage in WHERE conditions.
- The BFS approach ensures that all tables directly related to the root table are assigned their original names without any numeric suffixes, enhancing the readability and correctness of SQL queries, especially in WHERE clauses involving intermediate tables in multilevel joins.
- Implement a tracking mechanism for visited (table, FK) combinations to support multiple foreign keys and prevent infinite loops in cyclic relationships.
- Add detailed JavaDoc comments to explain the new BFS-based join building logic, its advantages in alias management, and its approach to handling complex table relationships.
- This change aims to improve the reliability of SQL query construction, particularly by ensuring that WHERE conditions on intermediate tables in multilevel joins are more intuitive and less error-prone.
@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (1804551) 42.85% compared to head (3ff367e) 42.92%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2693      +/-   ##
==========================================
+ Coverage   42.85%   42.92%   +0.06%     
==========================================
  Files         905      905              
  Lines       29098    29114      +16     
  Branches     4142     4144       +2     
==========================================
+ Hits        12471    12497      +26     
+ Misses      15269    15260       -9     
+ Partials     1358     1357       -1     

@ankpshah ankpshah marked this pull request as ready for review January 29, 2024 16:48
@ankpshah ankpshah requested a review from a team as a code owner January 29, 2024 16:48
@tylerjroach
Copy link
Member

Changes Look great. Just as a reminder, we discussed addition of query using CPK, as well as multiple statements in where clause.

@ankpshah
Copy link
Contributor Author

Thanks. I have added tests for querying using CPK, as well as multiple conditions in where statement.

@mattcreaser mattcreaser self-requested a review February 14, 2024 19:38
Copy link
Member

@mattcreaser mattcreaser left a comment

Choose a reason for hiding this comment

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

Just a few minor comments to add.

Copy link
Member

@mattcreaser mattcreaser left a comment

Choose a reason for hiding this comment

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

🎉

@ankpshah ankpshah merged commit 3bf1bbc into main Feb 19, 2024
3 checks passed
@ankpshah ankpshah deleted the bug/2488_query_alias_fix branch February 19, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants