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

Sometimes we can give the type to MRE::take_safely #30881

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ggevay
Copy link
Contributor

@ggevay ggevay commented Dec 20, 2024

More typ() call elimination, similar to e.g. #30878

This one goes after various take_safely calls, where we know the type already, so no need to call .typ().

Also, this makes take_safely() give a better key and nullability to the constant, making use of the fact that we have an empty collection, so we have the keys [[]] and nothing is nullable.

Motivation

  • This PR fixes a previously unreported bug: take_safely was often not giving the best possible key to the constant. Ideally, the keys that are recorded on a MRE::Constant should be the same as if we were to call keys_with_input_keys on it, so that we don't have anomalies such as create_fast_path_plan needing to // For best accuracy, we need to recalculate typ.. This PR moves one step closer to this.

  • This PR refactors existing code.

Tips for reviewer

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@ggevay ggevay added A-optimization Area: query optimization and transformation A-CLUSTER Topics related to the CLUSTER layer labels Dec 20, 2024
@ggevay ggevay requested a review from mgree December 20, 2024 11:49
@ggevay ggevay marked this pull request as ready for review December 20, 2024 11:50
@ggevay ggevay requested a review from a team as a code owner December 20, 2024 11:50
@ggevay
Copy link
Contributor Author

ggevay commented Dec 20, 2024

Tagging @frankmcsherry because of touching EQProp.

@ggevay
Copy link
Contributor Author

ggevay commented Dec 20, 2024

Also, maybe this eliminates that key derivation which @antiguru found to be a bottleneck in FoldConstants? Which one was that?

@ggevay
Copy link
Contributor Author

ggevay commented Dec 20, 2024

Bringing back to draft, because there are test failures.

@ggevay
Copy link
Contributor Author

ggevay commented Dec 20, 2024

Fixed

@ggevay ggevay marked this pull request as ready for review December 20, 2024 14:39
Copy link
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

Seems fine to me!

One possible change, if we're going golfing: you could have take_safely_with_rel_type take an Option<RelationType>, saving us the double call to MRE::typ() when calling MRE::take_safely(). Only matters in debug mode, so I'm not sure it's worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLUSTER Topics related to the CLUSTER layer A-optimization Area: query optimization and transformation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants