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

For ExprFunction, replace Environment with EvaluationSession #559

Merged
merged 3 commits into from
Mar 28, 2022

Conversation

dlurton
Copy link
Member

@dlurton dlurton commented Mar 28, 2022

I am working on a significant evaluator refactor to work with the new relational algebra. There is a requirement that this be implemented as a refactored copy of the original EvaluatingCompiler to allow customers to switch between the old and new evaluators via runtime configuration. One challenge associated with this is: the new evaluator does not use the same Environment class of the legacy evaluator and (until this PR) and the ExprFunction.call* functions all accepted Environment as a first argument. This PR changes that argument to session: EvaluationSession instead. Unfortunately, this is a breaking API change and customers who have their own ExprFunction implementations. (Although this has gone through a breaking change in the last release anyway and most of our customers probably haven't migrated yet.) There is, however, no other way that I can see to accomplish the goal of exposing the new physical plan evaluator as an alternative to the legacy AST evaluator.

Aside from the above:

  • Exposing Environment as public API wasn't a great idea because it exposed implementation details that the customer should never depend on (i.e. all of the properties here except for session).
  • No customer that I am aware of uses the env: Environment parameter. Nor is there any reason to that I am aware of, outside of accessing its session: EvaluationSession property.

This PR also makes Environment and several other types internal as well. These other types were all implementation details that customers should not be depending on and could not be made internal previously due to the requirement that Environment be public.

Copy link
Contributor

@lziq lziq left a comment

Choose a reason for hiding this comment

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

So I think this PR mainly changes Environment from public to internal. I think we should do so, even if this could break users' code, as users should not access it by design.

@@ -25,7 +25,7 @@ import java.util.TreeMap
* @param session The evaluation session.
* @param groups The map of [Group]s that is currently being built during query execution.
*/
data class Environment(
internal data class Environment(
Copy link
Contributor

@lziq lziq Mar 28, 2022

Choose a reason for hiding this comment

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

Nit: Currently the description in kdoc is "the environment for execution". Do you think is makes more sense to change it to "variables environment used during evaluation"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this version of the class is not just for variables--it includes groups too so the existing wording is more appropriate, IMHO. Anyway, perhaps that is outside the scope of this CR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then can we change it as "environment used during evaluation"? I think "execution" in the original definition is not really clear.

Comment on lines 3 to 4
#kotlin.daemon.jvmargs="-Xmx8g"
org.gradle.jvmargs="-Xmx8g"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to add these 2 lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot to remove this--it might be needed a bit later.

@dlurton dlurton requested a review from lziq March 28, 2022 20:15
@dlurton dlurton merged commit 18c06a8 into main Mar 28, 2022
@dlurton dlurton deleted the expr-function-evaluation-session branch March 28, 2022 22:41
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