cashmand commented on code in PR #3221: URL: https://github.com/apache/parquet-java/pull/3221#discussion_r2099160474
########## parquet-variant/src/main/java/org/apache/parquet/variant/VariantValueWriter.java: ########## @@ -0,0 +1,310 @@ +/* + * 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.parquet.variant; + +import java.nio.ByteBuffer; +import java.util.HashMap; +import org.apache.parquet.io.api.Binary; +import org.apache.parquet.io.api.RecordConsumer; +import org.apache.parquet.schema.GroupType; +import org.apache.parquet.schema.LogicalTypeAnnotation; +import org.apache.parquet.schema.PrimitiveType; +import org.apache.parquet.schema.Type; + +/** + * Class to write Variant values to a shredded schema. + */ +public class VariantValueWriter { + // We store a reference to the metdata so that we can lazily build metadataMap if needed. + // It could be relatively expensive to build, and if everything shreds perfectly, we won't need it. Review Comment: I'm not referring here to building metadata, but using the existing metadata to build a map from keys to metadata IDs. We need it in the case where we need to build a residual object in `value` if an object contains keys that aren't in `typed_value`. In any case, with the latest version of the reader PR, this happens in the `ImmutableMetadata` constructor, so the code will just defer creating `ImmutableMetadata` until it's actually needed. The truth is, I don't think we strictly need any metadata at all for the writer: it's only necessary because we're using the `VariantObjectBuilder` interface to construct the leftover objects in `value`, and the object builder requires the key String to be provided by name, not ID, so that it can sort objects by key when it finalizes an object. If we assume the input is valid (i.e. already sorted), we could create a short-circuiting version of `VariantObjectBuilder` that operates entirely in terms of the IDs in the variant, in which case we'd never need to reference the metadata. Let me know if you think it's worth extending the object builder with that change. I think it's just a performance benefit, and maybe we don't care that much about that if this is mainly a reference implementation. -- 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]
