scovich commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2178398309
##########
parquet-variant/src/from_json.rs:
##########
@@ -93,38 +132,32 @@ fn append_json(json: &Value, builder: &mut impl
AppendVariantHelper) -> Result<(
Value::String(s) => builder.append_value(s.as_str()),
Value::Array(arr) => {
let mut list_builder = builder.new_list();
- build_list(arr, &mut list_builder)?;
+ for val in arr {
+ append_json(val, &mut list_builder)?;
+ }
list_builder.finish();
}
Value::Object(obj) => {
let mut obj_builder = builder.new_object();
- build_object(obj, &mut obj_builder)?;
- obj_builder.finish();
+ for (key, value) in obj.iter() {
+ let mut field_builder = ObjectFieldBuilder {
+ key,
+ builder: &mut obj_builder,
+ };
+ append_json(value, &mut field_builder)?;
+ }
+ obj_builder.finish().unwrap();
Review Comment:
what makes the `unwrap` safe? And why is it even needed?
```suggestion
obj_builder.finish()?;
```
##########
parquet-variant/src/variant/decimal.rs:
##########
@@ -65,6 +65,12 @@ macro_rules! format_decimal {
}};
}
+// Common trait to classify VariantDecimalXXX types
+pub trait VariantDecimal {
+ fn integer(&self) -> i128;
Review Comment:
One annoyance -- i128 will be _significantly_ more expensive to work with
vs. i32 or i64, because it lacks native hardware support. Division will be
especially painful.
##########
parquet-variant/src/variant/decimal.rs:
##########
@@ -65,6 +65,12 @@ macro_rules! format_decimal {
}};
}
+// Common trait to classify VariantDecimalXXX types
+pub trait VariantDecimal {
+ fn integer(&self) -> i128;
Review Comment:
If we care about the performance aspect, we can replace the method that uses
this trait with a macro that takes advantage of the three decimal types being
syntactically identical.
##########
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:
Tho in theory one should be able to leverage e.g. `Variant::as_decimal4()`
method to finish the round trip?
##########
parquet-variant/src/to_json.rs:
##########
@@ -245,6 +249,27 @@ pub fn variant_to_json_string(variant: &Variant) ->
Result<String, ArrowError> {
.map_err(|e| ArrowError::InvalidArgumentError(format!("UTF-8
conversion error: {e}")))
}
+fn variant_decimal_to_json_value(decimal: &impl VariantDecimal) ->
Result<Value, ArrowError> {
+ let integer = decimal.integer();
+ let scale = decimal.scale();
+ if scale == 0 {
+ return Ok(Value::from(integer));
+ }
+ let divisor = 10_i128.pow(scale);
Review Comment:
nit?
```suggestion
let divisor = i128::pow(10, scale);
```
--
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]
