-
Notifications
You must be signed in to change notification settings - Fork 65
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
Better transitive path planning #1824
base: master
Are you sure you want to change the base?
Better transitive path planning #1824
Conversation
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.
A first round of the reviews,
I have not yet looked at the E2E test failures.
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.
Found one more issue.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1824 +/- ##
==========================================
+ Coverage 90.20% 90.36% +0.15%
==========================================
Files 400 400
Lines 38514 38624 +110
Branches 4319 4338 +19
==========================================
+ Hits 34743 34903 +160
+ Misses 2477 2418 -59
- Partials 1294 1303 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ive-path-planning
…ive-path-planning
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.
Some questions and suggestions.
Conformance check passed ✅No test result changes. |
|
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.
Only a tiny suggestion left.
AD_LOG_WARN << "Tried to re-sort a subtree that will already be sorted " | ||
"with `Sort` with a different sort order. This is a bug." | ||
<< std::endl; |
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.
This is not a bug per se but
The reason is possibly a missed optimization in the query planner
.
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.
So please rephrase this, so we don't get that many complaints of innocent users.
Related to #1809 and #1780
This changes the query planning so that joins are distributively pushed into unions if applicable if the cost is lower this way.