Reuse prod code and reduce EsqlSession public surface #132728
Conversation
idegtiarenko
added
>refactoring
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| new EsqlQueryRequest(), | ||
| new EsqlExecutionInfo(randomBoolean()), | ||
| planRunner(bigArrays, foldCtx, physicalOperationProviders), | ||
| session.optimizedPlan(preOptimized), |
There was a problem hiding this comment.
This is same as org.elasticsearch.xpack.esql.session.EsqlSession#optimizeAndExecute except it did not call preMapper.preMapper.
Lets test production code instead of its test copy.
There was a problem hiding this comment.
The preMapper part wasn't an issue with the CsvTests so far, since full text function tests aren't run part of this suite. (Still, great to have this part fixed!)
| } | ||
|
|
||
| // visible for testing in CsvTests | ||
| public void optimizeAndExecute( |
There was a problem hiding this comment.
I wish this could be protected or default visibility, however CsvTests are in different package.
I wonder if in future it would be possible to extend EsqlSession in CsvTests, override analyzedPlan with custom test resolution and reduce visibility of this method.
There was a problem hiding this comment.
The tests can make a TestEsqlSession inside of the same package. Right?
| private LogicalPlan optimizedPlan(LogicalPlan logicalPlan) { | ||
| if (logicalPlan.preOptimized() == false) { | ||
| throw new IllegalStateException("Expected pre-optimized plan"); | ||
| } |
There was a problem hiding this comment.
(here and in physicalPlan(..))
This is now private and we control the order of invocations in EsqlSession.
Should we convert those exceptions into assertions?
There was a problem hiding this comment.
I'm fine keeping these as hard if statements unless we see a problem on the benchmarks. It'd be bad to mess up the ordering. Not likely, but whatever.
| } | ||
|
|
||
| // visible for testing in CsvTests | ||
| public void optimizeAndExecute( |
There was a problem hiding this comment.
The tests can make a TestEsqlSession inside of the same package. Right?
| private LogicalPlan optimizedPlan(LogicalPlan logicalPlan) { | ||
| if (logicalPlan.preOptimized() == false) { | ||
| throw new IllegalStateException("Expected pre-optimized plan"); | ||
| } |
There was a problem hiding this comment.
I'm fine keeping these as hard if statements unless we see a problem on the benchmarks. It'd be bad to mess up the ordering. Not likely, but whatever.
| listener.delegateFailureAndWrap( | ||
| // Wrap so we can capture the warnings in the calling thread |
There was a problem hiding this comment.
Why are we not using delegateFailureAndWrap anymore? (Naive question)
This reverts commit f8b2b3b.
5 participants
Today CsvTests in order to run tests re-implemented a flow present in EsqlSession (except missing
preMapperstep).This change replaces this copy with a call to
EsqlSession#optimizeAndExecuteas well as making several methods private that are no longer need broad visibility by CsvTests.