julianhyde commented on code in PR #3084:
URL: https://github.com/apache/calcite/pull/3084#discussion_r1121109202
##########
core/src/main/java/org/apache/calcite/jdbc/Driver.java:
##########
@@ -64,6 +64,21 @@ public Driver() {
this.prepareFactory = createPrepareFactory();
}
+ private Driver(Function0<CalcitePrepare> prepareFactory) {
+ new Driver();
Review Comment:
This statement is useless. It creates a Driver and throws it away.
##########
core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java:
##########
@@ -954,7 +971,7 @@ static class CalcitePreparingStmt extends Prepare
private int expansionDepth;
private @Nullable SqlValidator sqlValidator;
- CalcitePreparingStmt(CalcitePrepareImpl prepare,
+ public CalcitePreparingStmt(CalcitePrepareImpl prepare,
Review Comment:
publlc methods need javadoc
##########
core/src/test/java/org/apache/calcite/test/JdbcTest.java:
##########
@@ -771,6 +771,11 @@ private void checkTableFunctionInModel(Class<?> clazz) {
}
}
+ @Test void testCustomValidator() {
+ final Driver driver = new
MockDdlDriver().withPrepareFactory(MockPrepareImpl::new);
+ assertTrue(driver.prepareFactory.apply().getClass() ==
MockPrepareImpl.class);
Review Comment:
Use assertThat(...getClass(), is()). Better error message if it fails.
##########
core/src/main/java/org/apache/calcite/jdbc/Driver.java:
##########
@@ -52,7 +52,7 @@
public class Driver extends UnregisteredDriver {
public static final String CONNECT_STRING_PREFIX = "jdbc:calcite:";
- final Function0<CalcitePrepare> prepareFactory;
+ public final Function0<CalcitePrepare> prepareFactory;
Review Comment:
This should remain package-private; but change the type to `Supplier`. Add a
method
```
public CalcitePrepare createPrepare() {
return prepareFactory.get();
}
```
##########
core/src/main/java/org/apache/calcite/prepare/CalciteSqlValidator.java:
##########
@@ -22,10 +22,13 @@
import org.apache.calcite.sql.SqlOperatorTable;
import org.apache.calcite.sql.validate.SqlValidatorImpl;
-/** Validator. */
-class CalciteSqlValidator extends SqlValidatorImpl {
+/** Validator.
+ *
+ * <p>This class has public access-level to allow overriding and custom
validation logic.</p>
Review Comment:
I don't think this paragraph adds much. I'd remove it.
##########
testkit/src/main/java/org/apache/calcite/test/RelOptFixture.java:
##########
@@ -76,8 +76,10 @@
* <p>A fixture is immutable. If you have two test cases that require a similar
* set up (for example, the same SQL expression and set of planner rules), it
is
* safe to use the same fixture object as a starting point for both tests.
+ *
+ * <p>The class has public access-level to allow implementing custom fixtures.
Review Comment:
I don't think this paragraph adds much. I think you should remove it. (It's
not even true; the constructor remains package-private because we don't want
people subclassing.)
##########
core/src/main/java/org/apache/calcite/jdbc/Driver.java:
##########
@@ -64,6 +64,21 @@ public Driver() {
this.prepareFactory = createPrepareFactory();
}
+ private Driver(Function0<CalcitePrepare> prepareFactory) {
+ new Driver();
+ this.prepareFactory = prepareFactory;
+ }
+
+ /** Allows changing prepareFactory without having to subclass Driver.
+ *
+ * @param prepareFactory {@link org.apache.calcite.jdbc.CalcitePrepare}
+ * @return Driver with the provided prepareFactory
+ */
+ public Driver withPrepareFactory(Function0<CalcitePrepare> prepareFactory) {
Review Comment:
Use java.util.Supplier rather than Guava Function0
--
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]
