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

[optimize] MRE::typ(): can we avoid recalculating types? #30871

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mgree
Copy link
Contributor

@mgree mgree commented Dec 19, 2024

Part of MirRelationExpr::typ() cleanup.

Is it possible to avoid recalculating types?

Motivation

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.

@mgree mgree changed the title [optimize] MRE::typ(): can we avoid recalculating types? [dnm] [optimize] MRE::typ(): can we avoid recalculating types? Dec 19, 2024
@mgree
Copy link
Contributor Author

mgree commented Dec 19, 2024

Short answer: no. Recalculating the types gives us more precise keys.

assertion `left == right` failed
  left: RelationType { column_types: [ColumnType { scalar_type: Int32, nullable: false }], keys: [] }
 right: RelationType { column_types: [ColumnType { scalar_type: Int32, nullable: false }], keys: [[]] }

Trying one more run, this time just using the found_typ... what breaks?

@mgree mgree force-pushed the mre-typ-ballast-check-recalc branch from fce5acb to 0779168 Compare December 19, 2024 18:12
@mgree mgree changed the title [dnm] [optimize] MRE::typ(): can we avoid recalculating types? [optimize] MRE::typ(): can we avoid recalculating types? Dec 19, 2024
@mgree mgree marked this pull request as ready for review December 19, 2024 20:30
@mgree mgree requested a review from a team as a code owner December 19, 2024 20:30
@mgree mgree requested a review from jkosh44 December 19, 2024 20:30
src/adapter/src/coord/peek.rs Outdated Show resolved Hide resolved
"keys": [
[]
]
"keys": []
Copy link
Contributor

Choose a reason for hiding this comment

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

It's surprising that the key was wrong in the MirRelationExpr::Constant from which as_const got the type. We might want to investigate this (maybe independently from this PR).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, could you rebase on top of #30881?
I have a theory on what might have happened: take_safely used to not set the best possible key, because it ran the key derivation on the original thing, where the key derivation often has no chance to realize that it's an empty relation (because the caller of take_safely might have determined this in complex ways). If this doesn't change after the rebase, then there are other places where we don't accurately set the keys on constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In #30894.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebase is green... which means that we're still getting the less precise types on constants, right? 🤔

@jkosh44 jkosh44 removed request for a team and jkosh44 December 23, 2024 16:20
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.

3 participants