Extract compileRequest from BaseSingleStageRequestHandler.doHandleRequest #15073
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #15073 +/- ##
============================================
+ Coverage 61.75% 63.43% +1.68%
- Complexity 207 1483 +1276
============================================
Files 2436 2748 +312
Lines 133233 154471 +21238
Branches 20636 23817 +3181
============================================
+ Hits 82274 97989 +15715
- Misses 44911 49086 +4175
- Partials 6048 7396 +1348
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
The ultimate goal is to abstract out the routing logic. Also the validation logic has to be extracted because logical table validation will be different. This PR will make it easier for the subsequent abstractions because a few small number of objects can be passed around. Right now there is too much dependence on local variables which makes it harder to reason as well as look at a diff and be confident the changes are correct. |
|
Another reason to have these boundaries is that what should be in compile or validation phase is done much later. These boundaries will force some discipline. Couple of examples:
Maybe the sequence is the correct. However the reasoning is not clear to a newbie. If there are boundaries, then the reason for an op not in the right phase will (hopefully) be documented |
| if (compileResult != null) { | ||
| rawTableName = compileResult._rawTableName; | ||
| } |
There was a problem hiding this comment.
compileResult will always be null, and this will be a behavior change. We need to either handle the exception inside the extracted method, or find a way to return the raw table name.
There was a problem hiding this comment.
I've removed the refactor here. I've kept the same code structure in the compileRequest function.
| static class CompileResult { | ||
| final PinotQuery _pinotQuery; | ||
| final PinotQuery _serverPinotQuery; | ||
| Schema _schema; |
There was a problem hiding this comment.
(minor) Should this be final?
| @@ -306,139 +306,71 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO | |||
| } | |||
| } | |||
|
|
|||
| static class CompileResult { | |||
There was a problem hiding this comment.
(minor) This can be private since it is just a helper wrapper class
vrajat
force-pushed
the
rv-extract-compile-request
branch
from
1f1da71 to
79b0b4f
Compare
BaseSingleStageBrokerRequestHandler.doHandleRequest is a large function of ~600 LOC. This makes it hard to reason about the function as well as extend and reuse parts of the function. The function can be broken down into four parts:
This PR only extracts the code to compile the request. The o/p of compile is stored in a
CompileResultobject that the rest of the code can use. A couple of open points are:BrokerResponse. This can be taken up in a subsequent PR.Otherwise the code has not been changed.