paul-rogers commented on code in PR #13426:
URL: https://github.com/apache/druid/pull/13426#discussion_r1037750456
##########
core/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java:
##########
@@ -36,6 +36,7 @@
import javax.validation.Validator;
import java.util.Properties;
+@LazySingleton
Review Comment:
Good question! This PR has unit tests for the "injector builders". As it
turns out, those builders will create an instance of this class even if we
don't explicitly configure it. (Guice helpfully creates instances as long as
the constructor is of the right form.) When created that way, Guice would
create multiple instances of this class, causing tests to fail.
I think this reveals why folks sometimes put annotations on classes: it is
needed for Guice's implicit object creation path.
##########
processing/src/main/java/org/apache/druid/jackson/DefaultObjectMapper.java:
##########
@@ -42,6 +42,8 @@
*/
public class DefaultObjectMapper extends ObjectMapper
{
+ public static final DefaultObjectMapper INSTANCE = new DefaultObjectMapper();
Review Comment:
The idea is that if a unit test needs the default mapper, then rather than
creating one per test, and wondering which one we're using, we just use this
single global one. Right now, there is only one test where I made that change.
We can change others as we come across them.
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteIngestionDmlTest.java:
##########
@@ -299,13 +363,23 @@ private void verifySuccess()
.expectedResources(expectedResources)
.run();
+ String expectedLogicalPlan;
+ if (expectedLogicalPlanResource != null) {
+ expectedLogicalPlan = StringUtils.getResource(
+ this,
+ "/calcite/expected/ingest/" + expectedLogicalPlanResource +
"-logicalPlan.txt"
Review Comment:
Yes, that is true. If we had Java 17, we could easily put the text inline in
the code. I thought about fiddling with quotes, or building up a chunk of text
from a list of lines, but in the end said, the heck with it, let's just use
resource files.
In code that uses resource files with tests, it is common to define a
directory structure to organize the files. This like creates the "structure"
for the Calcite DML tests: the stuff goes in that one folder.
This doesn't generalize (yet): if any other Calcite test (other than DML)
wants to use resources, then there is no defined place to put the files. We
could move this higher in the class hierarchy. (Or, wait for @imply-cheddar to
commit his file-based test solution.)
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerCaptureHook.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.calcite.planner;
+
+import org.apache.calcite.interpreter.BindableRel;
+import org.apache.calcite.rel.RelRoot;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.sql.SqlInsert;
+import org.apache.druid.sql.calcite.rel.DruidRel;
+
+public class PlannerCaptureHook implements PlannerHook
+{
+ // Uncomment the various fields, and add getter, when
+ // needed by tests. Code won't pass SpotBugs otherwise.
+ //private String sql;
Review Comment:
Yeah, this is a third or forth generation of this code, a bit far removed
from its original creation. I've gone ahead and removed the fields.
Unused public fields are hard to deal with: everything complains about the
and I seem to need to add multiple markers to get the tools to let me proceed.
We'll just add 'em back later if/when we need 'em.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
