Enable quidem shadowing for decoupled testcases #16431
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
There was a problem hiding this comment.
I didn't interrogate the .iq files as I believe that they are being primarily auto-generated and adjusted at this point. I see that the tests haven't changed, there's just extra definitions of things, so that all seems fine.
I left a few comments related to developer ergonomics more than content of the PR. If we can address those and then get green checkmarks again, I'm good to merge, so approving.
| @@ -1,4 +1,4 @@ | |||
| !use druidtest://?numMergeBuffers=3 | |||
There was a problem hiding this comment.
Why the change in case? keeping camel-case-for-variables as the convention seems good to me to avoid random errors from people forgetting that they need to capitalize the first letter in this one place.
There was a problem hiding this comment.
before this patch there was no validation enforced on these options - this patch introduced that and failed right away as this not valid
| .add(NumMergeBuffers.class.getSimpleName()) | ||
| .add(MinTopNThreshold.class.getSimpleName()) | ||
| .add(ResultCache.class.getSimpleName()) | ||
| .add(ComponentSupplier.class.getSimpleName()) |
There was a problem hiding this comment.
I see where the uppercase is coming from. I think it would be better to make these have a lowercase letter at the beginning, as I think that there will be more people trying to write these tests then there are adding things here. If we take a few minutes to create a simple static method that can take the Class, grab the simple name and lowercase the first letter, these lines can become
.add(toConfigName(NumMergeBuffers.class))
Which is equivalently easy to write.
There was a problem hiding this comment.
definetly true - changed it to use CaseFormat.LOWER_CAMEL; add that as a final method under the processor; it ended up as:
NumMergeBuffers.PROCESSOR.getConfigName()`
| if (!equals(newConfig)) { | ||
| throw new IAE("Some configs are not handled in this method or not persistable as QueryContext keys!\nold: %s\nnew: %s", this, newConfig); | ||
| } |
There was a problem hiding this comment.
This error message is just dumping the maps, but it's unclear what you are supposed to do with that? Who is this error message for? Is the expectation that the maps have the same keys? Which one had a bad key?
Generally speaking, can you make this a DruidException with relevant persona (is this something a user will see?), explain a bit more about what is expected and what the person is supposed to do?
There was a problem hiding this comment.
let me share a small back-story for this:
- for
BaseCalciteQueryTest#testBuildersomeone can specifyqueryContextandplannerConfigas well - however most of theplannerContextsettings can be achieved by settingqueryContextvariables - the quidem tests only support changes to the queryContext - so there was a need to retrieve the settings from the
plannerConfiginqueryContextform - I believe at some point it would be better to have
PlannerConfigas an implicit settings class which is implicitly defined by thequeryContextsettings
this method was just needed in that process - I've change the exception message to
Not all PlannerConfig options are not persistable as QueryContext keys!\nold: %s\nnew: %s
and used DruidException.defensive instead; as only a developer could fix this issue.
QueryTestBuilderto be able to switch to a backing quidem testDecoupledExtensionto reduce copy-pasteDecoupledTestConfig#quidemReasonmust describe why its being usedDecoupledTestConfig#separateDefaultModeTestcan be used to make multiple case files based onNullHandlingstate!druidPlanto report the final logical plan in non-decoupled mode as wellFixes #XXXX.
Description
Fixed the bug ...
Renamed the class ...
Added a forbidden-apis entry ...
Release note
Key changed/added classes in this PR
MyFooOurBarTheirBazThis PR has: