alamb commented on code in PR #7783: URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2173202067
########## parquet-variant/tests/test_json_to_variant.rs: ########## @@ -0,0 +1,380 @@ +// 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. + +//! Manually tests if parsing JSON strings to Variants returns the expected results. Review Comment: Another thing that would be really nice to test is "round tripping". That is, given a Variant 1. Convert the Variant to jSON 2. Convert the JSON back to a New Variant Ensure that Variant and New Variant are the same ########## parquet-variant/src/builder.rs: ########## @@ -680,6 +680,44 @@ impl<'a, 'b> ObjectBuilder<'a, 'b> { } } +/// Trait that abstracts functionality from Variant fconstruction implementations, namely +/// `VariantBuilder`, `ListBuilder` and `ObjectFieldBuilder` to minimize code duplication. +pub(crate) trait AppendVariantHelper { Review Comment: This is very nice -- A more standard Rust name for it might be `VariantBuilderExt` ```suggestion pub(crate) trait VariantBuilderExt { ``` ########## parquet-variant/tests/test_json_to_variant.rs: ########## @@ -0,0 +1,380 @@ +// 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. + +//! Manually tests if parsing JSON strings to Variants returns the expected results. + +use arrow_schema::ArrowError; +use parquet_variant::{ + json_to_variant, variant_to_json_string, VariantBuilder, +}; + +#[test] +fn test_json_to_variant() -> Result<(), ArrowError> { + fn compare_results( + json: &str, + expected_value: &[u8], + expected_metadata: &[u8], + ) -> Result<(), ArrowError> { + let mut variant_builder = VariantBuilder::new(); + json_to_variant(json, &mut variant_builder)?; + let (metadata, value) = variant_builder.finish(); + assert_eq!(&metadata, expected_metadata); Review Comment: I am finding it hard to know if these tests are validating the correct behavior because I can't look at a byte array like `[16u8, 254u8, 105u8` and know "yes, that is the correct binary encoding for a variant integer" Thus, instead of comparing the parsed output to some opaque (at least to me) string of bytes, I recommend comparing it to to a programmatically constructed `Variant` value Perhaps you can follow the model of the to_json tests: https://github.com/apache/arrow-rs/blob/674dc17b2c423be16d0725a6537b0063ac7b1b58/parquet-variant/src/to_json.rs#L600-L609 So instead of ```rust compare_results("null", &[0u8], empty_metadata)?; ``` It would be something more like ```rust Test { json_string: "null" expected_variant: Variant::Null, expected_value: &[0u8], expected_metadata: empty_metadata }.run() ``` ########## parquet-variant/src/from_json.rs: ########## @@ -0,0 +1,175 @@ +// 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. + +//! Module for parsing JSON strings as Variant + +pub use crate::variant::{VariantDecimal4, VariantDecimal8}; +use crate::{AppendVariantHelper, ListBuilder, ObjectBuilder, Variant, VariantBuilder}; +use arrow_schema::ArrowError; +use rust_decimal::prelude::*; +use serde_json::{Number, Value}; + +/// Converts a JSON string to Variant using `variant_builder`. The resulting `value` and `metadata` Review Comment: ```suggestion /// Converts a JSON string to Variant using [`VariantBuilder`]. The resulting `value` and `metadata` ``` -- 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: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
