Copilot commented on code in PR #104: URL: https://github.com/apache/cloudberry-pxf/pull/104#discussion_r3074314306
########## automation/src/main/java/org/apache/cloudberry/pxf/automation/testcontainers/OracleContainer.java: ########## @@ -0,0 +1,61 @@ +package org.apache.cloudberry.pxf.automation.testcontainers; + +/* + * 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. + */ + +import org.testcontainers.containers.Network; +import org.testcontainers.utility.DockerImageName; + +/** + * TestContainers wrapper around Oracle Database Free. + * + * The container joins a shared Docker network with alias `oracle`, + * so PXF inside the Cloudberry container can reach it at `oracle:1521` + * + */ +public class OracleContainer extends org.testcontainers.oracle.OracleContainer { + + private static final String DEFAULT_IMAGE = "gvenzl/oracle-free:23-slim"; Review Comment: Overriding `getJdbcUrl()` with a hard-coded service name is likely to break connectivity for `gvenzl/oracle-free`, which commonly uses `FREEPDB1` as the service name (and the recommended URL form often includes `@//host:port/service`). To avoid test breakage, prefer delegating to `super.getJdbcUrl()` for the host URL, and derive the internal URL from the container’s actual database/service name (or expose a configurable service name that matches the image defaults). ########## automation/src/test/java/org/apache/cloudberry/pxf/automation/features/jdbc/JdbcOracleTest.java: ########## @@ -0,0 +1,205 @@ +package org.apache.cloudberry.pxf.automation.features.jdbc; + +/* + * 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. + */ + +import annotations.WorksWithFDW; +import org.apache.cloudberry.pxf.automation.AbstractTestcontainersTest; +import org.apache.cloudberry.pxf.automation.structures.tables.pxf.ExternalTable; +import org.apache.cloudberry.pxf.automation.structures.tables.utils.TableFactory; +import org.apache.cloudberry.pxf.automation.testcontainers.OracleContainer; +import org.testng.Assert; +import org.testng.annotations.Test; + +import java.math.BigDecimal; +import java.sql.Connection; +import java.sql.Date; +import java.sql.DriverManager; +import java.sql.PreparedStatement; +import java.sql.SQLException; +import java.sql.Statement; +import java.sql.Timestamp; + +@WorksWithFDW +public class JdbcOracleTest extends AbstractTestcontainersTest { + + private static final String ORACLE_DRIVER = "oracle.jdbc.driver.OracleDriver"; Review Comment: The driver class `oracle.jdbc.driver.OracleDriver` is a legacy name; for `ojdbc11` the canonical class is `oracle.jdbc.OracleDriver`. Updating this reduces reliance on deprecated/compat classes and aligns with modern Oracle JDBC usage. ########## automation/src/main/java/org/apache/cloudberry/pxf/automation/testcontainers/OracleContainer.java: ########## @@ -0,0 +1,61 @@ +package org.apache.cloudberry.pxf.automation.testcontainers; + +/* + * 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. + */ + +import org.testcontainers.containers.Network; +import org.testcontainers.utility.DockerImageName; + +/** + * TestContainers wrapper around Oracle Database Free. + * + * The container joins a shared Docker network with alias `oracle`, + * so PXF inside the Cloudberry container can reach it at `oracle:1521` + * + */ +public class OracleContainer extends org.testcontainers.oracle.OracleContainer { + + private static final String DEFAULT_IMAGE = "gvenzl/oracle-free:23-slim"; + private static final String NETWORK_ALIAS = "oracle"; + + public static final int ORACLE_PORT = 1521; + public static final String SERVICE_NAME = "FREE"; Review Comment: Overriding `getJdbcUrl()` with a hard-coded service name is likely to break connectivity for `gvenzl/oracle-free`, which commonly uses `FREEPDB1` as the service name (and the recommended URL form often includes `@//host:port/service`). To avoid test breakage, prefer delegating to `super.getJdbcUrl()` for the host URL, and derive the internal URL from the container’s actual database/service name (or expose a configurable service name that matches the image defaults). ########## server/pxf-jdbc-drivers/build.gradle: ########## @@ -24,4 +24,9 @@ dependencies { // Clickhouse implementation('com.clickhouse:clickhouse-jdbc:0.9.8:all') { transitive = false } + // Oracle + implementation('com.oracle.database.jdbc:ojdbc11:23.3.0.23.09') + implementation('com.oracle.database.jdbc:ucp11:23.3.0.23.09') + implementation('com.oracle.database.nls:orai18n:23.3.0.23.09') Review Comment: Adding Oracle JDBC artifacts to the shipped drivers module can have distribution/licensing implications for downstream packaging. It would be safer to confirm the project’s redistribution policy for Oracle-provided binaries; if redistribution is not allowed/desired, consider keeping these as test-scoped dependencies (automation only) or requiring operators to supply the Oracle driver jars at runtime (as the docs describe). ########## automation/src/main/java/org/apache/cloudberry/pxf/automation/testcontainers/OracleContainer.java: ########## @@ -0,0 +1,61 @@ +package org.apache.cloudberry.pxf.automation.testcontainers; + +/* + * 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. + */ + +import org.testcontainers.containers.Network; +import org.testcontainers.utility.DockerImageName; + +/** + * TestContainers wrapper around Oracle Database Free. + * + * The container joins a shared Docker network with alias `oracle`, + * so PXF inside the Cloudberry container can reach it at `oracle:1521` + * + */ +public class OracleContainer extends org.testcontainers.oracle.OracleContainer { + + private static final String DEFAULT_IMAGE = "gvenzl/oracle-free:23-slim"; + private static final String NETWORK_ALIAS = "oracle"; + + public static final int ORACLE_PORT = 1521; + public static final String SERVICE_NAME = "FREE"; + + public static final String ORACLE_USER = "system"; + public static final String ORACLE_PASSWORD = "pxf-test"; + + public OracleContainer(Network network) { + super(DockerImageName.parse(DEFAULT_IMAGE)); + + withPassword(ORACLE_PASSWORD); + withNetwork(network); + withNetworkAliases(NETWORK_ALIAS); + } + + /** JDBC URL reachable from the host (mapped `ORACLE_PORT`) */ + @Override + public String getJdbcUrl() { + return "jdbc:oracle:thin:@localhost:" + getMappedPort(ORACLE_PORT) + "/" + SERVICE_NAME; + } Review Comment: Overriding `getJdbcUrl()` with a hard-coded service name is likely to break connectivity for `gvenzl/oracle-free`, which commonly uses `FREEPDB1` as the service name (and the recommended URL form often includes `@//host:port/service`). To avoid test breakage, prefer delegating to `super.getJdbcUrl()` for the host URL, and derive the internal URL from the container’s actual database/service name (or expose a configurable service name that matches the image defaults). ########## automation/pom.xml: ########## @@ -258,6 +258,18 @@ <classifier>all</classifier> </dependency> + <dependency> + <groupId>com.oracle.database.jdbc</groupId> + <artifactId>ojdbc11</artifactId> + <version>23.3.0.23.09</version> + </dependency> + + <dependency> + <groupId>org.testcontainers</groupId> + <artifactId>testcontainers-oracle-free</artifactId> + <version>2.0.4</version> + </dependency> Review Comment: Introducing `testcontainers-oracle-free` with an explicit version risks version skew with the rest of the Testcontainers ecosystem already used by automation (potential dependency convergence/runtime incompatibilities). Consider managing Testcontainers versions via a single property or importing a BOM so all Testcontainers artifacts are aligned. ########## docs/content/jdbc_pxf_oracle.html.md.erb: ########## @@ -149,6 +149,18 @@ Perform the following procedure to create a PXF external table that references t FORMAT 'CUSTOM' (formatter='pxfwritable_import'); ``` + OR create the PXF foreign table specifying `jdbc_pxf_fdw` Foreign Data Wrapper. For example: + + ```sql + gpadmin=# CREATE SERVER "oracle" FOREIGN DATA WRAPPER jdbc_pxf_fdw; + gpadmin=# CREATE USER MAPPING FOR CURRENT_USER SERVER "oracle"; + gpadmin=# CREATE FOREIGN TABLE oracle_countries (country_id int, country_name varchar, population float) + SERVER "oracle" + OPTIONS ( + resource 'oracleuser.countries' + ); + ``` Review Comment: These newly added lines are indented with tabs, which can cause Markdown to render them as code blocks or mis-nest them under the preceding list item. Replace the leading tabs with spaces and adjust list/paragraph indentation to match the surrounding formatting so the 'OR create...' section renders correctly. ########## server/pxf-jdbc-drivers/build.gradle: ########## @@ -24,4 +24,9 @@ dependencies { // Clickhouse implementation('com.clickhouse:clickhouse-jdbc:0.9.8:all') { transitive = false } + // Oracle + implementation('com.oracle.database.jdbc:ojdbc11:23.3.0.23.09') + implementation('com.oracle.database.jdbc:ucp11:23.3.0.23.09') + implementation('com.oracle.database.nls:orai18n:23.3.0.23.09') Review Comment: Unlike the ClickHouse dependency just above, these Oracle dependencies don’t disable transitive resolution. If this module is intended to bundle only specific driver jars, consider setting `transitive = false` (or otherwise controlling the resolved set) to avoid pulling in unexpected transitive dependencies that could bloat the distribution or introduce conflicts. ########## docs/content/jdbc_pxf_oracle.html.md.erb: ########## @@ -80,20 +80,20 @@ Perform the following steps to create an Oracle table named `countries` in the s You must create a JDBC server configuration for Oracle, download the Oracle driver JAR file to your system, copy the JAR file to the PXF user configuration directory, synchronize the PXF configuration, and then restart PXF. -This procedure will typically be performed by the Greenplum Database administrator. +This procedure will typically be performed by the Apache Cloudberry administrator. -1. Download the Oracle JDBC driver and place it under `$PXF_BASE/lib` of your Greenplum Database coordinator host. If you [relocated $PXF_BASE](about_pxf_dir.html#movebase), make sure you use the updated location. You can download a Oracle JDBC driver from your preferred download location. The following example places a driver downloaded from Oracle webiste under `$PXF_BASE/lib` of the Greenplum Database coordinator: +1. Download the Oracle JDBC driver and place it under `$PXF_BASE/lib` of your Apache Cloudberry coordinator host. If you [relocated $PXF_BASE](about_pxf_dir.html#movebase), make sure you use the updated location. You can download a Oracle JDBC driver from your preferred download location. The following example places a driver downloaded from Oracle webiste under `$PXF_BASE/lib` of the Apache Cloudberry coordinator: Review Comment: Corrected spelling of 'webiste' to 'website'. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
