Conversation
* Altered `QueryTestBuilder` to be able to switch to a backing quidem test * added a small crc to ensure that the shadow testcase does not deviate from the original one * Packaged all decoupled related things into a a single `DecoupledExtension` to reduce copy-paste * `DecoupledTestConfig#quidemReason` must describe why its being used * `DecoupledTestConfig#separateDefaultModeTest` can be used to make multiple case files based on `NullHandling` state * fixed a cosmetic bug during decoupled join translation * enhanced `!druidPlan` to report the final logical plan in non-decoupled mode as well * add check to ensure that only supported params are present in a druidtest uri * enabled shadow testcases for previously disabled testcases
style fixes clenaup
This reverts commit 3fbb3cb.
| /** | ||
| * Closely related to {@link CliBroker#getModules}. | ||
| */ | ||
| static List<? extends Module> brokerModules() |
There was a problem hiding this comment.
Where do we bind the ClientQuerySegmentWalker here? I haven't gone through the complete PR, however I wanted to know that so that I have a clearer understanding of what's going on.
There was a problem hiding this comment.
the role of this module is only to expose the ComponentSupplier as a broker; the QuerySegmentWalker is a more core part which is needed for regular tests as well - its exposed at SqlTestFramework#TestSetupModule#querySegmentWalker
actually this is the PR I was finally able to pull that object into the guice world - doing these from guice will make it possible to get rid of the complicated way the system have launched - and could also make it easier to customize the environment further
| .plus(DruidLogicalConvention.instance()), | ||
| newRoot | ||
| ); | ||
| Hook.JAVA_PLAN.run(newRoot); |
There was a problem hiding this comment.
I think it would be a bit nicer if this didn't have to rely on statics and could instead rely on an instance object. For this QueryHandler itself, it looks like these end up constructed via the DruidPlanner which is built by the PlannerFactory which is injected via Guice. Could we get the DruidHook (or something equivalent to it) injected into that and pushed down to this object instead of relying on statics?
There was a problem hiding this comment.
I was also thinking about doing that - but opted to not do that at first
fortunately 90% of the necessary machinery was already in place...so it wasn't really hard to plug it in via guice...but I had to touch a few more files (+20)
(cherry picked from commit 8d40cca50a3e4c8098f49f5d588c7b7220b76788)
asdf2014
left a comment
asdf2014
left a comment
There was a problem hiding this comment.
This failure doesn't seem related to this PR. We need to increase the value of timeout option in this unit test case to stabilize it.
|
merged into master |
6 participants
Hookmigrates to useDruidHookwhich we can add further keysquidem-itmodule which could be the place for tests which could iteract with modules/etc