julianhyde commented on a change in pull request #2305:
URL: https://github.com/apache/calcite/pull/2305#discussion_r549509647
##########
File path: core/src/test/java/org/apache/calcite/test/TableInRootSchemaTest.java
##########
@@ -179,5 +185,21 @@ public RelNode toRel(RelOptTable.ToRelContext context,
RelOptTable relOptTable) {
return EnumerableTableScan.create(context.getCluster(), relOptTable);
}
+
+ @Override public Expression getExpression(SchemaPlus schema, String
tableName, Class clazz) {
+ try {
+ MethodCallExpression queryableExpression =
+
Expressions.call(Expressions.new_(SimpleTable.class.getConstructor()),
+ QueryableTable.class.getMethod(
+ "asQueryable", QueryProvider.class, SchemaPlus.class,
String.class),
Review comment:
Convert to a BuiltInMethod constant
##########
File path: core/src/main/java/org/apache/calcite/prepare/RelOptTableImpl.java
##########
@@ -118,9 +115,9 @@ public static RelOptTableImpl create(
RelDataType rowType,
List<String> names,
Table table,
- Expression expression) {
+ Function<Class, Expression> expressionFunction) {
return new RelOptTableImpl(schema, rowType, names, table,
- c -> expression, table.getStatistic().getRowCount());
+ expressionFunction, table.getStatistic().getRowCount());
Review comment:
`expressionFunction` is not a very good abstraction. That didn't matter
when it was private. But now it is public. How can we improve the abstraction?
##########
File path: core/src/test/java/org/apache/calcite/test/JdbcTest.java
##########
@@ -461,6 +462,34 @@ private void addTableMacro(Connection connection, Method
method) throws SQLExcep
connection.close();
}
+ @Test void testQueryableTableWithTableMacro() throws SQLException {
Review comment:
JdbcTest is already huge. Is there a better place for this test. Say
`TableFunctionTest`?
##########
File path: core/src/test/java/org/apache/calcite/test/TableInRootSchemaTest.java
##########
@@ -97,7 +102,8 @@
private Class[] columnTypes = { String.class, Integer.class };
private Object[][] rows = new Object[3][];
- SimpleTable() {
+ // CHECKSTYLE: IGNORE 1
Review comment:
what are we ignoring?
##########
File path: core/src/main/java/org/apache/calcite/prepare/RelOptTableImpl.java
##########
@@ -118,9 +115,9 @@ public static RelOptTableImpl create(
RelDataType rowType,
List<String> names,
Table table,
- Expression expression) {
+ Function<Class, Expression> expressionFunction) {
Review comment:
This is a breaking change to a public method. (Albeit a method that has
no javadoc.) We need to keep the old method (deprecated) for compatibility.
New method needs javadoc.
##########
File path: core/src/main/java/org/apache/calcite/schema/Schemas.java
##########
@@ -183,6 +183,33 @@ public static Expression tableExpression(SchemaPlus
schema, Type elementType,
return EnumUtils.convert(expression, clazz);
}
+ /**
+ * Generates an expression with which table can be referenced in
+ * generated code.
+ *
+ * @param schema Schema
+ * @param tableName Table name (unique within schema)
+ * @param table table to be referenced
+ * @param clazz The desired collection class; for example {@code
Queryable}.
Review comment:
`clazz` is always the class of `table`, right? It's not clear, reading
the javadoc.
##########
File path: core/src/test/java/org/apache/calcite/test/JdbcTest.java
##########
@@ -461,6 +462,34 @@ private void addTableMacro(Connection connection, Method
method) throws SQLExcep
connection.close();
}
+ @Test void testQueryableTableWithTableMacro() throws SQLException {
+ try (Connection connection =
+ DriverManager.getConnection("jdbc:calcite:")) {
+
+ CalciteConnection calciteConnection =
+ connection.unwrap(CalciteConnection.class);
+ SchemaPlus rootSchema = calciteConnection.getRootSchema();
+ SchemaPlus schema = rootSchema.add("s", new AbstractSchema());
+ schema.add("simple", new TableMacro() {
Review comment:
Consider adding to `Smalls` as a class or method. As an anonymous class,
it is not clear what you are trying to achieve.
##########
File path: core/src/main/java/org/apache/calcite/prepare/RelOptTableImpl.java
##########
@@ -163,22 +160,7 @@ public RelOptTableImpl copy(RelDataType newRowType) {
private static Function<Class, Expression> getClassExpressionFunction(
Review comment:
is the `getClassExpressionFunction` method now obsolete?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
