Conversation
* adds a test scoped jdbc driver for `druidtest:///` backed `DruidAvaticaTestDriver` * `DecoupledTestConfig` can switch test validation to be backed by a quidem execution * this enables plan/etc validation for those cases as well * `DruidQuidemTestBase` can be used to create module level set of quidem tests * added quidem commands: `!nativePlan`, `!logicalPlan`, `!druidPlan`, `!convertedPlan`
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| protected final void executeExplain(Context x) | ||
| { | ||
| List<RelNode> logged = new ArrayList<>(); | ||
| try (final Hook.Closeable unhook = hook.add((Consumer<RelNode>) logged::add)) { |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation
|
|
||
| public void logQueriesForGlobal(Runnable r) | ||
| { | ||
| try (final Hook.Closeable unhook = Hook.QUERY_PLAN.add(this::accept)) { |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation
| ColumnType.STRING, | ||
| new SubstringDimExtractionFn(0, 1) | ||
| ) | ||
| ) |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation
imply-cheddar
left a comment
imply-cheddar
left a comment
There was a problem hiding this comment.
The actual test code looks fine to me. I'd be willing to approve that, but the setup of passing a Closer through the injector is a bit too error-prone. So, I'd like to see that adjusted before approving.
| // this is not nice - but it makes it possible to do queryContext | ||
| // changes |
There was a problem hiding this comment.
A nit about the comment, "this is not nice" will make the next developer that comes along believe that there is something wrong with the code. But, like, have no clue what is wrong. Like, what's "not nice"? Why is it "not nice"? Either leave out the statement and just say "we include the connectionFactory again so that it can see queryContext changes" or update the comment to include an explanation of what's sub-optimal and what future work could be done to make it better.
Sorry for being anal about "just a comment" but when developers are reading new code, they tend to put a ton of faith in the comments written in that code, even though the comments are often an after-thought/just a random thing written and forgotten by the original developer.
There was a problem hiding this comment.
decided that its the best to remove this comment - as it doesn't add much value; I believe its not a big issue
| // test pulls in a module, then pull in that module, even though we are | ||
| // not the Druid node to which the module is scoped. | ||
| .ignoreLoadScopes() | ||
| .addModule(binder -> binder.bind(Closer.class).toInstance(resourceCloser)) |
There was a problem hiding this comment.
What's depending on a Closer to be injected? There should likely be some other way of achieving this.
There was a problem hiding this comment.
Looks like it's the AvaticaServer. That should have its lifecycle managed more closely/explicitly. Just relying on a closer like this generates a pattern in code that leads to resource leaks and poor lifecycle management (I realize that it's probably safe in this one test, but the pattern itself gets seen and then repeated).
You should either mark it as @LifecycleManaged, if the test is getting a Lifecycle object, calling .start() and .stop() on it, then this will end up registering the object with that Lifecycle just naturally. This is the best pattern.
If the test isn't using the Lifecycle object, then it should be explicitly getting the AvaticaServer instance out of the Injector and closing it explicitly in the close method.
There was a problem hiding this comment.
we've talked about this offline - to fix this by properly closing the object where it was created (this is the current version).
the LifecycleManaged is not yet used around here - but it would be much better to rely on that; I'll probably do that in a later changeset - if the need to cleanup configurations will be necessary.
(cherry picked from commit 1fc55a9)
sql/src/test/java/org/apache/druid/sql/calcite/SqlTestFrameworkConfig.java
Fixed
Show fixed
Hide fixed
sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java
Fixed
Show fixed
Hide fixed
|
I've just noticed that some of the review updates were committed to a different branch: for example the |
* test scoped jdbc driver for druidtest:/// backed DruidAvaticaTestDriver ** DecoupledTestConfig is used inside the URI - this will make it possible to attach to existing things more easily * DruidQuidemTestBase can be used to create module level set of quidem tests * added quidem commands: !convertedPlan, !logicalPlan, !druidPlan, !nativePlan ** for these I've used some values of the Hook which was there in calcite * there are some shortcuts with proxies(they are only used during testing) - we can probably remove those later
4 participants
druidtest:///backedDruidAvaticaTestDriverDecoupledTestConfigis used inside the URI - this will make it possible to attach to existing things more easilyDruidQuidemTestBasecan be used to create module level set of quidem tests!convertedPlan,!logicalPlan,!druidPlan,!nativePlanHookwhich was there in calcite