Skip to content

Remove Metadata from RowExpressionInterpreter#22868

Merged
tdcmeehan merged 1 commit intoprestodb:masterfrom
tdcmeehan:rei_refactor
May 31, 2024
Merged

Remove Metadata from RowExpressionInterpreter#22868
tdcmeehan merged 1 commit intoprestodb:masterfrom
tdcmeehan:rei_refactor

Conversation

@tdcmeehan
Copy link
Contributor

Description

The only reference to the Metadata in RowExpressionInterpreter is to retrieve the FunctionAndTypeManager. However, the FunctionAndTypeManager actually doesn't even belong on the Metadata class and can be injected where needed. This refactoring reduces the call sites to Metadata#getFunctionAndTypeManager, and makes subsequent refactoring around RowExpressionInterpreter smaller and easier to understand.

Motivation and Context

Code cleanup and simplifying commits.

Impact

No external changes.

Test Plan

Existing tests are sufficient.

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* ... :pr:`12345`
* ... :pr:`12345`

Hive Connector Changes
* ... :pr:`12345`
* ... :pr:`12345`

If release note is NOT required, use:

== NO RELEASE NOTE ==

@tdcmeehan tdcmeehan requested a review from a team as a code owner May 30, 2024 14:17
@tdcmeehan
Copy link
Contributor Author

Our codeowner rules should have added @jaystarshot and @feilong-liu as reviewers for this PR. I will follow up why it did not.

Copy link
Member

@jaystarshot jaystarshot left a comment

Choose a reason for hiding this comment

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

LGTM, though I feel that metadata is always good to have

@jaystarshot
Copy link
Member

maybe because of https://github.com/prestodb/presto/blob/master/CODEOWNERS#L110

@tdcmeehan
Copy link
Contributor Author

Thanks @jaystarshot, I think you're right. Can you look at #22875?

The only reference to the Metadata in RowExpressionInterpreter is to
retrieve the FunctionAndTypeManager.  However, the FunctionAndTypeManager
actually doesn't even belong on the Metadata class and can be injected
where needed.  This refactoring reduces the call sites to
Metadata#getFunctionAndTypeManager.
@tdcmeehan tdcmeehan closed this May 31, 2024
@tdcmeehan tdcmeehan reopened this May 31, 2024
@tdcmeehan tdcmeehan requested a review from jaystarshot May 31, 2024 13:44
@tdcmeehan tdcmeehan merged commit 3cd2744 into prestodb:master May 31, 2024
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.

2 participants

Comments