julianhyde commented on code in PR #3421: URL: https://github.com/apache/calcite/pull/3421#discussion_r1332026824
########## core/src/main/java/org/apache/calcite/jdbc/CalciteMetaColumnFactory.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.calcite.jdbc; + +import org.apache.calcite.avatica.MetaImpl.MetaColumn; +import org.apache.calcite.schema.Table; + +import org.checkerframework.checker.nullness.qual.Nullable; + +/** Factory for creating MetaColumns for getColumns(). */ +public interface CalciteMetaColumnFactory { + /** Instantiates a MetaColumn. */ + MetaColumn newMetaColumn( + Table table, + String tableCat, + String tableSchem, + String tableName, + String columnName, + int dataType, + String typeName, + Integer columnSize, + @Nullable Integer decimalDigits, + Integer numPrecRadix, + int nullable, + Integer charOctetLength, + int ordinalPosition, + String isNullable); + + /** Returns the list of expected column names. */ + String[] getColumnNames(); + + /** Returns the class of MetaColumn that is created. */ + Class<?> getMetaColumnClass(); Review Comment: 'Returns the type of object created. Must be a sub-class of `MetaColumn`.' Does it work if you change the return type from `Class<?>` to `Class<? extends MetaColumn>`? ########## core/src/main/java/org/apache/calcite/jdbc/CalciteMetaColumnFactory.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.calcite.jdbc; + +import org.apache.calcite.avatica.MetaImpl.MetaColumn; +import org.apache.calcite.schema.Table; + +import org.checkerframework.checker.nullness.qual.Nullable; + +/** Factory for creating MetaColumns for getColumns(). */ +public interface CalciteMetaColumnFactory { + /** Instantiates a MetaColumn. */ + MetaColumn newMetaColumn( + Table table, + String tableCat, + String tableSchem, + String tableName, + String columnName, + int dataType, + String typeName, + Integer columnSize, + @Nullable Integer decimalDigits, + Integer numPrecRadix, Review Comment: If `numPrecRadix` nullable? If not, make it `int`. If so, add `@Nullable`. ########## core/src/test/java/org/apache/calcite/test/JdbcFrontJdbcBackTest.java: ########## @@ -73,6 +79,75 @@ class JdbcFrontJdbcBackTest { }); } + + /** + * Sample subclass used in {@link JdbcFrontJdbcBackTest#testTablesExtraColumn()}. + */ + public static class MetaExtraTable extends CalciteMetaTable { + public final String extraLabel; + MetaExtraTable(Table calciteTable, String tableCat, + String tableSchem, String tableName) { + super(calciteTable, tableCat, tableSchem, tableName); + this.extraLabel = "extraLabel1"; + } + } + + /** Sample factory that creates MetaExtraTables. */ + public static class MetaExtraTableFactoryImpl implements CalciteMetaTableFactory { + + public static final MetaExtraTableFactoryImpl INSTANCE = new MetaExtraTableFactoryImpl(); + + MetaExtraTableFactoryImpl() {} + + @Override public MetaTable newMetaTable(Table table, String tableCat, String tableSchem, + String tableName) { + return new MetaExtraTable(table, tableCat, tableSchem, tableName); + } + + @Override public String[] getColumnNames() { + // 11 columns total. + return new String[] { Review Comment: Would it be an error if the columns were not prefixed by the list of standard columns? (e.g. adding an extra column in the middle, or removing a column, or re-ordering columns) Figure out what the necessary constraints are, and add a test that checks that those constraints are being enforced. ########## core/src/main/java/org/apache/calcite/jdbc/CalciteMetaTableFactory.java: ########## @@ -0,0 +1,32 @@ +/* + * 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.calcite.jdbc; + +import org.apache.calcite.avatica.MetaImpl.MetaTable; +import org.apache.calcite.schema.Table; + +/** Factory for creating MetaTables for getTables(). */ +public interface CalciteMetaTableFactory { + /** Instantiates a MetaTable. */ + MetaTable newMetaTable(Table table, String tableCat, String tableSchem, String tableName); + + /** Returns the list of expected column names. */ + String[] getColumnNames(); Review Comment: Switch from `String[]` to `List<String>` so that you don't need defensive copy. ########## core/src/main/java/org/apache/calcite/jdbc/CalciteMetaImpl.java: ########## @@ -90,13 +90,40 @@ public class CalciteMetaImpl extends MetaImpl { static final Driver DRIVER = new Driver(); + private final CalciteMetaTableFactory metaTableFactory; + private final CalciteMetaColumnFactory metaColumnFactory; + + /** Creates a CalciteMetaImpl. + * + * @deprecated Use + * {@link #CalciteMetaImpl(CalciteConnectionImpl, CalciteMetaTableFactory, CalciteMetaColumnFactory)} + * instead. + */ + @Deprecated // to be removed before 2.0 public CalciteMetaImpl(CalciteConnectionImpl connection) { + this(connection, new CalciteMetaTableFactoryImpl(), new CalciteMetaColumnFactoryImpl()); + } + + public CalciteMetaImpl(CalciteConnectionImpl connection, Review Comment: Add javadoc. I would remove the `@Nullable`. Caller can easily provide factories. ########## core/src/main/java/org/apache/calcite/jdbc/CalciteMetaImpl.java: ########## @@ -816,10 +816,10 @@ public static CalciteConnection connect(CalciteSchema schema, } /** Metadata describing a Calcite table. */ - private static class CalciteMetaTable extends MetaTable { + public static class CalciteMetaTable extends MetaTable { private final Table calciteTable; - CalciteMetaTable(Table calciteTable, String tableCat, + public CalciteMetaTable(Table calciteTable, String tableCat, Review Comment: Now it's public it requires javadoc. ########## core/src/main/java/org/apache/calcite/jdbc/CalciteMetaColumnFactory.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.calcite.jdbc; + +import org.apache.calcite.avatica.MetaImpl.MetaColumn; +import org.apache.calcite.schema.Table; + +import org.checkerframework.checker.nullness.qual.Nullable; + +/** Factory for creating MetaColumns for getColumns(). */ +public interface CalciteMetaColumnFactory { + /** Instantiates a MetaColumn. */ + MetaColumn newMetaColumn( + Table table, + String tableCat, + String tableSchem, + String tableName, + String columnName, + int dataType, + String typeName, + Integer columnSize, + @Nullable Integer decimalDigits, + Integer numPrecRadix, + int nullable, + Integer charOctetLength, + int ordinalPosition, + String isNullable); + + /** Returns the list of expected column names. */ + String[] getColumnNames(); Review Comment: Let's make this `List<String>`. Then you can return an `ImmutableList` and don't have to worry about making a defensive copy. I'd give it a default implementation that returns the columns in the JDBC spec, and add the javadoc 'The default implementation returns the columns in the JDBC specification.' ########## core/src/main/java/org/apache/calcite/jdbc/CalciteMetaImpl.java: ########## @@ -178,11 +205,16 @@ private <E> MetaResultSet createResultSet(Enumerable<E> enumerable, for (String name : names) { final int index = fields.size(); final String fieldName = AvaticaUtils.toCamelCase(name); - final Field field; + Field field; try { field = clazz.getField(fieldName); } catch (NoSuchFieldException e) { - throw new RuntimeException(e); + try { + // Check if subclass contains the desired field. + field = clazz.getDeclaredField(fieldName); + } catch (NoSuchFieldException e2) { + throw new RuntimeException(e2); + } } Review Comment: I am surprised that `getDeclaredField` would ever succeed if `getField` has failed. ########## core/src/test/java/org/apache/calcite/test/JdbcFrontJdbcBackTest.java: ########## @@ -73,6 +79,75 @@ class JdbcFrontJdbcBackTest { }); } + + /** + * Sample subclass used in {@link JdbcFrontJdbcBackTest#testTablesExtraColumn()}. + */ + public static class MetaExtraTable extends CalciteMetaTable { Review Comment: Does this class need to be public? Does its field need to be public? If it can be private, that's good. If it needs to be public (because, say, reflection) then you should say that it needs to be public in the javadoc. ########## core/src/main/java/org/apache/calcite/jdbc/CalciteMetaTableFactoryImpl.java: ########## @@ -0,0 +1,56 @@ +/* + * 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.calcite.jdbc; + +import org.apache.calcite.jdbc.CalciteMetaImpl.CalciteMetaTable; +import org.apache.calcite.schema.Table; + +/** Default implementation of CalciteMetaTableFactory. */ +public class CalciteMetaTableFactoryImpl implements + CalciteMetaTableFactory { + + public static final CalciteMetaTableFactoryImpl INSTANCE = new CalciteMetaTableFactoryImpl(); + public CalciteMetaTableFactoryImpl() {} + + protected static final String[] COLUMN_NAMES = + new String[] {"TABLE_CAT", Review Comment: Remove `new String[]`. It is unnecessary when declaring and initializing an array-valued field in java. ########## core/src/main/java/org/apache/calcite/jdbc/CalciteMetaColumnFactory.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.calcite.jdbc; + +import org.apache.calcite.avatica.MetaImpl.MetaColumn; +import org.apache.calcite.schema.Table; + +import org.checkerframework.checker.nullness.qual.Nullable; + +/** Factory for creating MetaColumns for getColumns(). */ +public interface CalciteMetaColumnFactory { + /** Instantiates a MetaColumn. */ + MetaColumn newMetaColumn( Review Comment: I would prefer `createColumn`. What the method does is create (and populate) a column. "new" is a java operator to achieve that, but "new" isn't a verb. "Meta" seems to be spurious. Ditto "newMetaTable" should be "createTable". -- 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]
