Copilot commented on code in PR #5300: URL: https://github.com/apache/calcite/pull/5300#discussion_r4229463335
########## core/src/main/java/org/apache/calcite/runtime/CursorInputs.java: ########## @@ -0,0 +1,222 @@ +/* + * 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.runtime; + +import org.apache.calcite.avatica.AvaticaResultSet; +import org.apache.calcite.avatica.AvaticaResultSetMetaData; +import org.apache.calcite.avatica.ColumnMetaData; +import org.apache.calcite.avatica.Meta; +import org.apache.calcite.linq4j.AbstractEnumerable; +import org.apache.calcite.linq4j.Enumerable; +import org.apache.calcite.linq4j.Enumerator; +import org.apache.calcite.linq4j.function.Function1; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.sql.util.CursorInput; +import org.apache.calcite.util.Util; + +import org.checkerframework.checker.nullness.qual.Nullable; + +import java.sql.ResultSet; +import java.sql.SQLException; +import java.util.Collections; +import java.util.List; +import java.util.TimeZone; + +import static java.util.Objects.requireNonNull; + +/** Utilities for working with {@link CursorInput}. */ +public final class CursorInputs { + private CursorInputs() { + } + + public static CursorInput of(RelDataType rowType, Enumerable<Object[]> rows) { + return new CursorInput() { + @Override public RelDataType getRowType() { + return rowType; + } + + @Override public Enumerable<Object[]> rows() { + return rows; + } + }; + } + + public static ResultSet metadataOnly(List<ColumnMetaData> columns) { + return resultSet(new AbstractEnumerable<Object[]>() { + @Override public Enumerator<Object[]> enumerator() { + throw new UnsupportedOperationException( + "Cursor rows are unavailable during type inference"); Review Comment: `metadataOnly` returns a `ResultSet` whose `next()` ultimately throws `UnsupportedOperationException`. That is surprising for many `ResultSet` consumers (even in type inference), and can break table functions that defensively call `next()` before reading metadata. A more contract-friendly approach is to return a `ResultSet` backed by an empty enumerable (so `next()` returns `false`) while still providing the prepared metadata via `getMetaData()`. ########## core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java: ########## @@ -767,6 +767,12 @@ private static SqlValidator createSqlValidator(Context context, config); } + public static List<ColumnMetaData> getColumnMetaDataList( + JavaTypeFactory typeFactory, RelDataType rowType) { + return getColumnMetaDataList(typeFactory, rowType, rowType, + Collections.nCopies(rowType.getFieldCount(), null)); + } Review Comment: This introduces a new `public static` method on `CalcitePrepareImpl`, which expands the publicly accessible surface area of what is usually considered an internal implementation class. If this helper is intended for internal reuse, consider moving it to a dedicated utility class (e.g., alongside `CursorInputs`) or reducing its visibility (package-private) if feasible, to avoid unintentionally committing to it as part of a supported API. ########## core/src/test/java/org/apache/calcite/test/TableFunctionTest.java: ########## @@ -54,6 +67,187 @@ * @see Smalls */ class TableFunctionTest { + @Test void testCursorInput() { + withCursorFunctions() + .query("select * from table(\"s\".\"echo\"(" + + "cursor(select * from (values (1), (2)) as t(n))))") + .returnsUnordered("N=1", "N=2"); + } + + private CalciteAssert.AssertThat withCursorFunctions() { + return CalciteAssert.that() + .withSchema("s", new AbstractSchema()) + .with(connection -> { + final SchemaPlus schema = connection.unwrap(CalciteConnection.class) + .getRootSchema().subSchemas().get("s"); + schema.add("echo", TableFunctionImpl.create(CursorTable.class)); + schema.add("queryable", TableFunctionImpl.create(CursorTable.class, "queryable")); + schema.add("add", TableFunctionImpl.create(CursorTable.class, "add")); + schema.add("concat", TableFunctionImpl.create(CursorTable.class, "concat")); + return connection; + }); + } + + @Test void testCursorInputMultipleColumns() { + withCursorFunctions() + .query("select * from table(\"s\".\"echo\"(" + + "cursor(select * from (values (1, 'a'), (2, 'b')) as t(n, s))))") + .returnsUnordered("N=1; S=a", "N=2; S=b"); + } + + @Test void testCursorInputQueryableTable() { + withCursorFunctions() + .query("select * from table(\"s\".\"queryable\"(" + + "cursor(select * from (values (1, 'a'), (2, 'b')) as t(n, s))))") + .returnsUnordered("N=1; S=a", "N=2; S=b"); + } + + @Test void testCursorInputEmpty() { + withCursorFunctions() + .query("select * from table(\"s\".\"echo\"(" + + "cursor(select * from (values (1)) as t(n) where n < 0)))") + .returnsCount(0); + } + + @Test void testCursorInputScalarQuery() { + withCursorFunctions() + .query("select * from table(\"s\".\"add\"(" + + "cursor(select * from (values (1), (2)) as t(n))," + + " (select max(x) from (values (10), (20)) as d(x))))") + .returnsUnordered("N=21", "N=22"); + } + + @Test void testCursorInputMultipleCursors() { + withCursorFunctions() + .query("select * from table(\"s\".\"concat\"(" + + "cursor(select * from (values (1), (2)) as t(n))," + + "cursor(select * from (values (3), (4)) as t(n))))") + .returnsUnordered("N=1", "N=2", "N=3", "N=4"); + } + + @Test void testCursorInputNestedAndNull() { + withCursorFunctions() + .query("select * from table(\"s\".\"echo\"(cursor(" + + "select * from table(\"s\".\"echo\"(cursor(" + + "select * from (values (1), (cast(null as integer))) as t(n)))))))") + .returnsUnordered("N=1", "N=null"); + } + + @Test void testCursorInputPrepared() throws Exception { + withCursorFunctions().doWithConnection(connection -> { + try (PreparedStatement statement = + connection.prepareStatement("select * from table(\"s\".\"echo\"(" + + "cursor(select * from (values (1), (2)) as t(n) where n > ?)))")) { + statement.setInt(1, 0); + try (ResultSet result = statement.executeQuery()) { + assertThat(CalciteAssert.toString(result), equalTo("N=1\nN=2\n")); + } + statement.setInt(1, 1); + try (ResultSet result = statement.executeQuery()) { + assertThat(CalciteAssert.toString(result), equalTo("N=2\n")); + } + } catch (SQLException e) { + throw TestUtil.rethrow(e); + } + }); + } + + /** Java table function that receives its cursor arguments as result sets. */ + public static class CursorTable extends AbstractTable implements ScannableTable { + private final ResultSet input; + + private CursorTable(ResultSet input) { + this.input = input; + } + + public static ScannableTable eval(ResultSet input) { + return new CursorTable(input); + } + + public static QueryableTable queryable(ResultSet input) { + return new AbstractQueryableTable(Object[].class) { + @Override public RelDataType getRowType(RelDataTypeFactory typeFactory) { + return new CursorTable(input).getRowType(typeFactory); + } + + @Override public <T> Queryable<T> asQueryable(QueryProvider provider, + SchemaPlus schema, String tableName) { + return (Queryable<T>) new CursorTable(input).scan(DataContexts.EMPTY).asQueryable(); + } + }; + } + + public static ScannableTable add(ResultSet input, Integer offset) { + return new CursorTable(input) { + @Override public Enumerable<Object[]> scan(DataContext root) { + return super.scan(root).select(row -> new Object[] {(Integer) row[0] + offset}); + } + }; + } + + public static ScannableTable concat(ResultSet first, ResultSet second) { + return new CursorTable(first) { + @Override public Enumerable<Object[]> scan(DataContext root) { + return super.scan(root).concat(new CursorTable(second).scan(root)); + } + }; + } + + @Override public RelDataType getRowType(RelDataTypeFactory typeFactory) { + final RelDataTypeFactory.Builder builder = typeFactory.builder(); + try { + final java.sql.ResultSetMetaData metadata = input.getMetaData(); + for (int i = 1; i <= metadata.getColumnCount(); i++) { + final SqlTypeName type = SqlTypeName.getNameForJdbcType(metadata.getColumnType(i)); + final RelDataType columnType = type.allowsPrecNoScale() + ? typeFactory.createSqlType(type, metadata.getPrecision(i)) + : typeFactory.createSqlType(type); + builder.add(metadata.getColumnLabel(i), columnType) + .nullable(metadata.isNullable(i) != java.sql.ResultSetMetaData.columnNoNulls); + } + return builder.build(); + } catch (SQLException e) { + throw TestUtil.rethrow(e); + } + } + + @Override public Enumerable<Object[]> scan(DataContext root) { + return new AbstractEnumerable<Object[]>() { + @Override public Enumerator<Object[]> enumerator() { + return new Enumerator<Object[]>() { + @Override public Object[] current() { + try { + final Object[] row = new Object[input.getMetaData().getColumnCount()]; + for (int i = 0; i < row.length; i++) { + row[i] = input.getObject(i + 1); + } + return row; + } catch (SQLException e) { + throw TestUtil.rethrow(e); + } + } + + @Override public boolean moveNext() { + try { + return input.next(); + } catch (SQLException e) { + throw TestUtil.rethrow(e); + } + } + + @Override public void reset() { + throw new UnsupportedOperationException(); + } + + @Override public void close() { + // The engine owns and closes the input ResultSet. + } + }; Review Comment: In `CursorTable.scan()`, `current()` calls `input.getMetaData().getColumnCount()` on every row. JDBC metadata calls can be relatively expensive; this can become a noticeable per-row overhead for larger inputs. Cache the column count (and ideally the `ResultSetMetaData`) once when the enumerator is created, then reuse it in `current()`. ########## src/main/config/checkerframework/avatica/AvaticaResultSetMetaData.astub: ########## @@ -21,10 +21,10 @@ import org.checkerframework.checker.nullness.qual.*; import java.sql.*; class AvaticaResultSet { - AvaticaResultSet(AvaticaStatement statement, + AvaticaResultSet(@Nullable AvaticaStatement statement, @Nullable QueryState state, Meta.Signature signature, ResultSetMetaData resultSetMetaData, TimeZone timeZone, - Meta.Frame firstFrame); + @Nullable Meta.Frame firstFrame); } Review Comment: This stub file is named `AvaticaResultSetMetaData.astub` but declares `class AvaticaResultSet`, and the same `AvaticaResultSet` constructor stub also appears in `AvaticaResultSet.astub`. Having the same class stub in multiple `.astub` files (and a filename/class mismatch) makes it easy for future edits to diverge and can cause hard-to-debug Checker Framework stub resolution issues. Consider moving this `AvaticaResultSet` stub to only `AvaticaResultSet.astub` (or renaming the file / correcting the declared class to match the filename) so there is a single source of truth. -- 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]
