amoeba commented on code in PR #4466: URL: https://github.com/apache/arrow-adbc/pull/4466#discussion_r3545591486
########## java/core/src/main/java/org/apache/arrow/adbc/core/BulkIngestBuilder.java: ########## @@ -0,0 +1,64 @@ +/* + * 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.arrow.adbc.core; + +import org.apache.arrow.vector.VectorSchemaRoot; +import org.apache.arrow.vector.ipc.ArrowReader; + +/** A fluent builder-style interface for configuring and executing a bulk ingest. */ +public interface BulkIngestBuilder extends AutoCloseable { + BulkIngestBuilder create() throws AdbcException; + + BulkIngestBuilder append() throws AdbcException; + + BulkIngestBuilder replace() throws AdbcException; + + BulkIngestBuilder createAppend() throws AdbcException; Review Comment: These helpers that are specific to each ingest mode are nice but each one is kinda verbish so the user ends up writing, ```java ... .create() .ingest() ``` That is, a method like `.create()` kinda feels like it should take action. Maybe an alternative would be to have a finalize method on the builder like, ```java ... .create() .finish() b.ingest() ``` Just thinking out loud here. ########## java/core/src/main/java/org/apache/arrow/adbc/core/BulkIngestBuilder.java: ########## @@ -0,0 +1,64 @@ +/* + * 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.arrow.adbc.core; + +import org.apache.arrow.vector.VectorSchemaRoot; +import org.apache.arrow.vector.ipc.ArrowReader; + +/** A fluent builder-style interface for configuring and executing a bulk ingest. */ +public interface BulkIngestBuilder extends AutoCloseable { Review Comment: It would be good to add javadoc comments to these interface methods, something like, ```patch diff --git a/java/core/src/main/java/org/apache/arrow/adbc/core/BulkIngestBuilder.java b/java/core/src/main/java/org/apache/arrow/adbc/core/BulkIngestBuilder.java index d8b52f815..6a96f31e9 100644 --- a/java/core/src/main/java/org/apache/arrow/adbc/core/BulkIngestBuilder.java +++ b/java/core/src/main/java/org/apache/arrow/adbc/core/BulkIngestBuilder.java @@ -21,38 +21,128 @@ import org.apache.arrow.vector.ipc.ArrowReader; /** A fluent builder-style interface for configuring and executing a bulk ingest. */ public interface BulkIngestBuilder extends AutoCloseable { + /** + * Set the ingest mode to {@link BulkIngestMode#CREATE}: create the table and insert data, error + * if the table already exists. + */ BulkIngestBuilder create() throws AdbcException; + /** + * Set the ingest mode to {@link BulkIngestMode#APPEND}: append data to an existing table, error + * if the table does not exist or its schema does not match. + */ BulkIngestBuilder append() throws AdbcException; + /** + * Set the ingest mode to {@link BulkIngestMode#REPLACE}: drop the existing table (if any) and + * create a new one, then insert data. + * + * @since ADBC API revision 1.1.0 + */ BulkIngestBuilder replace() throws AdbcException; + /** + * Set the ingest mode to {@link BulkIngestMode#CREATE_APPEND}: insert data, creating the table + * if it does not exist; error if the table exists but the schema does not match. + */ BulkIngestBuilder createAppend() throws AdbcException; + /** + * Set the ingest mode explicitly. + * + * <p>Prefer the convenience methods {@link #create()}, {@link #append()}, {@link #replace()}, and + * {@link #createAppend()} when possible. + * + * @param mode the ingest mode to use + */ BulkIngestBuilder mode(BulkIngestMode mode) throws AdbcException; + /** + * Bind a {@link VectorSchemaRoot} as the data source for ingestion. + * + * <p>The builder will NOT close the root after use. + * + * @param root the data to ingest + */ BulkIngestBuilder bind(VectorSchemaRoot root) throws AdbcException; + /** + * Bind an {@link ArrowReader} as the data source for ingestion. + * + * <p>The underlying statement will close the reader after use. + * + * @param stream the data to ingest + */ BulkIngestBuilder bind(ArrowReader stream) throws AdbcException; + /** + * Set the name of the target table to ingest into. + * + * @param table the table name (required) + */ BulkIngestBuilder targetTable(String table) throws AdbcException; + /** + * Set the schema that contains the target table. + * + * @param schema the schema name, or {@code null} to use the connection default + */ BulkIngestBuilder targetSchema(String schema) throws AdbcException; + /** + * Set the catalog that contains the target table. + * + * @param catalog the catalog name, or {@code null} to use the connection default + */ BulkIngestBuilder targetCatalog(String catalog) throws AdbcException; + /** + * Mark the target table as temporary. + * + * <p>Equivalent to calling {@link #temporary(boolean) temporary(true)}. + */ default BulkIngestBuilder temporary() throws AdbcException { return this.temporary(true); } + /** + * Control whether the target table should be created as a temporary table. + * + * @param isTemporary {@code true} to create a temporary table, {@code false} for a persistent one + */ BulkIngestBuilder temporary(boolean isTemporary) throws AdbcException; + /** + * Set a driver-specific ingest option. + * + * @param option the option to set + */ BulkIngestBuilder option(IngestOption option) throws AdbcException; + /** + * Set a typed driver-specific ingest option. + * + * @param key the option key + * @param value the option value + */ <T> BulkIngestBuilder option(TypedKey<T> key, T value) throws AdbcException; + /** + * Build and return the configured {@link AdbcStatement} without executing it. + * + * <p>The caller is responsible for closing the returned statement. Prefer {@link #ingest()} when + * you do not need to inspect or reuse the statement. + */ AdbcStatement toStatement() throws AdbcException; + /** + * Execute the ingest and return the number of affected rows. + * + * <p>This is a convenience method that calls {@link #toStatement()}, executes it, closes it, and + * returns the {@link AdbcStatement.UpdateResult}. + * + * @return the result of the update, including the number of rows affected + */ default AdbcStatement.UpdateResult ingest() throws AdbcException { try (var statement = toStatement()) { return statement.executeUpdate(); ``` ########## java/core/src/main/java/org/apache/arrow/adbc/core/BulkIngestBuilder.java: ########## @@ -0,0 +1,64 @@ +/* + * 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.arrow.adbc.core; + +import org.apache.arrow.vector.VectorSchemaRoot; +import org.apache.arrow.vector.ipc.ArrowReader; + +/** A fluent builder-style interface for configuring and executing a bulk ingest. */ +public interface BulkIngestBuilder extends AutoCloseable { + BulkIngestBuilder create() throws AdbcException; + + BulkIngestBuilder append() throws AdbcException; + + BulkIngestBuilder replace() throws AdbcException; + + BulkIngestBuilder createAppend() throws AdbcException; + + BulkIngestBuilder mode(BulkIngestMode mode) throws AdbcException; + + BulkIngestBuilder bind(VectorSchemaRoot root) throws AdbcException; + + BulkIngestBuilder bind(ArrowReader stream) throws AdbcException; + + BulkIngestBuilder targetTable(String table) throws AdbcException; + + BulkIngestBuilder targetSchema(String schema) throws AdbcException; + + BulkIngestBuilder targetCatalog(String catalog) throws AdbcException; + + default BulkIngestBuilder temporary() throws AdbcException { + return this.temporary(true); + } + + BulkIngestBuilder temporary(boolean isTemporary) throws AdbcException; + + BulkIngestBuilder option(IngestOption option) throws AdbcException; + + <T> BulkIngestBuilder option(TypedKey<T> key, T value) throws AdbcException; + + AdbcStatement toStatement() throws AdbcException; Review Comment: I think this would be the way for users to cancel their ingest after they run `.ingest()` right? Would be good to document in the javadoc comment that goes here. ########## java/core/src/main/java/org/apache/arrow/adbc/core/BulkIngestBuilder.java: ########## @@ -0,0 +1,64 @@ +/* + * 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.arrow.adbc.core; + +import org.apache.arrow.vector.VectorSchemaRoot; +import org.apache.arrow.vector.ipc.ArrowReader; + +/** A fluent builder-style interface for configuring and executing a bulk ingest. */ +public interface BulkIngestBuilder extends AutoCloseable { + BulkIngestBuilder create() throws AdbcException; + + BulkIngestBuilder append() throws AdbcException; + + BulkIngestBuilder replace() throws AdbcException; + + BulkIngestBuilder createAppend() throws AdbcException; + + BulkIngestBuilder mode(BulkIngestMode mode) throws AdbcException; + + BulkIngestBuilder bind(VectorSchemaRoot root) throws AdbcException; + + BulkIngestBuilder bind(ArrowReader stream) throws AdbcException; + + BulkIngestBuilder targetTable(String table) throws AdbcException; + + BulkIngestBuilder targetSchema(String schema) throws AdbcException; + + BulkIngestBuilder targetCatalog(String catalog) throws AdbcException; + + default BulkIngestBuilder temporary() throws AdbcException { + return this.temporary(true); + } + + BulkIngestBuilder temporary(boolean isTemporary) throws AdbcException; + + BulkIngestBuilder option(IngestOption option) throws AdbcException; + + <T> BulkIngestBuilder option(TypedKey<T> key, T value) throws AdbcException; + + AdbcStatement toStatement() throws AdbcException; + + default AdbcStatement.UpdateResult ingest() throws AdbcException { Review Comment: What happens when the user calls .ingest multiple times on the same builder? It looks like the answer depends on whether they used a root or a reader (with root, you can call multiple times, with reader you can't). What should happen? ########## java/core/src/main/java/org/apache/arrow/adbc/core/BulkIngestBuilderImpl.java: ########## @@ -0,0 +1,187 @@ +/* + * 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.arrow.adbc.core; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import org.apache.arrow.vector.VectorSchemaRoot; +import org.apache.arrow.vector.ipc.ArrowReader; +import org.checkerframework.checker.nullness.qual.Nullable; + +class BulkIngestBuilderImpl implements BulkIngestBuilder { + private final AdbcConnection connection; + BulkIngestMode mode = BulkIngestMode.CREATE; + @Nullable VectorSchemaRoot root; + @Nullable ArrowReader reader; + @Nullable String targetTable, targetSchema, targetCatalog; + boolean temporary = false; + List<IngestOption> ingestOptions = new ArrayList<>(); + List<Map.Entry<TypedKey<?>, Object>> options = new ArrayList<>(); + + BulkIngestBuilderImpl(AdbcConnection connection) { + this.connection = Objects.requireNonNull(connection, "connection must not be null"); + } + + @Override + public BulkIngestBuilder create() { + mode = BulkIngestMode.CREATE; + return this; + } + + @Override + public BulkIngestBuilder append() { + mode = BulkIngestMode.APPEND; + return this; + } + + @Override + public BulkIngestBuilder replace() { + mode = BulkIngestMode.REPLACE; + return this; + } + + @Override + public BulkIngestBuilder createAppend() { + mode = BulkIngestMode.CREATE_APPEND; + return this; + } + + @Override + public BulkIngestBuilder mode(BulkIngestMode mode) { + this.mode = mode; + return this; + } + + @Override + public BulkIngestBuilder bind(VectorSchemaRoot root) throws AdbcException { + this.root = root; + if (this.reader != null) { Review Comment: Interesting. Why check for a reader and auto close? Another approach would be to have .bind fail if it was called before on the same builder. -- 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]
