thomaskirz commented on code in PR #4872: URL: https://github.com/apache/calcite/pull/4872#discussion_r3274603566
########## core/src/test/java/org/apache/calcite/jdbc/JdbcSchemaDecimalBugTest.java: ########## @@ -0,0 +1,298 @@ +/* + * 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.adapter.jdbc.JdbcSchema; +import org.apache.calcite.schema.SchemaPlus; + +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.EnabledIfSystemProperty; + +import java.io.PrintWriter; +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.ResultSet; +import java.sql.ResultSetMetaData; +import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; +import java.sql.Statement; +import java.util.Properties; +import java.util.logging.Logger; +import javax.sql.DataSource; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +/** + * Integration tests for + * <a href="https://issues.apache.org/jira/browse/CALCITE-6654";>[CALCITE-6654] + * JdbcSchema throws error for NUMERIC/DECIMAL columns without explicit + * precision (Oracle, PostgreSQL, MSSQL)</a>. + * + * <p>These are integration tests that require live database instances. + * Run with: + * <pre>{@code ./gradlew :core:integTestCalcite6654}</pre> + * + * <p>Required Docker containers – start with: + * <pre>{@code + * docker compose -f core/src/test/resources/docker-compose.yml up -d + * }</pre> + * + * <p>Tests are skipped automatically if a container is unreachable. + */ + +@Tag("integration") +@EnabledIfSystemProperty(named = "calcite.integration.tests", matches = "true") +class JdbcSchemaDecimalBugTest { + + // --------------------------------------------------------------------------- + // DataSource factory + // --------------------------------------------------------------------------- + + /** + * Minimal {@link DataSource} backed by a JDBC URL. + * Sufficient for integration tests; not for production use. + */ + private static class DriverManagerDataSource implements DataSource { + private final String url; + private final String user; + private final String password; + + DriverManagerDataSource(String url, String user, String password) { + this.url = url; + this.user = user; + this.password = password; + } + + @Override public Connection getConnection() throws SQLException { + return DriverManager.getConnection(url, user, password); + } + + @Override public Connection getConnection(String u, String p) throws SQLException { + return getConnection(); + } + + @Override public PrintWriter getLogWriter() { + return null; + } + + @Override public void setLogWriter(PrintWriter w) { + } + + @Override public void setLoginTimeout(int seconds) { + } + + @Override public int getLoginTimeout() { + return 0; + } + + @Override public Logger getParentLogger() throws SQLFeatureNotSupportedException { + throw new SQLFeatureNotSupportedException("getParentLogger"); + } + + @Override public <T> T unwrap(Class<T> iface) throws SQLException { + throw new SQLException("Not a wrapper for " + iface); + } + + @Override public boolean isWrapperFor(Class<?> iface) { + return false; + } + } + + private static DataSource oracleDataSource() { + return new DriverManagerDataSource( + "jdbc:oracle:thin:@localhost:1521/FREEPDB1", "system", "testpass"); + } + + private static DataSource postgresDataSource() { + return new DriverManagerDataSource( + "jdbc:postgresql://localhost:5432/testdb", "testuser", "testpass"); + } + + private static DataSource mssqlDataSource() { + return new DriverManagerDataSource( + "jdbc:sqlserver://localhost:1433;databaseName=master;encrypt=false", + "sa", "TestPass123!"); + } + + // --------------------------------------------------------------------------- + // Helper methods + // --------------------------------------------------------------------------- + + /** Skips the test if the database is not reachable. */ + private static void assumeReachable(DataSource ds, String dbName) { + try (Connection ignored = ds.getConnection()) { + // connection succeeded - proceed with the test + } catch (SQLException e) { + assumeTrue(false, + dbName + " is not reachable - skipping test. Cause: " + e.getMessage()); + } + } + + /** + * Executes DDL and DML statements to set up a test table. + * Silently ignores the drop statement if the table does not exist. + */ + private static void createTestTable(DataSource ds, String dropSql, + String createSql, String insertSql) throws SQLException { + try (Connection conn = ds.getConnection(); + Statement st = conn.createStatement()) { + try { + st.execute(dropSql); + } catch (SQLException ignored) { + // table did not exist yet - that is fine + } + st.execute(createSql); + st.execute(insertSql); + } + // connection is closed here via try-with-resources + } + + // --------------------------------------------------------------------------- + // Tests - Bug fix: columns without explicit precision must not throw + // --------------------------------------------------------------------------- + + /** + * Oracle {@code NUMBER} without precision (e.g. {@code col NUMBER}) must + * not cause Calcite to throw during schema registration or query execution. + * + * <p>Oracle reports {@code precision=0, scale=-127} for such columns. + */ + @Test void oracleNumberWithoutPrecisionShouldNotThrow() throws Exception { + DataSource ds = oracleDataSource(); + assumeReachable(ds, "Oracle"); + + // Oracle stores unquoted identifiers in UPPERCASE + createTestTable(ds, + "BEGIN EXECUTE IMMEDIATE 'DROP TABLE TEST_NUMBERS';" + + " EXCEPTION WHEN OTHERS THEN NULL; END;", + "CREATE TABLE TEST_NUMBERS (id NUMBER, val NUMBER)", + "INSERT INTO TEST_NUMBERS VALUES (1, 42.5)"); + + try (Connection conn = DriverManager.getConnection("jdbc:calcite:", new Properties())) { + CalciteConnection calciteConn = conn.unwrap(CalciteConnection.class); + SchemaPlus root = calciteConn.getRootSchema(); + root.add("oracle", JdbcSchema.create(root, "oracle", ds, null, "SYSTEM")); + try (Statement st = conn.createStatement(); + ResultSet rs = + st.executeQuery("SELECT * FROM \"oracle\".\"TEST_NUMBERS\"")) { + while (rs.next()) { + //drain + } + } + } + } + + /** + * PostgreSQL {@code NUMERIC} without precision (e.g. {@code col NUMERIC}) + * must not cause Calcite to throw. + * <p>PostgreSQL reports {@code precision=0, scale=0} for such columns. Review Comment: ```suggestion * * <p>PostgreSQL reports {@code precision=0, scale=0} for such columns. ``` ########## core/src/test/java/org/apache/calcite/jdbc/JdbcSchemaDecimalBugTest.java: ########## @@ -0,0 +1,298 @@ +/* + * 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.adapter.jdbc.JdbcSchema; +import org.apache.calcite.schema.SchemaPlus; + +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.EnabledIfSystemProperty; + +import java.io.PrintWriter; +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.ResultSet; +import java.sql.ResultSetMetaData; +import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; +import java.sql.Statement; +import java.util.Properties; +import java.util.logging.Logger; +import javax.sql.DataSource; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +/** + * Integration tests for + * <a href="https://issues.apache.org/jira/browse/CALCITE-6654";>[CALCITE-6654] + * JdbcSchema throws error for NUMERIC/DECIMAL columns without explicit + * precision (Oracle, PostgreSQL, MSSQL)</a>. + * + * <p>These are integration tests that require live database instances. + * Run with: + * <pre>{@code ./gradlew :core:integTestCalcite6654}</pre> + * + * <p>Required Docker containers – start with: + * <pre>{@code + * docker compose -f core/src/test/resources/docker-compose.yml up -d + * }</pre> + * + * <p>Tests are skipped automatically if a container is unreachable. + */ + +@Tag("integration") +@EnabledIfSystemProperty(named = "calcite.integration.tests", matches = "true") +class JdbcSchemaDecimalBugTest { + + // --------------------------------------------------------------------------- + // DataSource factory + // --------------------------------------------------------------------------- + + /** + * Minimal {@link DataSource} backed by a JDBC URL. + * Sufficient for integration tests; not for production use. + */ + private static class DriverManagerDataSource implements DataSource { + private final String url; + private final String user; + private final String password; + + DriverManagerDataSource(String url, String user, String password) { + this.url = url; + this.user = user; + this.password = password; + } + + @Override public Connection getConnection() throws SQLException { + return DriverManager.getConnection(url, user, password); + } + + @Override public Connection getConnection(String u, String p) throws SQLException { + return getConnection(); + } + + @Override public PrintWriter getLogWriter() { + return null; + } + + @Override public void setLogWriter(PrintWriter w) { + } + + @Override public void setLoginTimeout(int seconds) { + } + + @Override public int getLoginTimeout() { + return 0; + } + + @Override public Logger getParentLogger() throws SQLFeatureNotSupportedException { + throw new SQLFeatureNotSupportedException("getParentLogger"); + } + + @Override public <T> T unwrap(Class<T> iface) throws SQLException { + throw new SQLException("Not a wrapper for " + iface); + } + + @Override public boolean isWrapperFor(Class<?> iface) { + return false; + } + } + + private static DataSource oracleDataSource() { + return new DriverManagerDataSource( + "jdbc:oracle:thin:@localhost:1521/FREEPDB1", "system", "testpass"); + } + + private static DataSource postgresDataSource() { + return new DriverManagerDataSource( + "jdbc:postgresql://localhost:5432/testdb", "testuser", "testpass"); + } + + private static DataSource mssqlDataSource() { + return new DriverManagerDataSource( + "jdbc:sqlserver://localhost:1433;databaseName=master;encrypt=false", + "sa", "TestPass123!"); + } + + // --------------------------------------------------------------------------- + // Helper methods + // --------------------------------------------------------------------------- + + /** Skips the test if the database is not reachable. */ + private static void assumeReachable(DataSource ds, String dbName) { + try (Connection ignored = ds.getConnection()) { + // connection succeeded - proceed with the test + } catch (SQLException e) { + assumeTrue(false, + dbName + " is not reachable - skipping test. Cause: " + e.getMessage()); + } + } + + /** + * Executes DDL and DML statements to set up a test table. + * Silently ignores the drop statement if the table does not exist. + */ + private static void createTestTable(DataSource ds, String dropSql, + String createSql, String insertSql) throws SQLException { + try (Connection conn = ds.getConnection(); + Statement st = conn.createStatement()) { + try { + st.execute(dropSql); + } catch (SQLException ignored) { + // table did not exist yet - that is fine + } + st.execute(createSql); + st.execute(insertSql); + } + // connection is closed here via try-with-resources + } + + // --------------------------------------------------------------------------- + // Tests - Bug fix: columns without explicit precision must not throw + // --------------------------------------------------------------------------- + + /** + * Oracle {@code NUMBER} without precision (e.g. {@code col NUMBER}) must + * not cause Calcite to throw during schema registration or query execution. + * + * <p>Oracle reports {@code precision=0, scale=-127} for such columns. + */ + @Test void oracleNumberWithoutPrecisionShouldNotThrow() throws Exception { + DataSource ds = oracleDataSource(); + assumeReachable(ds, "Oracle"); + + // Oracle stores unquoted identifiers in UPPERCASE + createTestTable(ds, + "BEGIN EXECUTE IMMEDIATE 'DROP TABLE TEST_NUMBERS';" + + " EXCEPTION WHEN OTHERS THEN NULL; END;", + "CREATE TABLE TEST_NUMBERS (id NUMBER, val NUMBER)", + "INSERT INTO TEST_NUMBERS VALUES (1, 42.5)"); + + try (Connection conn = DriverManager.getConnection("jdbc:calcite:", new Properties())) { + CalciteConnection calciteConn = conn.unwrap(CalciteConnection.class); + SchemaPlus root = calciteConn.getRootSchema(); + root.add("oracle", JdbcSchema.create(root, "oracle", ds, null, "SYSTEM")); + try (Statement st = conn.createStatement(); + ResultSet rs = + st.executeQuery("SELECT * FROM \"oracle\".\"TEST_NUMBERS\"")) { + while (rs.next()) { + //drain Review Comment: ```suggestion // drain ``` -- 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]
