Copilot commented on code in PR #4441: URL: https://github.com/apache/arrow-adbc/pull/4441#discussion_r3472140001
########## java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/impl/TiedArrowReader.java: ########## @@ -0,0 +1,105 @@ +/* + * 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.driver.jni.impl; + +import java.io.IOException; +import java.util.Map; +import java.util.Set; +import org.apache.arrow.memory.BufferAllocator; +import org.apache.arrow.vector.VectorSchemaRoot; +import org.apache.arrow.vector.dictionary.Dictionary; +import org.apache.arrow.vector.ipc.ArrowReader; +import org.apache.arrow.vector.types.pojo.Schema; +import org.checkerframework.checker.nullness.qual.Nullable; + +/** A proxy {@link ArrowReader} that keeps an associated ADBC resource alive. */ +class TiedArrowReader extends ArrowReader { + private final ArrowReader delegate; + private @Nullable HasChildReferences parent; + + TiedArrowReader( + BufferAllocator allocator, ArrowReader delegate, @Nullable HasChildReferences parent) { + // XXX: ArrowReader being an abstract class and not an interface is a massive wart in arrow-java + // design + super(allocator); + this.delegate = delegate; + this.parent = parent; + if (parent != null) { + parent.getChildReferences().addReference(this); + } + } + + @Override + public void close(boolean closeReadSource) throws IOException { + delegate.close(closeReadSource); + if (parent != null) { + parent.getChildReferences().releaseReference(this); + } + parent = null; + } Review Comment: If delegate.close(closeReadSource) throws, the parent reference is never released, leaving this reader registered in the parent's ChildReferences set. Wrap the delegate close in a try/finally so releaseReference runs even on exceptions. ########## java/driver/jni/src/main/java/org/apache/arrow/adbc/driver/jni/impl/TiedArrowReader.java: ########## @@ -0,0 +1,105 @@ +/* + * 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.driver.jni.impl; + +import java.io.IOException; +import java.util.Map; +import java.util.Set; +import org.apache.arrow.memory.BufferAllocator; +import org.apache.arrow.vector.VectorSchemaRoot; +import org.apache.arrow.vector.dictionary.Dictionary; +import org.apache.arrow.vector.ipc.ArrowReader; +import org.apache.arrow.vector.types.pojo.Schema; +import org.checkerframework.checker.nullness.qual.Nullable; + +/** A proxy {@link ArrowReader} that keeps an associated ADBC resource alive. */ +class TiedArrowReader extends ArrowReader { + private final ArrowReader delegate; + private @Nullable HasChildReferences parent; + + TiedArrowReader( + BufferAllocator allocator, ArrowReader delegate, @Nullable HasChildReferences parent) { + // XXX: ArrowReader being an abstract class and not an interface is a massive wart in arrow-java + // design + super(allocator); + this.delegate = delegate; + this.parent = parent; + if (parent != null) { + parent.getChildReferences().addReference(this); + } + } + + @Override + public void close(boolean closeReadSource) throws IOException { + delegate.close(closeReadSource); + if (parent != null) { + parent.getChildReferences().releaseReference(this); + } + parent = null; + } + + @Override + public void close() throws IOException { + delegate.close(); + if (parent != null) { + parent.getChildReferences().releaseReference(this); + } + parent = null; + } Review Comment: Same issue as close(boolean): if delegate.close() throws, this reader may remain registered in the parent's ChildReferences set. Use try/finally to ensure releaseReference happens reliably. ########## java/driver/jni/src/test/java/org/apache/arrow/adbc/driver/jni/impl/ImplTest.java: ########## @@ -119,4 +120,37 @@ void offsetSlow() throws Exception { assertThat(buf.remaining()).isEqualTo(3); assertThat(JniLoader.INSTANCE.internalGetByteBuffer(buf)).isEqualTo(new byte[] {1, 2, 3}); } + + @Test + void childReferencesCloses() throws Exception { + ChildReferences refs = new ChildReferences(); + var flag = new Closeable(); + refs.addReference(flag); + refs.close(); + assertThat(flag.closed).isTrue(); + } + + @Test + void childReferencesIsWeak() throws Exception { + ChildReferences refs = new ChildReferences(); + var flag = new Closeable(); + refs.addReference(flag); + var ref = new WeakReference<>(flag); + //noinspection UnusedAssignment + flag = null; + System.gc(); + System.gc(); + + assertThat(ref.get()).isNull(); + refs.close(); Review Comment: This test relies on System.gc() and then asserts the object was collected, which is not deterministic across JVMs/CI and can be flaky. Consider converting this to a best-effort GC attempt and skipping the assertion if the JVM doesn't collect within a short timeout (while still closing refs in a finally). ########## java/driver/jni/src/test/java/org/apache/arrow/adbc/driver/jni/JniDriverTest.java: ########## @@ -144,15 +144,17 @@ void querySimple() throws Exception { } } - // Ensure strings with characters that differ between UTF-8 and Java's "modifiefd UTF-8" are + // Ensure strings with characters that differ between UTF-8 and Java's "modified UTF-8" are // properly serialized @Test void queryNonBmpUtf8() throws Exception { try (final BufferAllocator allocator = new RootAllocator()) { JniDriver driver = new JniDriver(allocator); Map<String, Object> parameters = new HashMap<>(); JniDriver.PARAM_DRIVER.set(parameters, "adbc_driver_sqlite"); - String expected = "\uD83D\uDE00"; + String expected = "\uD83D\uDE00"; // U+1f600 Grinning face in big-endian Review Comment: The comment mentions "big-endian", but endianness isn't applicable to UTF-8 encodings. Since the assertion is checking the UTF-8 byte sequence, please update the comment to avoid confusion. -- 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]
