lidavidm commented on code in PR #697: URL: https://github.com/apache/arrow-java/pull/697#discussion_r2028374926
########## vector/src/test/java/org/apache/arrow/vector/types/pojo/TestUuidVector.java: ########## @@ -0,0 +1,152 @@ +/* + * 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.vector.types.pojo; + +import java.nio.ByteBuffer; +import java.util.UUID; +import org.apache.arrow.memory.BufferAllocator; +import org.apache.arrow.memory.util.hash.ArrowBufHasher; +import org.apache.arrow.vector.ExtensionTypeVector; +import org.apache.arrow.vector.FieldVector; +import org.apache.arrow.vector.FixedSizeBinaryVector; +import org.apache.arrow.vector.ValueIterableVector; +import org.apache.arrow.vector.ValueVector; +import org.apache.arrow.vector.types.pojo.ArrowType.ExtensionType; +import org.apache.arrow.vector.util.TransferPair; + +public class TestUuidVector { + + public static class UuidType extends ExtensionType { Review Comment: What I meant was, can we just make this a toplevel class? Not a nested class? ########## vector/src/test/java/org/apache/arrow/vector/types/pojo/TestUuidVector.java: ########## @@ -0,0 +1,152 @@ +/* + * 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.vector.types.pojo; + +import java.nio.ByteBuffer; +import java.util.UUID; +import org.apache.arrow.memory.BufferAllocator; +import org.apache.arrow.memory.util.hash.ArrowBufHasher; +import org.apache.arrow.vector.ExtensionTypeVector; +import org.apache.arrow.vector.FieldVector; +import org.apache.arrow.vector.FixedSizeBinaryVector; +import org.apache.arrow.vector.ValueIterableVector; +import org.apache.arrow.vector.ValueVector; +import org.apache.arrow.vector.types.pojo.ArrowType.ExtensionType; +import org.apache.arrow.vector.util.TransferPair; + +public class TestUuidVector { + + public static class UuidType extends ExtensionType { + + @Override + public ArrowType storageType() { + return new ArrowType.FixedSizeBinary(16); + } + + @Override + public String extensionName() { + return "uuid"; + } + + @Override + public boolean extensionEquals(ExtensionType other) { + return other instanceof UuidType; + } + + @Override + public ArrowType deserialize(ArrowType storageType, String serializedData) { + if (!storageType.equals(storageType())) { + throw new UnsupportedOperationException( + "Cannot construct UuidType from underlying type " + storageType); + } + return new UuidType(); + } + + @Override + public String serialize() { + return ""; + } + + @Override + public FieldVector getNewVector(String name, FieldType fieldType, BufferAllocator allocator) { + return new UuidVector(name, allocator, new FixedSizeBinaryVector(name, allocator, 16)); + } + } + + public static class UuidVector extends ExtensionTypeVector<FixedSizeBinaryVector> Review Comment: Same here ########## vector/src/main/codegen/templates/BaseWriter.java: ########## @@ -101,6 +103,13 @@ public interface MapWriter extends ListWriter { MapWriter value(); } + public interface ExtensionWriter extends BaseWriter { + void writeNull(); + <T extends ExtensionHolder> void write(T var1); + void writeExtensionType(Object var1); + <T extends ExtensionTypeWriterFactory> void addExtensionTypeFactory(T var1); Review Comment: While the existing code is short on docstrings, can we add docstrings for new code going forward? In particular it's not clear what `addExtensionTypeFactory` is or how to use it ########## vector/src/main/codegen/templates/BaseWriter.java: ########## @@ -101,6 +103,13 @@ public interface MapWriter extends ListWriter { MapWriter value(); } + public interface ExtensionWriter extends BaseWriter { + void writeNull(); + <T extends ExtensionHolder> void write(T var1); + void writeExtensionType(Object var1); + <T extends ExtensionTypeWriterFactory> void addExtensionTypeFactory(T var1); Review Comment: Are the generic bounds actually useful here? Why can't it just be `ExtensionTypeWriterFactory var1`? ########## vector/src/main/codegen/templates/BaseWriter.java: ########## @@ -101,6 +103,13 @@ public interface MapWriter extends ListWriter { MapWriter value(); } + public interface ExtensionWriter extends BaseWriter { + void writeNull(); + <T extends ExtensionHolder> void write(T var1); Review Comment: Same here - why not just `void write(ExtensionHolder value)`? ########## vector/src/test/java/org/apache/arrow/vector/complex/impl/TestPromotableWriter.java: ########## Review Comment: I don't see any test of using ExtensionHolder? ########## vector/src/main/java/org/apache/arrow/vector/complex/impl/ExtensionTypeWriterFactory.java: ########## @@ -0,0 +1,29 @@ +/* + * 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.vector.complex.impl; + +import org.apache.arrow.vector.ExtensionTypeVector; + +/** + * A factory for {@link ExtensionTypeWriter} instances. The factory allow to configure writer + * implementation for specific ExtensionTypeVector. + * + * @param <T> writer implementation for specific {@link ExtensionTypeVector}. + */ +public interface ExtensionTypeWriterFactory<T extends AbstractFieldWriter> { + T getWriterImpl(ExtensionTypeVector vector); Review Comment: Can we document the methods as well? ########## vector/src/test/java/org/apache/arrow/vector/types/pojo/TestUuidVector.java: ########## @@ -0,0 +1,152 @@ +/* + * 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.vector.types.pojo; + +import java.nio.ByteBuffer; +import java.util.UUID; +import org.apache.arrow.memory.BufferAllocator; +import org.apache.arrow.memory.util.hash.ArrowBufHasher; +import org.apache.arrow.vector.ExtensionTypeVector; +import org.apache.arrow.vector.FieldVector; +import org.apache.arrow.vector.FixedSizeBinaryVector; +import org.apache.arrow.vector.ValueIterableVector; +import org.apache.arrow.vector.ValueVector; +import org.apache.arrow.vector.types.pojo.ArrowType.ExtensionType; +import org.apache.arrow.vector.util.TransferPair; + +public class TestUuidVector { Review Comment: In fact why is this class here at all? All it does is wrap the two inner classes ########## vector/src/main/codegen/templates/BaseWriter.java: ########## @@ -101,6 +103,13 @@ public interface MapWriter extends ListWriter { MapWriter value(); } + public interface ExtensionWriter extends BaseWriter { + void writeNull(); + <T extends ExtensionHolder> void write(T var1); + void writeExtensionType(Object var1); + <T extends ExtensionTypeWriterFactory> void addExtensionTypeFactory(T var1); Review Comment: Can we replace `var1` with meaningful parameter names? ########## vector/src/main/java/org/apache/arrow/vector/complex/impl/ExtensionTypeWriterFactory.java: ########## @@ -0,0 +1,23 @@ +/* + * 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.vector.complex.impl; + +import org.apache.arrow.vector.ExtensionTypeVector; + +public interface ExtensionTypeWriterFactory<T extends AbstractFieldWriter> { Review Comment: IMO abstract implementation classes should not go in generic bounds - it should be the interface type -- 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]
