You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "harshmotw-db (via GitHub)" <gi...@apache.org> on 2025/06/25 21:46:19 UTC
[PR] add json_to_variant [arrow-rs]
harshmotw-db opened a new pull request, #7783:
URL: https://github.com/apache/arrow-rs/pull/7783
# Which issue does this PR close?
- Closes #7425.
# Rationale for this change
Explained in the issue
# What changes are included in this PR?
This PR includes a `json_to_variant` API to parse JSON strings as Variants. `json_to_variant` takes as argument the input JSON string and an object implementing `VariantBufferManager` (explained later), and returns the size of the resulting `metadata` and `value` fields.
`VariantBufferManager` is a new trait that allows the caller to have control over the output buffers where the Variant data would be written. This PR proposes that this trait be expanded deep into the library to give the caller lower-level control. A sample vector-based buffer manager could be provided to callers that are not interested in implementing their own buffer manager.
`VariantBufferManager` requires the implementation of four API's:
1. `borrow_metadata_buffer` which returns a slice where metadata writes should be performed. The slice must preserve data that may have been written so far.
2. `borrow_value_buffer` which returns a slice where value writes should be performed. The slice must preserve data that may have been written so far.
3. `ensure_metadata_buffer_size(size)` which must ensure that subsequent calls to `borrow_metadata_buffer` return a slice of at least `size` bytes.
4. `ensure_value_buffer_size(size)` which must ensure that subsequent calls to `borrow_value_buffer` return a slice of at least `size` bytes.
# Are these changes tested?
Unit Tests, and an example file.
# Are there any user-facing changes?
Yes, the PR introduces the `json_to_variant` API and the `VariantBufferManager` trait.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2168244070
##########
parquet-variant/src/variant_buffer_manager.rs:
##########
@@ -0,0 +1,124 @@
+use arrow_schema::ArrowError;
+
+pub trait VariantBufferManager {
Review Comment:
I think it's reasonable to return the whole slice, and have the Variant builder choose to write data wherever it chooses to. The trait was originally created for [this PR](https://github.com/delta-io/delta-kernel-rs/pull/1030) where bytes are often being shifted to make way for headers etc. Let me know if you disagree or would recommend a different simple alternative.
As for why the methods are separate, I think you are generally correct and they could be combined. In the original PR, I was calling `ensure_value_buffer_size` only occasionally but that was because I was performing size checks in the variant library before calling the method and that check could be moved into the method.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2167899808
##########
parquet-variant/src/variant.rs:
##########
@@ -32,6 +32,8 @@ mod decimal;
mod list;
mod metadata;
mod object;
+use rust_decimal::prelude::*;
+use serde_json::Number;
Review Comment:
I think we should be eventually be planning to avoid all use of serde_json for parsing / serializing.
It is fine to use serde_json for the initial version / getting things functionally working, but it is terribly inefficient compared to more optimized implementations like the tape decoder
Thus can we please try and keep anything serde_json specific out of Variant (like this From impl)?
##########
parquet-variant/src/variant.rs:
##########
@@ -1031,6 +1033,57 @@ impl From<VariantDecimal16> for Variant<'_, '_> {
}
}
+impl TryFrom<&Number> for Variant<'_, '_> {
+ type Error = ArrowError;
+
+ fn try_from(n: &Number) -> Result<Self, Self::Error> {
Review Comment:
follow up sounds like a good plan to me
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2181103506
##########
parquet-variant/tests/test_json_to_variant.rs:
##########
@@ -254,7 +269,7 @@ fn test_json_to_variant_decimal16_max_scale() -> Result<(), ArrowError> {
fn test_json_to_variant_double_precision() -> Result<(), ArrowError> {
JsonToVariantTest {
json: "0.79228162514264337593543950335",
- expected: Variant::Double(0.792_281_625_142_643_3_f64),
+ expected: Variant::Double(0.792_281_625_142_643_4_f64),
Review Comment:
I didn't investigate too much, but I suppose the arbitrary_precision flag was causing slightly different results in parsing doubles. I didn't care because doubles lose precision anyway.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3033131373
I merged up from main and fixed a clippy error to get a clean CI run
Given how long this PR has been outstanding and that it blocks a bunch of other work, I think we should try to merge it in and file follow ons asap. I am giving it another review now
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3025373479
> What I suggest is that we do this incrementally:
>
> 1. Implement basic JSON decoding (this PR) merged without proper decimal handling
>
> 2. Mark any failing tests as `#[ignore]` and file a ticket to fix the json decoding in a follow on PR
>
> 3. Merge this one in
Seems reasonable. And as a quick-follow, it's not _that_ hard to manually parse a JSON numeric string literal to Variant (pathfinding already done). The [grammar](https://www.json.org/json-en.html) is _super_ simple, basically:
```
E := "e" | "E"
number := signed_integer (. unsigned_integer)? ( E signed_integer )?
```
And for parsing, it's basically:
* If contains "e" or "E" => parse as double
* else if contains "." => parse as decimal
* fall back to double if precision is higher than 38
* else parse as int
* fall back to decimal if precision is 20..=38
* fall back to double if precision is higher than 38
```rust
if !strval.contains(['e', 'E']) {
// not obviously float
if !strval.contains(".") {
// not obviously decimal
if let Ok(val) = parse_variant_int(strval) {
return Ok(val);
}
}
if let Ok(val) = parse_variant_decimal(strval) {
return Ok(val);
}
}
parse_variant_float(strval)
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2173203893
##########
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:
(we can do this test as a follow on PR, to be clear)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3033659254
(I recommend a new ticket to track this idea BTW)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3006688750
Here is some parallel art from @zeroshade in the go implementation:
- https://github.com/apache/arrow-go/pull/426
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3024958975
One thing I noticed is that the tests take 21 seconds for me:
I'll try and break them up into smaller chunks to go faster
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3025591614
@alamb @scovich Yeah, I think that makes sense. For now, we could do away with the decimal type in json_to_variant and always use double, and soon replace it when the custom parser is ready.
That being said, the `preserve_order` flag ensures that object keys are inserted into the Variant in the order that they appear. Removing it would result in a behavior difference between Arrow and Spark's parse_json expression. I don't think that makes a logical difference but the binaries would look different as the keys would be sorted in the Arrow library.
Just to clarify, the number parsing was never a hard problem - that just overcomes the rust_decimal dependency, but the fact that `serde_json` doesn't give you strings without the `arbitrary_precision` flag that needs to be done away with. But I guess the tape decoder PR implements all of JSON parsing internally so that's great.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3046635352
@alamb https://github.com/apache/arrow-rs/issues/7883
I am working on something to resolve this.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add json_to_variant [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3006364629
FYI @carpecodeum
This is amazing @harshmotw-db -- I was just thinking about this.
I started the CI checks and will check this PR more carefully tomorrow
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2167823998
##########
parquet-variant/Cargo.toml:
##########
@@ -35,7 +35,10 @@ rust-version = "1.83"
[dependencies]
arrow-schema = { workspace = true }
chrono = { workspace = true }
-serde_json = "1.0"
+# "arbitrary_precision" allows us to manually parse numbers. "preserve_order" does not automatically
+# sort object keys
+serde_json = { version = "1.0", features = ["arbitrary_precision", "preserve_order"] }
Review Comment:
Question tho -- do we need to support both modes of serde_json, given that the user (not arrow-rs) probably decides whether to use that feature flag?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2167835293
##########
parquet-variant/tests/test_json_to_variant.rs:
##########
@@ -0,0 +1,349 @@
+use arrow_schema::ArrowError;
+use parquet_variant::{json_to_variant, VariantBufferManager};
+
+pub struct SampleVariantBufferManager {
Review Comment:
Good catch! I forgot to replace this
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3021296297
The `arbitrary_precision` feature also gets propagated and causes `test_serialize_decimal` and `test_serialize_timestamp` to fail. I tried fixing it for a while with no success. Let me know what you think @alamb
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3033221449
Thanks again @harshmotw-db and @scovich
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2181292589
##########
parquet-variant/src/from_json.rs:
##########
@@ -0,0 +1,149 @@
+// 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
+
+use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder, VariantBuilderExt};
+use arrow_schema::ArrowError;
+use serde_json::{Number, Value};
+
+/// Converts a JSON string to Variant using [`VariantBuilder`]. The resulting `value` and `metadata`
+/// buffers can be extracted using `builder.finish()`
+///
+/// # Arguments
+/// * `json` - The JSON string to parse as Variant.
+/// * `variant_builder` - Object of type `VariantBuilder` used to build the vatiant from the JSON
+/// string
+///
+/// # Returns
+///
+/// * `Ok(())` if successful
+/// * `Err` with error details if the conversion fails
+///
+/// ```rust
+/// # use parquet_variant::{
+/// json_to_variant, variant_to_json, variant_to_json_string, variant_to_json_value, VariantBuilder
+/// };
+///
+/// let mut variant_builder = VariantBuilder::new();
+/// let person_string = "{\"name\":\"Alice\", \"age\":30, ".to_string()
+/// + "\"email\":\"alice@example.com\", \"is_active\": true, \"score\": 95.7,"
+/// + "\"additional_info\": null}";
+/// json_to_variant(&person_string, &mut variant_builder)?;
+///
+/// let (metadata, value) = variant_builder.finish();
+///
+/// let variant = parquet_variant::Variant::try_new(&metadata, &value)?;
+///
+/// let json_result = variant_to_json_string(&variant)?;
+/// let json_value = variant_to_json_value(&variant)?;
+///
+/// let mut buffer = Vec::new();
+/// variant_to_json(&mut buffer, &variant)?;
+/// let buffer_result = String::from_utf8(buffer)?;
+/// assert_eq!(json_result, "{\"additional_info\":null,\"age\":30,".to_string() +
+/// "\"email\":\"alice@example.com\",\"is_active\":true,\"name\":\"Alice\",\"score\":95.7}");
+/// assert_eq!(json_result, buffer_result);
+/// assert_eq!(json_result, serde_json::to_string(&json_value)?);
+/// # Ok::<(), Box<dyn std::error::Error>>(())
+/// ```
+pub fn json_to_variant(json: &str, builder: &mut VariantBuilder) -> Result<(), ArrowError> {
+ let json: Value = serde_json::from_str(json)
+ .map_err(|e| ArrowError::InvalidArgumentError(format!("JSON format error: {e}")))?;
+
+ build_json(&json, builder)?;
+ Ok(())
+}
+
+fn build_json(json: &Value, builder: &mut VariantBuilder) -> Result<(), ArrowError> {
+ append_json(json, builder)?;
+ Ok(())
+}
+
+fn variant_from_number<'a, 'b>(n: &Number) -> Result<Variant<'a, 'b>, ArrowError> {
+ if let Some(i) = n.as_i64() {
+ // Find minimum Integer width to fit
+ if i as i8 as i64 == i {
+ Ok((i as i8).into())
+ } else if i as i16 as i64 == i {
+ Ok((i as i16).into())
+ } else if i as i32 as i64 == i {
+ Ok((i as i32).into())
+ } else {
+ Ok(i.into())
+ }
+ } else {
+ // Todo: Try decimal once we implement custom JSON parsing where we have access to strings
+ // Try double - currently json_to_variant does not produce decimal
+ match n.as_f64() {
+ Some(f) => return Ok(f.into()),
+ None => Err(ArrowError::InvalidArgumentError(format!(
+ "Failed to parse {} as number",
+ n.to_string()
+ ))),
+ }?
+ }
+}
+
+fn append_json(json: &Value, builder: &mut impl VariantBuilderExt) -> Result<(), ArrowError> {
+ match json {
+ Value::Null => builder.append_value(Variant::Null),
+ Value::Bool(b) => builder.append_value(*b),
+ Value::Number(n) => {
+ builder.append_value(variant_from_number(n)?);
+ }
+ Value::String(s) => builder.append_value(s.as_str()),
+ Value::Array(arr) => {
+ let mut list_builder = builder.new_list();
+ for val in arr {
+ append_json(val, &mut list_builder)?;
+ }
+ list_builder.finish();
+ }
+ Value::Object(obj) => {
+ let mut obj_builder = builder.new_object();
+ 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();
+ }
+ };
+ Ok(())
+}
+
+struct ObjectFieldBuilder<'a, 'b, 'c> {
+ key: &'a str,
+ builder: &'b mut ObjectBuilder<'c, 'a>,
Review Comment:
I've changed it to 's, 'o and 'v where 's is the lifetime of the 's is the lifetime of the string, 'o is the lifetime of [`ObjectBuilder`] and `v` is the lifetime of the variant buffers.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3033353107
Here is a follow on PR to break out the json functions:
- https://github.com/apache/arrow-rs/issues/7800
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2178455887
##########
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:
I agree -- when I was playing around with this, I am convinced that a custom decimal parser from `&str` is the way to go. That way we won't have to artifically use i128 -- the code can look at the `&str` instead and figure out how what decimal size it needs
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add json_to_variant [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3006289927
cc @scovich
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2167848191
##########
parquet-variant/src/variant_buffer_manager.rs:
##########
@@ -0,0 +1,124 @@
+use arrow_schema::ArrowError;
+
+pub trait VariantBufferManager {
+ /// Returns the slice where the variant metadata needs to be written to. This method may be
+ /// called several times during the construction of a new `metadata` field in a variant. The
+ /// implementation must make sure that on every call, all the data written to the metadata
+ /// buffer so far are preserved.
+ fn borrow_metadata_buffer(&mut self) -> &mut [u8];
+
+ /// Returns the slice where value needs to be written to. This method may be called several
+ /// times during the construction of a new `value` field in a variant. The implementation must
+ /// make sure that on every call, all the data written to the value buffer so far are preserved.
+ fn borrow_value_buffer(&mut self) -> &mut [u8];
+
+ /// Ensures that the next call to `borrow_metadata_buffer` returns a slice having at least
+ /// `size` bytes. Also ensures that the value metadata written so far are persisted - this means
+ /// that if `borrow_metadata_buffer` is to return a new buffer from the next call onwards, the
+ /// new buffer must have the contents of the old metadata buffer.
+ fn ensure_metadata_buffer_size(&mut self, size: usize) -> Result<(), ArrowError>;
+
+ /// Ensures that the next call to `borrow_value_buffer` returns a slice having at least `size`
+ /// bytes. Also ensures that the value bytes written so far are persisted - this means that
+ /// if `borrow_value_buffer` is to return a new buffer from the next call onwards, the new
+ /// buffer must have the contents of the old value buffer.
+ fn ensure_value_buffer_size(&mut self, size: usize) -> Result<(), ArrowError>;
+
+ fn get_immutable_metadata_buffer(&self) -> &[u8] {
+ unimplemented!("Optional method not implemented in VariantBufferManager")
+ }
+
+ fn get_immutable_value_buffer(&self) -> &[u8] {
+ unimplemented!("Optional method not implemented in VariantBufferManager")
+ }
+}
+
+pub struct SampleBoxBasedVariantBufferManager {
+ pub value_buffer: Box<[u8]>,
+ pub metadata_buffer: Box<[u8]>,
Review Comment:
Yes, I do plan on adding an arrow-based sample buffer manager. I currently do not fully understand arrow though but I believe the trait is simple enough that it could be extended to such use cases. I will do that as a follow up if you agree with the general design.
##########
parquet-variant/src/variant_buffer_manager.rs:
##########
@@ -0,0 +1,124 @@
+use arrow_schema::ArrowError;
+
+pub trait VariantBufferManager {
+ /// Returns the slice where the variant metadata needs to be written to. This method may be
+ /// called several times during the construction of a new `metadata` field in a variant. The
+ /// implementation must make sure that on every call, all the data written to the metadata
+ /// buffer so far are preserved.
+ fn borrow_metadata_buffer(&mut self) -> &mut [u8];
+
+ /// Returns the slice where value needs to be written to. This method may be called several
+ /// times during the construction of a new `value` field in a variant. The implementation must
+ /// make sure that on every call, all the data written to the value buffer so far are preserved.
+ fn borrow_value_buffer(&mut self) -> &mut [u8];
+
+ /// Ensures that the next call to `borrow_metadata_buffer` returns a slice having at least
+ /// `size` bytes. Also ensures that the value metadata written so far are persisted - this means
+ /// that if `borrow_metadata_buffer` is to return a new buffer from the next call onwards, the
+ /// new buffer must have the contents of the old metadata buffer.
+ fn ensure_metadata_buffer_size(&mut self, size: usize) -> Result<(), ArrowError>;
+
+ /// Ensures that the next call to `borrow_value_buffer` returns a slice having at least `size`
+ /// bytes. Also ensures that the value bytes written so far are persisted - this means that
+ /// if `borrow_value_buffer` is to return a new buffer from the next call onwards, the new
+ /// buffer must have the contents of the old value buffer.
+ fn ensure_value_buffer_size(&mut self, size: usize) -> Result<(), ArrowError>;
+
+ fn get_immutable_metadata_buffer(&self) -> &[u8] {
+ unimplemented!("Optional method not implemented in VariantBufferManager")
+ }
+
+ fn get_immutable_value_buffer(&self) -> &[u8] {
+ unimplemented!("Optional method not implemented in VariantBufferManager")
+ }
+}
+
+pub struct SampleBoxBasedVariantBufferManager {
+ pub value_buffer: Box<[u8]>,
+ pub metadata_buffer: Box<[u8]>,
Review Comment:
Yes, I do plan on adding an arrow-compliant sample buffer manager. I currently do not fully understand arrow though but I believe the trait is simple enough that it could be extended to such use cases. I will do that as a follow up if you agree with the general design.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2183512150
##########
parquet-variant/src/lib.rs:
##########
@@ -33,10 +33,12 @@ mod decoder;
mod variant;
// TODO: dead code removal
mod builder;
+mod from_json;
mod to_json;
#[allow(dead_code)]
Review Comment:
This allow was bothering me , so here is a PR to clean it up:
- https://github.com/apache/arrow-rs/pull/7861
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3033652862
> @alamb Do you think it is worth creating an arrow utility that abstracts away string to variant conversion? i.e. a function that takes in a StringArray (array of JSON strings some of which could be null), and returns a StructArray (array of corresponding variants where nulls are propagated directly)? If so, where should such functionality be in the codebase?
Yes I think that is an important function
I suggest calling it a `kernel` and putting it in a `parquet-variant-compute` crate
Perhaps an API like this:
```rust
fn to_variant(array: &Array) -> StructArray {
...
}
```
>
> Also, adding this high-level functionality would also abstract away many of the changes we make to the lower-level library. If we make changes to `VariantBuilder` to require buffers from the caller, we could simply modify this high-level function and users' workflows would stay the same.
This is a great idea
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3024555740
Thanks @harshmotw-db -- I am starting to check this one out in more detail
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2167830142
##########
parquet-variant/src/from_json.rs:
##########
@@ -0,0 +1,97 @@
+use crate::variant_buffer_manager::VariantBufferManager;
+use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder};
+use arrow_schema::ArrowError;
+use serde_json::{Map, Value};
+
+/// Eventually, internal writes should also be performed using VariantBufferManager instead of
+/// ValueBuffer and MetadataBuffer so the caller has control of the memory.
+/// Returns a pair <value_size, metadata_size>
+pub fn json_to_variant<'a, T: VariantBufferManager>(
+ json: &str,
+ variant_buffer_manager: &'a mut T,
+) -> Result<(usize, usize), ArrowError> {
+ let mut builder = VariantBuilder::new();
+ let json: Value = serde_json::from_str(json)
+ .map_err(|e| ArrowError::InvalidArgumentError(format!("JSON format error: {}", e)))?;
+
+ build_json(&json, &mut builder)?;
+ let (metadata, value) = builder.finish();
+ let value_size = value.len();
+ let metadata_size = metadata.len();
+
+ // Write to caller's buffers - Remove this when the library internally writes to the caller's
+ // buffers anyway
+ variant_buffer_manager.ensure_metadata_buffer_size(metadata_size)?;
+ variant_buffer_manager.ensure_value_buffer_size(value_size)?;
+ let caller_metadata_buffer = variant_buffer_manager.borrow_metadata_buffer();
+ caller_metadata_buffer[..metadata_size].copy_from_slice(metadata.as_slice());
+ let caller_value_buffer = variant_buffer_manager.borrow_value_buffer();
+ caller_value_buffer[..value_size].copy_from_slice(value.as_slice());
+ Ok((metadata_size, value_size))
+}
+
+fn build_json(json: &Value, builder: &mut VariantBuilder) -> Result<(), ArrowError> {
+ match json {
+ Value::Null => builder.append_value(Variant::Null),
+ Value::Bool(b) => builder.append_value(*b),
+ Value::Number(n) => {
+ let v: Variant = n.try_into()?;
+ builder.append_value(v)
+ }
+ 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)?;
+ list_builder.finish();
+ }
+ Value::Object(obj) => {
+ let mut obj_builder = builder.new_object();
+ build_object(obj, &mut obj_builder)?;
+ obj_builder.finish();
+ }
+ };
+ Ok(())
+}
+
+fn build_list(arr: &Vec<Value>, builder: &mut ListBuilder) -> Result<(), ArrowError> {
+ for val in arr {
+ match val {
+ Value::Null => builder.append_value(Variant::Null),
+ Value::Bool(b) => builder.append_value(*b),
+ Value::Number(n) => {
+ let v: Variant = n.try_into()?;
+ builder.append_value(v)
+ }
+ 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)?;
+ list_builder.finish()
+ }
+ Value::Object(obj) => {
+ let mut obj_builder = builder.new_object();
+ build_object(obj, &mut obj_builder)?;
+ obj_builder.finish();
+ }
+ }
+ }
+ Ok(())
+}
+
+fn build_object(obj: &Map<String, Value>, builder: &mut ObjectBuilder) -> Result<(), ArrowError> {
+ for (key, value) in obj.iter() {
+ match value {
+ Value::Null => builder.append_value(key, Variant::Null),
+ Value::Bool(b) => builder.append_value(key, *b),
+ Value::Number(n) => {
+ let v: Variant = n.try_into()?;
+ builder.append_value(key, v)
+ }
+ Value::String(s) => builder.append_value(key, s.as_str()),
+ Value::Array(_) | Value::Object(_) => {
+ todo!("Nesting within objects unsupported right now.");
+ }
Review Comment:
Nesting support for objects was merged a few hours ago and has been incorporated into the latest version of this PR
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2168007614
##########
parquet-variant/src/variant.rs:
##########
@@ -32,6 +32,8 @@ mod decimal;
mod list;
mod metadata;
mod object;
+use rust_decimal::prelude::*;
+use serde_json::Number;
Review Comment:
Oh, I'll move much of this code from `variant.rs` to `from_json.rs` if that's what you meant.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2167844189
##########
parquet-variant/Cargo.toml:
##########
@@ -35,7 +35,10 @@ rust-version = "1.83"
[dependencies]
arrow-schema = { workspace = true }
chrono = { workspace = true }
-serde_json = "1.0"
+# "arbitrary_precision" allows us to manually parse numbers. "preserve_order" does not automatically
+# sort object keys
+serde_json = { version = "1.0", features = ["arbitrary_precision", "preserve_order"] }
Review Comment:
It's possible some projects might prioritize performance and might not want the Decimal type at all. I would prefer doing that as a follow up though. If you agree, I'll go ahead and create an issue.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2178211552
##########
parquet-variant/tests/test_json_to_variant.rs:
##########
@@ -166,281 +247,291 @@ fn test_json_to_variant() -> Result<(), ArrowError> {
28,
)?),
}
- .run()?;
- // Double
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_double_precision() -> Result<(), ArrowError> {
JsonToVariantTest {
json: "0.79228162514264337593543950335",
expected: Variant::Double(0.792_281_625_142_643_3_f64),
}
- .run()?;
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_double_scientific_positive() -> Result<(), ArrowError> {
JsonToVariantTest {
json: "15e-1",
expected: Variant::Double(15e-1f64),
}
- .run()?;
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_double_scientific_negative() -> Result<(), ArrowError> {
JsonToVariantTest {
json: "-15e-1",
expected: Variant::Double(-15e-1f64),
}
- .run()?;
- // short strings
- // random short string
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_short_string() -> Result<(), ArrowError> {
JsonToVariantTest {
json: "\"harsh\"",
expected: Variant::ShortString(ShortString::try_new("harsh")?),
}
- .run()?;
- // longest short string
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_short_string_max_length() -> Result<(), ArrowError> {
JsonToVariantTest {
json: &format!("\"{}\"", "a".repeat(63)),
expected: Variant::ShortString(ShortString::try_new(&"a".repeat(63))?),
}
- .run()?;
- // long strings
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_long_string() -> Result<(), ArrowError> {
JsonToVariantTest {
json: &format!("\"{}\"", "a".repeat(64)),
expected: Variant::String(&"a".repeat(64)),
}
- .run()?;
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_very_long_string() -> Result<(), ArrowError> {
JsonToVariantTest {
json: &format!("\"{}\"", "b".repeat(100000)),
expected: Variant::String(&"b".repeat(100000)),
}
- .run()?;
+ .run()
+}
- // arrays
- // u8 offset
- {
- let mut variant_builder = VariantBuilder::new();
- let mut list_builder = variant_builder.new_list();
- list_builder.append_value(Variant::Int8(127));
- list_builder.append_value(Variant::Int16(128));
- list_builder.append_value(Variant::Int32(-32767431));
- list_builder.finish();
- let (metadata, value) = variant_builder.finish();
- let variant = Variant::try_new(&metadata, &value)?;
+#[test]
+fn test_json_to_variant_array_simple() -> Result<(), ArrowError> {
+ let mut variant_builder = VariantBuilder::new();
+ let mut list_builder = variant_builder.new_list();
+ list_builder.append_value(Variant::Int8(127));
+ list_builder.append_value(Variant::Int16(128));
+ list_builder.append_value(Variant::Int32(-32767431));
+ list_builder.finish();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
- JsonToVariantTest {
- json: "[127, 128, -32767431]",
- expected: variant,
- }
- .run()?;
+ JsonToVariantTest {
+ json: "[127, 128, -32767431]",
+ expected: variant,
}
- {
- let mut variant_builder = VariantBuilder::new();
- let mut list_builder = variant_builder.new_list();
- let mut object_builder_inner = list_builder.new_object();
- object_builder_inner.insert("age", Variant::Int8(32));
- object_builder_inner.finish().unwrap();
- list_builder.append_value(Variant::Int16(128));
- list_builder.append_value(Variant::BooleanFalse);
- list_builder.finish();
- let (metadata, value) = variant_builder.finish();
- let variant = Variant::try_new(&metadata, &value)?;
+ .run()
+}
- JsonToVariantTest {
- json: "[{\"age\": 32}, 128, false]",
- expected: variant,
- }
- .run()?;
+#[test]
+fn test_json_to_variant_array_with_object() -> Result<(), ArrowError> {
+ let mut variant_builder = VariantBuilder::new();
+ let mut list_builder = variant_builder.new_list();
+ let mut object_builder_inner = list_builder.new_object();
+ object_builder_inner.insert("age", Variant::Int8(32));
+ object_builder_inner.finish().unwrap();
+ list_builder.append_value(Variant::Int16(128));
+ list_builder.append_value(Variant::BooleanFalse);
+ list_builder.finish();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+
+ JsonToVariantTest {
+ json: "[{\"age\": 32}, 128, false]",
+ expected: variant,
}
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_array_large_u16_offset() -> Result<(), ArrowError> {
// u16 offset - 128 i8's + 1 "true" = 257 bytes
- {
- let mut variant_builder = VariantBuilder::new();
- let mut list_builder = variant_builder.new_list();
- for _ in 0..128 {
- list_builder.append_value(Variant::Int8(1));
- }
- list_builder.append_value(Variant::BooleanTrue);
- list_builder.finish();
- let (metadata, value) = variant_builder.finish();
- let variant = Variant::try_new(&metadata, &value)?;
+ let mut variant_builder = VariantBuilder::new();
+ let mut list_builder = variant_builder.new_list();
+ for _ in 0..128 {
+ list_builder.append_value(Variant::Int8(1));
+ }
+ list_builder.append_value(Variant::BooleanTrue);
+ list_builder.finish();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
- JsonToVariantTest {
- json: &format!("[{} true]", "1, ".repeat(128)),
- expected: variant,
- }
- .run()?;
+ JsonToVariantTest {
+ json: &format!("[{} true]", "1, ".repeat(128)),
+ expected: variant,
}
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_array_nested_large() -> Result<(), ArrowError> {
// verify u24, and large_size
- {
- let mut variant_builder = VariantBuilder::new();
- let mut list_builder = variant_builder.new_list();
- for _ in 0..256 {
- let mut list_builder_inner = list_builder.new_list();
- for _ in 0..255 {
- list_builder_inner.append_value(Variant::Null);
- }
- list_builder_inner.finish();
+ let mut variant_builder = VariantBuilder::new();
+ let mut list_builder = variant_builder.new_list();
+ for _ in 0..256 {
+ let mut list_builder_inner = list_builder.new_list();
+ for _ in 0..255 {
+ list_builder_inner.append_value(Variant::Null);
}
- list_builder.finish();
- let (metadata, value) = variant_builder.finish();
- let variant = Variant::try_new(&metadata, &value)?;
- let intermediate = format!("[{}]", vec!["null"; 255].join(", "));
- let json = format!("[{}]", vec![intermediate; 256].join(", "));
- JsonToVariantTest {
- json: json.as_str(),
- expected: variant,
- }
- .run()?;
+ list_builder_inner.finish();
}
+ list_builder.finish();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+ let intermediate = format!("[{}]", vec!["null"; 255].join(", "));
+ let json = format!("[{}]", vec![intermediate; 256].join(", "));
+ JsonToVariantTest {
+ json: json.as_str(),
+ expected: variant,
+ }
+ .run()
+}
- // objects
- {
- let mut variant_builder = VariantBuilder::new();
- let mut object_builder = variant_builder.new_object();
- object_builder.insert("b", Variant::Int8(2));
- object_builder.insert("a", Variant::Int8(3));
- object_builder.finish().unwrap();
- let (metadata, value) = variant_builder.finish();
- let variant = Variant::try_new(&metadata, &value)?;
- JsonToVariantTest {
- json: "{\"b\": 2, \"a\": 1, \"a\": 3}",
- expected: variant,
- }
- .run()?;
+#[test]
+fn test_json_to_variant_object_simple() -> Result<(), ArrowError> {
+ let mut variant_builder = VariantBuilder::new();
+ let mut object_builder = variant_builder.new_object();
+ object_builder.insert("b", Variant::Int8(2));
+ object_builder.insert("a", Variant::Int8(3));
+ object_builder.finish().unwrap();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+ JsonToVariantTest {
+ json: "{\"b\": 2, \"a\": 1, \"a\": 3}",
+ expected: variant,
}
- {
Review Comment:
this one was copy/paste duplicated (found by claude / copilot)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3033558943
@alamb Do you think it is worth creating an arrow utility that abstracts away string to variant conversion? i.e. a function that takes in a StringArray (array of JSON strings some of which could be null), and returns a StructArray (array of corresponding variants where nulls are propagated directly)? If so, where should such functionality be in the codebase?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2176116436
##########
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:
Round tripping doesn't currently work, mainly due to numerical types. For example, "9.0" is treated as decimal during parsing. When converting to JSON, it becomes "9", which when parsed again is treated as int.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2168269820
##########
parquet-variant/src/variant.rs:
##########
@@ -1031,6 +1033,57 @@ impl From<VariantDecimal16> for Variant<'_, '_> {
}
}
+impl TryFrom<&Number> for Variant<'_, '_> {
+ type Error = ArrowError;
+
+ fn try_from(n: &Number) -> Result<Self, Self::Error> {
Review Comment:
https://github.com/delta-io/delta-kernel-rs/issues/1035
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3025627762
> That being said, the preserve_order flag ensures that object keys are inserted into the Variant in the order that they appear. Removing it would result in a behavior difference between Arrow and Spark's parse_json expression. I don't think that makes a logical difference but the binaries would look different as the keys would be sorted in the Arrow library.
I think this also is a property of the tape decoder -- since it doesn't parse into a tree / hash map structure it will present the fields in the order they appear in the json text
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3029466012
> Just to clarify, the number parsing was never a hard problem... But I guess the tape decoder PR implements all of JSON parsing internally so that's great.
The tape decoder presents JSON numeric values as strings. But they still need to be parsed. Meanwhile, I had a little too much fun playing with parsing code ([playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=1b8244695bf1753c94bc5b18bd6095be)).
It's a lot more efficient than the "try it and see" approach my pathfinding PR took. Probably a little too fancy tho (code size could cause instruction cache and branch prediction problems that actually hurt performance).
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2183459952
##########
parquet-variant/tests/test_json_to_variant.rs:
##########
@@ -0,0 +1,552 @@
+// 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, ShortString, Variant, VariantBuilder,
+ VariantDecimal16, VariantDecimal4, VariantDecimal8,
+};
+
+struct JsonToVariantTest<'a> {
+ json: &'a str,
+ expected: Variant<'a, 'a>,
+}
+
+impl<'a> JsonToVariantTest<'a> {
+ fn run(self) -> Result<(), ArrowError> {
+ let mut variant_builder = VariantBuilder::new();
+ json_to_variant(self.json, &mut variant_builder)?;
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+ assert_eq!(variant, self.expected);
+ Ok(())
+ }
+}
+
+#[test]
+fn test_json_to_variant_null() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "null",
+ expected: Variant::Null,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_boolean_true() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "true",
+ expected: Variant::BooleanTrue,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_boolean_false() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "false",
+ expected: Variant::BooleanFalse,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_int8_positive() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: " 127 ",
+ expected: Variant::Int8(127),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_int8_negative() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: " -128 ",
+ expected: Variant::Int8(-128),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_int16() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: " 27134 ",
+ expected: Variant::Int16(27134),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_int32() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: " -32767431 ",
+ expected: Variant::Int32(-32767431),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_int64() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "92842754201389",
+ expected: Variant::Int64(92842754201389),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal4_basic() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "1.23",
+ expected: Variant::from(VariantDecimal4::try_new(123, 2)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal4_large_positive() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "99999999.9",
+ expected: Variant::from(VariantDecimal4::try_new(999999999, 1)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal4_large_negative() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "-99999999.9",
+ expected: Variant::from(VariantDecimal4::try_new(-999999999, 1)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal4_small_positive() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "0.999999999",
+ expected: Variant::from(VariantDecimal4::try_new(999999999, 9)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal4_tiny_positive() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "0.000000001",
+ expected: Variant::from(VariantDecimal4::try_new(1, 9)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal4_small_negative() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "-0.999999999",
+ expected: Variant::from(VariantDecimal4::try_new(-999999999, 9)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal8_positive() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "999999999.0",
+ expected: Variant::from(VariantDecimal8::try_new(9999999990, 1)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal8_negative() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "-999999999.0",
+ expected: Variant::from(VariantDecimal8::try_new(-9999999990, 1)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal8_high_precision() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "0.999999999999999999",
+ expected: Variant::from(VariantDecimal8::try_new(999999999999999999, 18)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal8_large_with_scale() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "9999999999999999.99",
+ expected: Variant::from(VariantDecimal8::try_new(999999999999999999, 2)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal8_large_negative_with_scale() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "-9999999999999999.99",
+ expected: Variant::from(VariantDecimal8::try_new(-999999999999999999, 2)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal16_large_integer() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "9999999999999999999", // integer larger than i64
+ expected: Variant::from(VariantDecimal16::try_new(9999999999999999999, 0)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal16_high_precision() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "0.9999999999999999999",
+ expected: Variant::from(VariantDecimal16::try_new(9999999999999999999, 19)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal16_max_value() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "79228162514264337593543950335", // 2 ^ 96 - 1
+ expected: Variant::from(VariantDecimal16::try_new(79228162514264337593543950335, 0)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal16_max_scale() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "7.9228162514264337593543950335", // using scale higher than this falls into double
+ // since the max scale is 28.
+ expected: Variant::from(VariantDecimal16::try_new(
+ 79228162514264337593543950335,
+ 28,
+ )?),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_double_precision() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "0.79228162514264337593543950335",
+ expected: Variant::Double(0.792_281_625_142_643_4_f64),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_double_scientific_positive() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "15e-1",
+ expected: Variant::Double(15e-1f64),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_double_scientific_negative() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "-15e-1",
+ expected: Variant::Double(-15e-1f64),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_short_string() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "\"harsh\"",
+ expected: Variant::ShortString(ShortString::try_new("harsh")?),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_short_string_max_length() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: &format!("\"{}\"", "a".repeat(63)),
+ expected: Variant::ShortString(ShortString::try_new(&"a".repeat(63))?),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_long_string() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: &format!("\"{}\"", "a".repeat(64)),
+ expected: Variant::String(&"a".repeat(64)),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_very_long_string() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: &format!("\"{}\"", "b".repeat(100000)),
+ expected: Variant::String(&"b".repeat(100000)),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_array_simple() -> Result<(), ArrowError> {
+ let mut variant_builder = VariantBuilder::new();
+ let mut list_builder = variant_builder.new_list();
+ list_builder.append_value(Variant::Int8(127));
+ list_builder.append_value(Variant::Int16(128));
+ list_builder.append_value(Variant::Int32(-32767431));
+ list_builder.finish();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+
+ JsonToVariantTest {
+ json: "[127, 128, -32767431]",
+ expected: variant,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_array_with_object() -> Result<(), ArrowError> {
+ let mut variant_builder = VariantBuilder::new();
+ let mut list_builder = variant_builder.new_list();
+ let mut object_builder_inner = list_builder.new_object();
+ object_builder_inner.insert("age", Variant::Int8(32));
+ object_builder_inner.finish().unwrap();
+ list_builder.append_value(Variant::Int16(128));
+ list_builder.append_value(Variant::BooleanFalse);
+ list_builder.finish();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+
+ JsonToVariantTest {
+ json: "[{\"age\": 32}, 128, false]",
+ expected: variant,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_array_large_u16_offset() -> Result<(), ArrowError> {
+ // u16 offset - 128 i8's + 1 "true" = 257 bytes
+ let mut variant_builder = VariantBuilder::new();
+ let mut list_builder = variant_builder.new_list();
+ for _ in 0..128 {
+ list_builder.append_value(Variant::Int8(1));
+ }
+ list_builder.append_value(Variant::BooleanTrue);
+ list_builder.finish();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+
+ JsonToVariantTest {
+ json: &format!("[{} true]", "1, ".repeat(128)),
+ expected: variant,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_array_nested_large() -> Result<(), ArrowError> {
+ // verify u24, and large_size
+ let mut variant_builder = VariantBuilder::new();
+ let mut list_builder = variant_builder.new_list();
+ for _ in 0..256 {
+ let mut list_builder_inner = list_builder.new_list();
+ for _ in 0..255 {
+ list_builder_inner.append_value(Variant::Null);
+ }
+ list_builder_inner.finish();
+ }
+ list_builder.finish();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+ let intermediate = format!("[{}]", vec!["null"; 255].join(", "));
+ let json = format!("[{}]", vec![intermediate; 256].join(", "));
+ JsonToVariantTest {
+ json: json.as_str(),
+ expected: variant,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_object_simple() -> Result<(), ArrowError> {
+ let mut variant_builder = VariantBuilder::new();
+ let mut object_builder = variant_builder.new_object();
+ object_builder.insert("a", Variant::Int8(3));
+ object_builder.insert("b", Variant::Int8(2));
+ object_builder.finish().unwrap();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+ JsonToVariantTest {
+ json: "{\"b\": 2, \"a\": 1, \"a\": 3}",
+ expected: variant,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_object_complex() -> Result<(), ArrowError> {
+ let mut variant_builder = VariantBuilder::new();
+ let mut object_builder = variant_builder.new_object();
+ let mut inner_list_builder = object_builder.new_list("booleans");
+ inner_list_builder.append_value(Variant::BooleanTrue);
+ inner_list_builder.append_value(Variant::BooleanFalse);
+ inner_list_builder.finish();
+ object_builder.insert("null", Variant::Null);
+ let mut inner_list_builder = object_builder.new_list("numbers");
+ inner_list_builder.append_value(Variant::Int8(4));
+ inner_list_builder.append_value(Variant::Double(-3e0));
+ inner_list_builder.append_value(Variant::Double(1001e-3));
+ inner_list_builder.finish();
+ object_builder.finish().unwrap();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+ JsonToVariantTest {
+ json: "{\"numbers\": [4, -3e0, 1001e-3], \"null\": null, \"booleans\": [true, false]}",
+ expected: variant,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_object_very_large() -> Result<(), ArrowError> {
Review Comment:
this single test currently takes over 22 seconds to complete
<img width="608" alt="Screenshot 2025-07-03 at 2 11 16 PM" src="https://github.com/user-attachments/assets/4dfde376-8efd-4d67-a632-27e1f6f7f5dd" />
I will see if I can make it faster
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2183494895
##########
parquet-variant/tests/test_json_to_variant.rs:
##########
@@ -0,0 +1,552 @@
+// 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, ShortString, Variant, VariantBuilder,
+ VariantDecimal16, VariantDecimal4, VariantDecimal8,
+};
+
+struct JsonToVariantTest<'a> {
+ json: &'a str,
+ expected: Variant<'a, 'a>,
+}
+
+impl<'a> JsonToVariantTest<'a> {
+ fn run(self) -> Result<(), ArrowError> {
+ let mut variant_builder = VariantBuilder::new();
+ json_to_variant(self.json, &mut variant_builder)?;
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+ assert_eq!(variant, self.expected);
+ Ok(())
+ }
+}
+
+#[test]
+fn test_json_to_variant_null() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "null",
+ expected: Variant::Null,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_boolean_true() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "true",
+ expected: Variant::BooleanTrue,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_boolean_false() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "false",
+ expected: Variant::BooleanFalse,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_int8_positive() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: " 127 ",
+ expected: Variant::Int8(127),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_int8_negative() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: " -128 ",
+ expected: Variant::Int8(-128),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_int16() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: " 27134 ",
+ expected: Variant::Int16(27134),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_int32() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: " -32767431 ",
+ expected: Variant::Int32(-32767431),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_int64() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "92842754201389",
+ expected: Variant::Int64(92842754201389),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal4_basic() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "1.23",
+ expected: Variant::from(VariantDecimal4::try_new(123, 2)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal4_large_positive() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "99999999.9",
+ expected: Variant::from(VariantDecimal4::try_new(999999999, 1)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal4_large_negative() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "-99999999.9",
+ expected: Variant::from(VariantDecimal4::try_new(-999999999, 1)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal4_small_positive() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "0.999999999",
+ expected: Variant::from(VariantDecimal4::try_new(999999999, 9)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal4_tiny_positive() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "0.000000001",
+ expected: Variant::from(VariantDecimal4::try_new(1, 9)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal4_small_negative() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "-0.999999999",
+ expected: Variant::from(VariantDecimal4::try_new(-999999999, 9)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal8_positive() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "999999999.0",
+ expected: Variant::from(VariantDecimal8::try_new(9999999990, 1)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal8_negative() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "-999999999.0",
+ expected: Variant::from(VariantDecimal8::try_new(-9999999990, 1)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal8_high_precision() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "0.999999999999999999",
+ expected: Variant::from(VariantDecimal8::try_new(999999999999999999, 18)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal8_large_with_scale() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "9999999999999999.99",
+ expected: Variant::from(VariantDecimal8::try_new(999999999999999999, 2)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal8_large_negative_with_scale() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "-9999999999999999.99",
+ expected: Variant::from(VariantDecimal8::try_new(-999999999999999999, 2)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal16_large_integer() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "9999999999999999999", // integer larger than i64
+ expected: Variant::from(VariantDecimal16::try_new(9999999999999999999, 0)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal16_high_precision() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "0.9999999999999999999",
+ expected: Variant::from(VariantDecimal16::try_new(9999999999999999999, 19)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal16_max_value() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "79228162514264337593543950335", // 2 ^ 96 - 1
+ expected: Variant::from(VariantDecimal16::try_new(79228162514264337593543950335, 0)?),
+ }
+ .run()
+}
+
+#[ignore]
+#[test]
+fn test_json_to_variant_decimal16_max_scale() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "7.9228162514264337593543950335", // using scale higher than this falls into double
+ // since the max scale is 28.
+ expected: Variant::from(VariantDecimal16::try_new(
+ 79228162514264337593543950335,
+ 28,
+ )?),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_double_precision() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "0.79228162514264337593543950335",
+ expected: Variant::Double(0.792_281_625_142_643_4_f64),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_double_scientific_positive() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "15e-1",
+ expected: Variant::Double(15e-1f64),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_double_scientific_negative() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "-15e-1",
+ expected: Variant::Double(-15e-1f64),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_short_string() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: "\"harsh\"",
+ expected: Variant::ShortString(ShortString::try_new("harsh")?),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_short_string_max_length() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: &format!("\"{}\"", "a".repeat(63)),
+ expected: Variant::ShortString(ShortString::try_new(&"a".repeat(63))?),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_long_string() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: &format!("\"{}\"", "a".repeat(64)),
+ expected: Variant::String(&"a".repeat(64)),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_very_long_string() -> Result<(), ArrowError> {
+ JsonToVariantTest {
+ json: &format!("\"{}\"", "b".repeat(100000)),
+ expected: Variant::String(&"b".repeat(100000)),
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_array_simple() -> Result<(), ArrowError> {
+ let mut variant_builder = VariantBuilder::new();
+ let mut list_builder = variant_builder.new_list();
+ list_builder.append_value(Variant::Int8(127));
+ list_builder.append_value(Variant::Int16(128));
+ list_builder.append_value(Variant::Int32(-32767431));
+ list_builder.finish();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+
+ JsonToVariantTest {
+ json: "[127, 128, -32767431]",
+ expected: variant,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_array_with_object() -> Result<(), ArrowError> {
+ let mut variant_builder = VariantBuilder::new();
+ let mut list_builder = variant_builder.new_list();
+ let mut object_builder_inner = list_builder.new_object();
+ object_builder_inner.insert("age", Variant::Int8(32));
+ object_builder_inner.finish().unwrap();
+ list_builder.append_value(Variant::Int16(128));
+ list_builder.append_value(Variant::BooleanFalse);
+ list_builder.finish();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+
+ JsonToVariantTest {
+ json: "[{\"age\": 32}, 128, false]",
+ expected: variant,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_array_large_u16_offset() -> Result<(), ArrowError> {
+ // u16 offset - 128 i8's + 1 "true" = 257 bytes
+ let mut variant_builder = VariantBuilder::new();
+ let mut list_builder = variant_builder.new_list();
+ for _ in 0..128 {
+ list_builder.append_value(Variant::Int8(1));
+ }
+ list_builder.append_value(Variant::BooleanTrue);
+ list_builder.finish();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+
+ JsonToVariantTest {
+ json: &format!("[{} true]", "1, ".repeat(128)),
+ expected: variant,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_array_nested_large() -> Result<(), ArrowError> {
+ // verify u24, and large_size
+ let mut variant_builder = VariantBuilder::new();
+ let mut list_builder = variant_builder.new_list();
+ for _ in 0..256 {
+ let mut list_builder_inner = list_builder.new_list();
+ for _ in 0..255 {
+ list_builder_inner.append_value(Variant::Null);
+ }
+ list_builder_inner.finish();
+ }
+ list_builder.finish();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+ let intermediate = format!("[{}]", vec!["null"; 255].join(", "));
+ let json = format!("[{}]", vec![intermediate; 256].join(", "));
+ JsonToVariantTest {
+ json: json.as_str(),
+ expected: variant,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_object_simple() -> Result<(), ArrowError> {
+ let mut variant_builder = VariantBuilder::new();
+ let mut object_builder = variant_builder.new_object();
+ object_builder.insert("a", Variant::Int8(3));
+ object_builder.insert("b", Variant::Int8(2));
+ object_builder.finish().unwrap();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+ JsonToVariantTest {
+ json: "{\"b\": 2, \"a\": 1, \"a\": 3}",
+ expected: variant,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_object_complex() -> Result<(), ArrowError> {
+ let mut variant_builder = VariantBuilder::new();
+ let mut object_builder = variant_builder.new_object();
+ let mut inner_list_builder = object_builder.new_list("booleans");
+ inner_list_builder.append_value(Variant::BooleanTrue);
+ inner_list_builder.append_value(Variant::BooleanFalse);
+ inner_list_builder.finish();
+ object_builder.insert("null", Variant::Null);
+ let mut inner_list_builder = object_builder.new_list("numbers");
+ inner_list_builder.append_value(Variant::Int8(4));
+ inner_list_builder.append_value(Variant::Double(-3e0));
+ inner_list_builder.append_value(Variant::Double(1001e-3));
+ inner_list_builder.finish();
+ object_builder.finish().unwrap();
+ let (metadata, value) = variant_builder.finish();
+ let variant = Variant::try_new(&metadata, &value)?;
+ JsonToVariantTest {
+ json: "{\"numbers\": [4, -3e0, 1001e-3], \"null\": null, \"booleans\": [true, false]}",
+ expected: variant,
+ }
+ .run()
+}
+
+#[test]
+fn test_json_to_variant_object_very_large() -> Result<(), ArrowError> {
Review Comment:
I wasn't able to make it faster with a small effort, but I did briefly look at some profiling and it is spending a very large amount of time validating the offsets in the variant
That is likely something we can improve on over time
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3033196169
@harshmotw-db can you please file a follow on ticket that describes the remaining work to support decimal in your mind?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2167845968
##########
parquet-variant/src/variant.rs:
##########
@@ -1031,6 +1033,57 @@ impl From<VariantDecimal16> for Variant<'_, '_> {
}
}
+impl TryFrom<&Number> for Variant<'_, '_> {
+ type Error = ArrowError;
+
+ fn try_from(n: &Number) -> Result<Self, Self::Error> {
Review Comment:
Thanks for the code pointer! Do you think we could implement custom decimal parsing as a follow up? I think 2^96 is a good bound for now even though the Variant spec allows for a larger width (about 2^128).
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2167830622
##########
parquet-variant/src/from_json.rs:
##########
@@ -0,0 +1,97 @@
+use crate::variant_buffer_manager::VariantBufferManager;
+use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder};
+use arrow_schema::ArrowError;
+use serde_json::{Map, Value};
+
+/// Eventually, internal writes should also be performed using VariantBufferManager instead of
+/// ValueBuffer and MetadataBuffer so the caller has control of the memory.
+/// Returns a pair <value_size, metadata_size>
+pub fn json_to_variant<'a, T: VariantBufferManager>(
+ json: &str,
+ variant_buffer_manager: &'a mut T,
+) -> Result<(usize, usize), ArrowError> {
+ let mut builder = VariantBuilder::new();
+ let json: Value = serde_json::from_str(json)
+ .map_err(|e| ArrowError::InvalidArgumentError(format!("JSON format error: {}", e)))?;
+
+ build_json(&json, &mut builder)?;
+ let (metadata, value) = builder.finish();
+ let value_size = value.len();
+ let metadata_size = metadata.len();
+
+ // Write to caller's buffers - Remove this when the library internally writes to the caller's
+ // buffers anyway
+ variant_buffer_manager.ensure_metadata_buffer_size(metadata_size)?;
+ variant_buffer_manager.ensure_value_buffer_size(value_size)?;
+ let caller_metadata_buffer = variant_buffer_manager.borrow_metadata_buffer();
+ caller_metadata_buffer[..metadata_size].copy_from_slice(metadata.as_slice());
+ let caller_value_buffer = variant_buffer_manager.borrow_value_buffer();
+ caller_value_buffer[..value_size].copy_from_slice(value.as_slice());
+ Ok((metadata_size, value_size))
+}
+
+fn build_json(json: &Value, builder: &mut VariantBuilder) -> Result<(), ArrowError> {
+ match json {
+ Value::Null => builder.append_value(Variant::Null),
+ Value::Bool(b) => builder.append_value(*b),
+ Value::Number(n) => {
+ let v: Variant = n.try_into()?;
+ builder.append_value(v)
+ }
+ 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)?;
+ list_builder.finish();
+ }
+ Value::Object(obj) => {
+ let mut obj_builder = builder.new_object();
+ build_object(obj, &mut obj_builder)?;
+ obj_builder.finish();
+ }
+ };
+ Ok(())
+}
+
+fn build_list(arr: &Vec<Value>, builder: &mut ListBuilder) -> Result<(), ArrowError> {
+ for val in arr {
+ match val {
+ Value::Null => builder.append_value(Variant::Null),
+ Value::Bool(b) => builder.append_value(*b),
+ Value::Number(n) => {
+ let v: Variant = n.try_into()?;
+ builder.append_value(v)
+ }
+ 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)?;
+ list_builder.finish()
+ }
+ Value::Object(obj) => {
+ let mut obj_builder = builder.new_object();
+ build_object(obj, &mut obj_builder)?;
+ obj_builder.finish();
+ }
+ }
+ }
+ Ok(())
+}
+
+fn build_object(obj: &Map<String, Value>, builder: &mut ObjectBuilder) -> Result<(), ArrowError> {
+ for (key, value) in obj.iter() {
+ match value {
+ Value::Null => builder.append_value(key, Variant::Null),
+ Value::Bool(b) => builder.append_value(key, *b),
+ Value::Number(n) => {
+ let v: Variant = n.try_into()?;
+ builder.append_value(key, v)
+ }
+ Value::String(s) => builder.append_value(key, s.as_str()),
+ Value::Array(_) | Value::Object(_) => {
+ todo!("Nesting within objects unsupported right now.");
+ }
Review Comment:
This is the commit: https://github.com/harshmotw-db/arrow-rs/commit/7d3a25ad7f57854c4ea14aa1f9c63b5717ed8ac9
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3025152962
> The `arbitrary_precision` feature also gets propagated and causes `test_serialize_decimal` and `test_serialize_timestamp` to fail. I tried fixing it for a while with no success. Let me know what you think @alamb
I played around with this for a while and I concluded that if we use the serde_json parser we will potentially lose precision as internally it converts the JSON strings to `f64`. I suspect this is what lead you down the `arbitrary_precision` hole
I think the only way we'll be able to properly handle / round trip values in JSON will be to use our own parser rather than than the serde_json one. Conveniently we already have such a think in arrow-json (the tape decoder that @scovich has played around with https://github.com/apache/arrow-rs/pull/7403)
What I suggest is that we do this incrementally:
1. Implement basic JSON decoding (this PR) merged without proper decimal handling
2. Mark any failing tests as `#[ignore]` and file a ticket to fix the json decoding in a follow on PR
3. Merge this one in
So that would mean in this PR, remove the added dependencies on serde_json and Rust decimal and do the best we can to convert from serde_json::Value --> Variant. A bunch of tests will fail that we can mark as `#[ignore]` and leave a reference to the ticket to fix.
While maybe not as satisfying as getting it all working at first, I think it will unblock other development
What I am thinking we can do is
1. Merge this PR
2. Move all the JSON handling into `parquet-variant-json`
3. Potentially use / reuse the tape decoder API from `parquet-variant-json`
Does this make sense @harshmotw-db ?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3025388245
> Seems reasonable. And as a quick-follow, it's not _that_ hard to manually parse a JSON numeric string literal to Variant (pathfinding already done). The [grammar](https://www.json.org/json-en.html) is _super_ simple, basically:
For sure
Also, we have some prior art in these crates here:
https://github.com/apache/arrow-rs/blob/5505113d9745aba2cb46df2fd11a4b3d9672d5d2/arrow-cast/src/parse.rs#L854-L853
those require knowing the precison up front, but we could perhaps adapt the code
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3029377499
@alamb I have now removed all dependency on the custom serde_json and ignored all tests. See if the PR can be merged now.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2181080710
##########
parquet-variant/src/from_json.rs:
##########
@@ -0,0 +1,149 @@
+// 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
+
+use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder, VariantBuilderExt};
+use arrow_schema::ArrowError;
+use serde_json::{Number, Value};
+
+/// Converts a JSON string to Variant using [`VariantBuilder`]. The resulting `value` and `metadata`
+/// buffers can be extracted using `builder.finish()`
+///
+/// # Arguments
+/// * `json` - The JSON string to parse as Variant.
+/// * `variant_builder` - Object of type `VariantBuilder` used to build the vatiant from the JSON
+/// string
+///
+/// # Returns
+///
+/// * `Ok(())` if successful
+/// * `Err` with error details if the conversion fails
+///
+/// ```rust
+/// # use parquet_variant::{
+/// json_to_variant, variant_to_json, variant_to_json_string, variant_to_json_value, VariantBuilder
+/// };
+///
+/// let mut variant_builder = VariantBuilder::new();
+/// let person_string = "{\"name\":\"Alice\", \"age\":30, ".to_string()
+/// + "\"email\":\"alice@example.com\", \"is_active\": true, \"score\": 95.7,"
+/// + "\"additional_info\": null}";
+/// json_to_variant(&person_string, &mut variant_builder)?;
+///
+/// let (metadata, value) = variant_builder.finish();
+///
+/// let variant = parquet_variant::Variant::try_new(&metadata, &value)?;
+///
+/// let json_result = variant_to_json_string(&variant)?;
+/// let json_value = variant_to_json_value(&variant)?;
+///
+/// let mut buffer = Vec::new();
+/// variant_to_json(&mut buffer, &variant)?;
+/// let buffer_result = String::from_utf8(buffer)?;
+/// assert_eq!(json_result, "{\"additional_info\":null,\"age\":30,".to_string() +
+/// "\"email\":\"alice@example.com\",\"is_active\":true,\"name\":\"Alice\",\"score\":95.7}");
+/// assert_eq!(json_result, buffer_result);
+/// assert_eq!(json_result, serde_json::to_string(&json_value)?);
+/// # Ok::<(), Box<dyn std::error::Error>>(())
+/// ```
+pub fn json_to_variant(json: &str, builder: &mut VariantBuilder) -> Result<(), ArrowError> {
+ let json: Value = serde_json::from_str(json)
+ .map_err(|e| ArrowError::InvalidArgumentError(format!("JSON format error: {e}")))?;
+
+ build_json(&json, builder)?;
+ Ok(())
+}
+
+fn build_json(json: &Value, builder: &mut VariantBuilder) -> Result<(), ArrowError> {
+ append_json(json, builder)?;
+ Ok(())
+}
+
+fn variant_from_number<'a, 'b>(n: &Number) -> Result<Variant<'a, 'b>, ArrowError> {
+ if let Some(i) = n.as_i64() {
+ // Find minimum Integer width to fit
+ if i as i8 as i64 == i {
+ Ok((i as i8).into())
+ } else if i as i16 as i64 == i {
+ Ok((i as i16).into())
+ } else if i as i32 as i64 == i {
+ Ok((i as i32).into())
+ } else {
+ Ok(i.into())
+ }
+ } else {
+ // Todo: Try decimal once we implement custom JSON parsing where we have access to strings
+ // Try double - currently json_to_variant does not produce decimal
+ match n.as_f64() {
+ Some(f) => return Ok(f.into()),
+ None => Err(ArrowError::InvalidArgumentError(format!(
+ "Failed to parse {} as number",
+ n.to_string()
+ ))),
+ }?
+ }
+}
+
+fn append_json(json: &Value, builder: &mut impl VariantBuilderExt) -> Result<(), ArrowError> {
+ match json {
+ Value::Null => builder.append_value(Variant::Null),
+ Value::Bool(b) => builder.append_value(*b),
+ Value::Number(n) => {
+ builder.append_value(variant_from_number(n)?);
+ }
+ Value::String(s) => builder.append_value(s.as_str()),
+ Value::Array(arr) => {
+ let mut list_builder = builder.new_list();
+ for val in arr {
+ append_json(val, &mut list_builder)?;
+ }
+ list_builder.finish();
+ }
+ Value::Object(obj) => {
+ let mut obj_builder = builder.new_object();
+ 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();
+ }
+ };
+ Ok(())
+}
+
+struct ObjectFieldBuilder<'a, 'b, 'c> {
+ key: &'a str,
+ builder: &'b mut ObjectBuilder<'c, 'a>,
+}
+
+impl VariantBuilderExt for ObjectFieldBuilder<'_, '_, '_> {
+ fn append_value<'m, 'd, T: Into<Variant<'m, 'd>>>(&mut self, value: T) {
Review Comment:
This seems like a lot of lifetimes and generics?
```suggestion
impl<'m, 'v> VariantBuilderExt for ObjectFieldBuilder<'m, 'v, '_> {
fn append_value(&mut self, value: impl Into<Variant<'m, 'v>>) {
```
##########
parquet-variant/src/from_json.rs:
##########
@@ -0,0 +1,149 @@
+// 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
+
+use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder, VariantBuilderExt};
+use arrow_schema::ArrowError;
+use serde_json::{Number, Value};
+
+/// Converts a JSON string to Variant using [`VariantBuilder`]. The resulting `value` and `metadata`
+/// buffers can be extracted using `builder.finish()`
+///
+/// # Arguments
+/// * `json` - The JSON string to parse as Variant.
+/// * `variant_builder` - Object of type `VariantBuilder` used to build the vatiant from the JSON
+/// string
+///
+/// # Returns
+///
+/// * `Ok(())` if successful
+/// * `Err` with error details if the conversion fails
+///
+/// ```rust
+/// # use parquet_variant::{
+/// json_to_variant, variant_to_json, variant_to_json_string, variant_to_json_value, VariantBuilder
+/// };
+///
+/// let mut variant_builder = VariantBuilder::new();
+/// let person_string = "{\"name\":\"Alice\", \"age\":30, ".to_string()
+/// + "\"email\":\"alice@example.com\", \"is_active\": true, \"score\": 95.7,"
+/// + "\"additional_info\": null}";
+/// json_to_variant(&person_string, &mut variant_builder)?;
+///
+/// let (metadata, value) = variant_builder.finish();
+///
+/// let variant = parquet_variant::Variant::try_new(&metadata, &value)?;
+///
+/// let json_result = variant_to_json_string(&variant)?;
+/// let json_value = variant_to_json_value(&variant)?;
+///
+/// let mut buffer = Vec::new();
+/// variant_to_json(&mut buffer, &variant)?;
+/// let buffer_result = String::from_utf8(buffer)?;
+/// assert_eq!(json_result, "{\"additional_info\":null,\"age\":30,".to_string() +
+/// "\"email\":\"alice@example.com\",\"is_active\":true,\"name\":\"Alice\",\"score\":95.7}");
+/// assert_eq!(json_result, buffer_result);
+/// assert_eq!(json_result, serde_json::to_string(&json_value)?);
+/// # Ok::<(), Box<dyn std::error::Error>>(())
+/// ```
+pub fn json_to_variant(json: &str, builder: &mut VariantBuilder) -> Result<(), ArrowError> {
+ let json: Value = serde_json::from_str(json)
+ .map_err(|e| ArrowError::InvalidArgumentError(format!("JSON format error: {e}")))?;
+
+ build_json(&json, builder)?;
+ Ok(())
+}
+
+fn build_json(json: &Value, builder: &mut VariantBuilder) -> Result<(), ArrowError> {
+ append_json(json, builder)?;
+ Ok(())
+}
+
+fn variant_from_number<'a, 'b>(n: &Number) -> Result<Variant<'a, 'b>, ArrowError> {
+ if let Some(i) = n.as_i64() {
+ // Find minimum Integer width to fit
+ if i as i8 as i64 == i {
+ Ok((i as i8).into())
+ } else if i as i16 as i64 == i {
+ Ok((i as i16).into())
+ } else if i as i32 as i64 == i {
+ Ok((i as i32).into())
+ } else {
+ Ok(i.into())
+ }
+ } else {
+ // Todo: Try decimal once we implement custom JSON parsing where we have access to strings
+ // Try double - currently json_to_variant does not produce decimal
+ match n.as_f64() {
+ Some(f) => return Ok(f.into()),
+ None => Err(ArrowError::InvalidArgumentError(format!(
+ "Failed to parse {} as number",
+ n.to_string()
+ ))),
+ }?
+ }
+}
+
+fn append_json(json: &Value, builder: &mut impl VariantBuilderExt) -> Result<(), ArrowError> {
+ match json {
+ Value::Null => builder.append_value(Variant::Null),
+ Value::Bool(b) => builder.append_value(*b),
+ Value::Number(n) => {
+ builder.append_value(variant_from_number(n)?);
+ }
+ Value::String(s) => builder.append_value(s.as_str()),
+ Value::Array(arr) => {
+ let mut list_builder = builder.new_list();
+ for val in arr {
+ append_json(val, &mut list_builder)?;
+ }
+ list_builder.finish();
+ }
+ Value::Object(obj) => {
+ let mut obj_builder = builder.new_object();
+ 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();
+ }
+ };
+ Ok(())
+}
+
+struct ObjectFieldBuilder<'a, 'b, 'c> {
+ key: &'a str,
+ builder: &'b mut ObjectBuilder<'c, 'a>,
Review Comment:
This is hard to interpret... can we use `'m` and `'v`?
```suggestion
struct ObjectFieldBuilder<'m, 'v, 'r> {
key: &'r str,
builder: &'r mut ObjectBuilder<'m, 'v>,
```
(here, `'r` is the lifetime of the references used to construct the field builder)
##########
parquet-variant/src/to_json.rs:
##########
@@ -249,27 +245,6 @@ 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> {
Review Comment:
Seems like a macro could avoid both overhead and duplication? The only part that needs to change (besides the data types) is Decimal16 has a more complicated way of handling the integer value produced at the end.
##########
parquet-variant/src/from_json.rs:
##########
@@ -0,0 +1,149 @@
+// 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
+
+use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder, VariantBuilderExt};
+use arrow_schema::ArrowError;
+use serde_json::{Number, Value};
+
+/// Converts a JSON string to Variant using [`VariantBuilder`]. The resulting `value` and `metadata`
+/// buffers can be extracted using `builder.finish()`
+///
+/// # Arguments
+/// * `json` - The JSON string to parse as Variant.
+/// * `variant_builder` - Object of type `VariantBuilder` used to build the vatiant from the JSON
+/// string
+///
+/// # Returns
+///
+/// * `Ok(())` if successful
+/// * `Err` with error details if the conversion fails
+///
+/// ```rust
+/// # use parquet_variant::{
+/// json_to_variant, variant_to_json, variant_to_json_string, variant_to_json_value, VariantBuilder
+/// };
+///
+/// let mut variant_builder = VariantBuilder::new();
+/// let person_string = "{\"name\":\"Alice\", \"age\":30, ".to_string()
+/// + "\"email\":\"alice@example.com\", \"is_active\": true, \"score\": 95.7,"
+/// + "\"additional_info\": null}";
+/// json_to_variant(&person_string, &mut variant_builder)?;
+///
+/// let (metadata, value) = variant_builder.finish();
+///
+/// let variant = parquet_variant::Variant::try_new(&metadata, &value)?;
+///
+/// let json_result = variant_to_json_string(&variant)?;
+/// let json_value = variant_to_json_value(&variant)?;
+///
+/// let mut buffer = Vec::new();
+/// variant_to_json(&mut buffer, &variant)?;
+/// let buffer_result = String::from_utf8(buffer)?;
+/// assert_eq!(json_result, "{\"additional_info\":null,\"age\":30,".to_string() +
+/// "\"email\":\"alice@example.com\",\"is_active\":true,\"name\":\"Alice\",\"score\":95.7}");
+/// assert_eq!(json_result, buffer_result);
+/// assert_eq!(json_result, serde_json::to_string(&json_value)?);
+/// # Ok::<(), Box<dyn std::error::Error>>(())
+/// ```
+pub fn json_to_variant(json: &str, builder: &mut VariantBuilder) -> Result<(), ArrowError> {
+ let json: Value = serde_json::from_str(json)
+ .map_err(|e| ArrowError::InvalidArgumentError(format!("JSON format error: {e}")))?;
+
+ build_json(&json, builder)?;
+ Ok(())
+}
+
+fn build_json(json: &Value, builder: &mut VariantBuilder) -> Result<(), ArrowError> {
+ append_json(json, builder)?;
+ Ok(())
+}
+
+fn variant_from_number<'a, 'b>(n: &Number) -> Result<Variant<'a, 'b>, ArrowError> {
Review Comment:
```suggestion
fn variant_from_number<'m, 'v>(n: &Number) -> Result<Variant<'m, 'v>, ArrowError> {
```
##########
parquet-variant/tests/test_json_to_variant.rs:
##########
@@ -254,7 +269,7 @@ fn test_json_to_variant_decimal16_max_scale() -> Result<(), ArrowError> {
fn test_json_to_variant_double_precision() -> Result<(), ArrowError> {
JsonToVariantTest {
json: "0.79228162514264337593543950335",
- expected: Variant::Double(0.792_281_625_142_643_3_f64),
+ expected: Variant::Double(0.792_281_625_142_643_4_f64),
Review Comment:
what made this one change in the latest commit, out of curiosity?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2168001273
##########
parquet-variant/Cargo.toml:
##########
@@ -35,7 +35,10 @@ rust-version = "1.83"
[dependencies]
arrow-schema = { workspace = true }
chrono = { workspace = true }
-serde_json = "1.0"
+# "arbitrary_precision" allows us to manually parse numbers. "preserve_order" does not automatically
+# sort object keys
+serde_json = { version = "1.0", features = ["arbitrary_precision", "preserve_order"] }
Review Comment:
Oh, for sure it's a follow-up. But we probably do need a story for how to handle that feature flag in a dependency (maybe part of the bigger story of how to handle a serde_json dependency in the first place).
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2168279235
##########
parquet-variant/Cargo.toml:
##########
@@ -35,7 +35,10 @@ rust-version = "1.83"
[dependencies]
arrow-schema = { workspace = true }
chrono = { workspace = true }
-serde_json = "1.0"
+# "arbitrary_precision" allows us to manually parse numbers. "preserve_order" does not automatically
+# sort object keys
+serde_json = { version = "1.0", features = ["arbitrary_precision", "preserve_order"] }
Review Comment:
https://github.com/delta-io/delta-kernel-rs/issues/1036
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3033658614
I envision the reverse cast as well
```rust
fn variant_to_string<O: OffsetSize>(array: &ArrayRef) -> GenericStringArray<O> {
...
}
fn variant_to_string_view(array: &ArrayRef) -> StringViewArray {
...
}
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2167745526
##########
parquet-variant/src/decoder.rs:
##########
@@ -26,6 +26,11 @@ use std::num::TryFromIntError;
// Makes the code a bit more readable
pub(crate) const VARIANT_VALUE_HEADER_BYTES: usize = 1;
+pub(crate) const MAX_UNSCALED_DECIMAL_4: i32 = 999999999;
+pub(crate) const MAX_PRECISION_DECIMAL_4: u8 = 9;
+pub(crate) const MAX_UNSCALED_DECIMAL_8: i64 = 999999999999999999i64;
+pub(crate) const MAX_PRECISION_DECIMAL_8: u8 = 18;
Review Comment:
Now that https://github.com/apache/arrow-rs/pull/7776 merged, you can use `VariantDecimalXX::MAX_PRECISION` and `VariantDecimalXX::MAX_UNSCALED_VALUE`
##########
parquet-variant/src/from_json.rs:
##########
@@ -0,0 +1,97 @@
+use crate::variant_buffer_manager::VariantBufferManager;
+use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder};
+use arrow_schema::ArrowError;
+use serde_json::{Map, Value};
+
+/// Eventually, internal writes should also be performed using VariantBufferManager instead of
+/// ValueBuffer and MetadataBuffer so the caller has control of the memory.
+/// Returns a pair <value_size, metadata_size>
+pub fn json_to_variant<'a, T: VariantBufferManager>(
+ json: &str,
+ variant_buffer_manager: &'a mut T,
Review Comment:
```suggestion
pub fn json_to_variant<'a>(
json: &str,
variant_buffer_manager: &'a mut impl VariantBufferManager,
```
##########
parquet-variant/src/from_json.rs:
##########
@@ -0,0 +1,97 @@
+use crate::variant_buffer_manager::VariantBufferManager;
+use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder};
+use arrow_schema::ArrowError;
+use serde_json::{Map, Value};
+
+/// Eventually, internal writes should also be performed using VariantBufferManager instead of
+/// ValueBuffer and MetadataBuffer so the caller has control of the memory.
+/// Returns a pair <value_size, metadata_size>
+pub fn json_to_variant<'a, T: VariantBufferManager>(
+ json: &str,
+ variant_buffer_manager: &'a mut T,
+) -> Result<(usize, usize), ArrowError> {
+ let mut builder = VariantBuilder::new();
+ let json: Value = serde_json::from_str(json)
+ .map_err(|e| ArrowError::InvalidArgumentError(format!("JSON format error: {}", e)))?;
+
+ build_json(&json, &mut builder)?;
+ let (metadata, value) = builder.finish();
+ let value_size = value.len();
+ let metadata_size = metadata.len();
+
+ // Write to caller's buffers - Remove this when the library internally writes to the caller's
+ // buffers anyway
+ variant_buffer_manager.ensure_metadata_buffer_size(metadata_size)?;
+ variant_buffer_manager.ensure_value_buffer_size(value_size)?;
+ let caller_metadata_buffer = variant_buffer_manager.borrow_metadata_buffer();
+ caller_metadata_buffer[..metadata_size].copy_from_slice(metadata.as_slice());
+ let caller_value_buffer = variant_buffer_manager.borrow_value_buffer();
+ caller_value_buffer[..value_size].copy_from_slice(value.as_slice());
+ Ok((metadata_size, value_size))
+}
+
+fn build_json(json: &Value, builder: &mut VariantBuilder) -> Result<(), ArrowError> {
+ match json {
+ Value::Null => builder.append_value(Variant::Null),
+ Value::Bool(b) => builder.append_value(*b),
+ Value::Number(n) => {
+ let v: Variant = n.try_into()?;
+ builder.append_value(v)
+ }
+ 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)?;
+ list_builder.finish();
+ }
+ Value::Object(obj) => {
+ let mut obj_builder = builder.new_object();
+ build_object(obj, &mut obj_builder)?;
+ obj_builder.finish();
+ }
+ };
+ Ok(())
+}
+
+fn build_list(arr: &Vec<Value>, builder: &mut ListBuilder) -> Result<(), ArrowError> {
+ for val in arr {
+ match val {
+ Value::Null => builder.append_value(Variant::Null),
+ Value::Bool(b) => builder.append_value(*b),
+ Value::Number(n) => {
+ let v: Variant = n.try_into()?;
+ builder.append_value(v)
+ }
+ 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)?;
+ list_builder.finish()
+ }
+ Value::Object(obj) => {
+ let mut obj_builder = builder.new_object();
+ build_object(obj, &mut obj_builder)?;
+ obj_builder.finish();
+ }
+ }
+ }
+ Ok(())
+}
+
+fn build_object(obj: &Map<String, Value>, builder: &mut ObjectBuilder) -> Result<(), ArrowError> {
+ for (key, value) in obj.iter() {
+ match value {
+ Value::Null => builder.append_value(key, Variant::Null),
Review Comment:
Following the above idea, we could build a simple newtype wrapper for `(&mut ObjectBuilder, &str)`, that can then impl the same trait as `VariantBuilder` and `ListBuilder`, which allows something like:
```rust
for (key, value) in obj.iter() {
let builder = ObjectFieldBuilder::new(builder, key);
build_json(val, &mut builder);
}
```
##########
parquet-variant/src/from_json.rs:
##########
@@ -0,0 +1,97 @@
+use crate::variant_buffer_manager::VariantBufferManager;
+use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder};
+use arrow_schema::ArrowError;
+use serde_json::{Map, Value};
+
+/// Eventually, internal writes should also be performed using VariantBufferManager instead of
+/// ValueBuffer and MetadataBuffer so the caller has control of the memory.
+/// Returns a pair <value_size, metadata_size>
+pub fn json_to_variant<'a, T: VariantBufferManager>(
+ json: &str,
+ variant_buffer_manager: &'a mut T,
+) -> Result<(usize, usize), ArrowError> {
+ let mut builder = VariantBuilder::new();
+ let json: Value = serde_json::from_str(json)
+ .map_err(|e| ArrowError::InvalidArgumentError(format!("JSON format error: {}", e)))?;
+
+ build_json(&json, &mut builder)?;
+ let (metadata, value) = builder.finish();
+ let value_size = value.len();
+ let metadata_size = metadata.len();
+
+ // Write to caller's buffers - Remove this when the library internally writes to the caller's
+ // buffers anyway
+ variant_buffer_manager.ensure_metadata_buffer_size(metadata_size)?;
+ variant_buffer_manager.ensure_value_buffer_size(value_size)?;
+ let caller_metadata_buffer = variant_buffer_manager.borrow_metadata_buffer();
+ caller_metadata_buffer[..metadata_size].copy_from_slice(metadata.as_slice());
+ let caller_value_buffer = variant_buffer_manager.borrow_value_buffer();
+ caller_value_buffer[..value_size].copy_from_slice(value.as_slice());
+ Ok((metadata_size, value_size))
+}
+
+fn build_json(json: &Value, builder: &mut VariantBuilder) -> Result<(), ArrowError> {
+ match json {
+ Value::Null => builder.append_value(Variant::Null),
+ Value::Bool(b) => builder.append_value(*b),
+ Value::Number(n) => {
+ let v: Variant = n.try_into()?;
+ builder.append_value(v)
+ }
+ 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)?;
+ list_builder.finish();
+ }
+ Value::Object(obj) => {
+ let mut obj_builder = builder.new_object();
+ build_object(obj, &mut obj_builder)?;
+ obj_builder.finish();
+ }
+ };
+ Ok(())
+}
+
+fn build_list(arr: &Vec<Value>, builder: &mut ListBuilder) -> Result<(), ArrowError> {
+ for val in arr {
+ match val {
+ Value::Null => builder.append_value(Variant::Null),
+ Value::Bool(b) => builder.append_value(*b),
+ Value::Number(n) => {
+ let v: Variant = n.try_into()?;
+ builder.append_value(v)
+ }
+ 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)?;
+ list_builder.finish()
+ }
+ Value::Object(obj) => {
+ let mut obj_builder = builder.new_object();
+ build_object(obj, &mut obj_builder)?;
+ obj_builder.finish();
+ }
Review Comment:
This match is syntactically identical to the match in `build_json`. Maybe we need a trait that `VariantBuilder` and `ListBuilder` both implement, and then can just do:
```rust
for val in arr {
build_json(val, &mut builder);
}
```
##########
parquet-variant/src/from_json.rs:
##########
@@ -0,0 +1,97 @@
+use crate::variant_buffer_manager::VariantBufferManager;
+use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder};
+use arrow_schema::ArrowError;
+use serde_json::{Map, Value};
+
+/// Eventually, internal writes should also be performed using VariantBufferManager instead of
+/// ValueBuffer and MetadataBuffer so the caller has control of the memory.
+/// Returns a pair <value_size, metadata_size>
+pub fn json_to_variant<'a, T: VariantBufferManager>(
+ json: &str,
+ variant_buffer_manager: &'a mut T,
+) -> Result<(usize, usize), ArrowError> {
+ let mut builder = VariantBuilder::new();
+ let json: Value = serde_json::from_str(json)
+ .map_err(|e| ArrowError::InvalidArgumentError(format!("JSON format error: {}", e)))?;
+
+ build_json(&json, &mut builder)?;
+ let (metadata, value) = builder.finish();
+ let value_size = value.len();
+ let metadata_size = metadata.len();
+
+ // Write to caller's buffers - Remove this when the library internally writes to the caller's
+ // buffers anyway
+ variant_buffer_manager.ensure_metadata_buffer_size(metadata_size)?;
+ variant_buffer_manager.ensure_value_buffer_size(value_size)?;
+ let caller_metadata_buffer = variant_buffer_manager.borrow_metadata_buffer();
+ caller_metadata_buffer[..metadata_size].copy_from_slice(metadata.as_slice());
+ let caller_value_buffer = variant_buffer_manager.borrow_value_buffer();
+ caller_value_buffer[..value_size].copy_from_slice(value.as_slice());
+ Ok((metadata_size, value_size))
+}
+
+fn build_json(json: &Value, builder: &mut VariantBuilder) -> Result<(), ArrowError> {
+ match json {
+ Value::Null => builder.append_value(Variant::Null),
+ Value::Bool(b) => builder.append_value(*b),
+ Value::Number(n) => {
+ let v: Variant = n.try_into()?;
+ builder.append_value(v)
+ }
+ 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)?;
+ list_builder.finish();
+ }
+ Value::Object(obj) => {
+ let mut obj_builder = builder.new_object();
+ build_object(obj, &mut obj_builder)?;
+ obj_builder.finish();
+ }
+ };
+ Ok(())
+}
+
+fn build_list(arr: &Vec<Value>, builder: &mut ListBuilder) -> Result<(), ArrowError> {
+ for val in arr {
+ match val {
+ Value::Null => builder.append_value(Variant::Null),
+ Value::Bool(b) => builder.append_value(*b),
+ Value::Number(n) => {
+ let v: Variant = n.try_into()?;
+ builder.append_value(v)
+ }
+ 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)?;
+ list_builder.finish()
+ }
+ Value::Object(obj) => {
+ let mut obj_builder = builder.new_object();
+ build_object(obj, &mut obj_builder)?;
+ obj_builder.finish();
+ }
+ }
+ }
+ Ok(())
+}
+
+fn build_object(obj: &Map<String, Value>, builder: &mut ObjectBuilder) -> Result<(), ArrowError> {
+ for (key, value) in obj.iter() {
+ match value {
+ Value::Null => builder.append_value(key, Variant::Null),
+ Value::Bool(b) => builder.append_value(key, *b),
+ Value::Number(n) => {
+ let v: Variant = n.try_into()?;
+ builder.append_value(key, v)
+ }
+ Value::String(s) => builder.append_value(key, s.as_str()),
+ Value::Array(_) | Value::Object(_) => {
+ todo!("Nesting within objects unsupported right now.");
+ }
Review Comment:
Is there some blocker here? Or just TODO?
(surprised we can nest inside arrays but not inside objects?)
##########
parquet-variant/src/variant_buffer_manager.rs:
##########
@@ -0,0 +1,124 @@
+use arrow_schema::ArrowError;
+
+pub trait VariantBufferManager {
+ /// Returns the slice where the variant metadata needs to be written to. This method may be
+ /// called several times during the construction of a new `metadata` field in a variant. The
+ /// implementation must make sure that on every call, all the data written to the metadata
+ /// buffer so far are preserved.
+ fn borrow_metadata_buffer(&mut self) -> &mut [u8];
+
+ /// Returns the slice where value needs to be written to. This method may be called several
+ /// times during the construction of a new `value` field in a variant. The implementation must
+ /// make sure that on every call, all the data written to the value buffer so far are preserved.
+ fn borrow_value_buffer(&mut self) -> &mut [u8];
+
+ /// Ensures that the next call to `borrow_metadata_buffer` returns a slice having at least
+ /// `size` bytes. Also ensures that the value metadata written so far are persisted - this means
+ /// that if `borrow_metadata_buffer` is to return a new buffer from the next call onwards, the
+ /// new buffer must have the contents of the old metadata buffer.
+ fn ensure_metadata_buffer_size(&mut self, size: usize) -> Result<(), ArrowError>;
+
+ /// Ensures that the next call to `borrow_value_buffer` returns a slice having at least `size`
+ /// bytes. Also ensures that the value bytes written so far are persisted - this means that
+ /// if `borrow_value_buffer` is to return a new buffer from the next call onwards, the new
+ /// buffer must have the contents of the old value buffer.
+ fn ensure_value_buffer_size(&mut self, size: usize) -> Result<(), ArrowError>;
+
+ fn get_immutable_metadata_buffer(&self) -> &[u8] {
+ unimplemented!("Optional method not implemented in VariantBufferManager")
+ }
+
+ fn get_immutable_value_buffer(&self) -> &[u8] {
+ unimplemented!("Optional method not implemented in VariantBufferManager")
+ }
+}
+
+pub struct SampleBoxBasedVariantBufferManager {
+ pub value_buffer: Box<[u8]>,
+ pub metadata_buffer: Box<[u8]>,
Review Comment:
Continuing the above thought about building up an arrow `Array`... maybe a `SampleMultiVariantBufferManager` that can stack a bunch of variants end to end, with a `len` method that allows to extract offsets?
##########
parquet-variant/Cargo.toml:
##########
@@ -35,7 +35,10 @@ rust-version = "1.83"
[dependencies]
arrow-schema = { workspace = true }
chrono = { workspace = true }
-serde_json = "1.0"
+# "arbitrary_precision" allows us to manually parse numbers. "preserve_order" does not automatically
+# sort object keys
+serde_json = { version = "1.0", features = ["arbitrary_precision", "preserve_order"] }
Review Comment:
TIL about the (undocumented) `arbitrary_precision` feature flag and (undocumented) `Number::as_str` method it unlocks 🤯
That could solve a lot of problems for variant decimals on the to_json path as well:
https://github.com/apache/arrow-rs/blob/main/parquet-variant/src/to_json.rs#L147-L162
##########
parquet-variant/src/from_json.rs:
##########
@@ -0,0 +1,97 @@
+use crate::variant_buffer_manager::VariantBufferManager;
+use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder};
+use arrow_schema::ArrowError;
+use serde_json::{Map, Value};
+
+/// Eventually, internal writes should also be performed using VariantBufferManager instead of
+/// ValueBuffer and MetadataBuffer so the caller has control of the memory.
+/// Returns a pair <value_size, metadata_size>
+pub fn json_to_variant<'a, T: VariantBufferManager>(
+ json: &str,
+ variant_buffer_manager: &'a mut T,
+) -> Result<(usize, usize), ArrowError> {
+ let mut builder = VariantBuilder::new();
+ let json: Value = serde_json::from_str(json)
+ .map_err(|e| ArrowError::InvalidArgumentError(format!("JSON format error: {}", e)))?;
+
+ build_json(&json, &mut builder)?;
+ let (metadata, value) = builder.finish();
+ let value_size = value.len();
+ let metadata_size = metadata.len();
+
+ // Write to caller's buffers - Remove this when the library internally writes to the caller's
+ // buffers anyway
+ variant_buffer_manager.ensure_metadata_buffer_size(metadata_size)?;
+ variant_buffer_manager.ensure_value_buffer_size(value_size)?;
+ let caller_metadata_buffer = variant_buffer_manager.borrow_metadata_buffer();
+ caller_metadata_buffer[..metadata_size].copy_from_slice(metadata.as_slice());
+ let caller_value_buffer = variant_buffer_manager.borrow_value_buffer();
+ caller_value_buffer[..value_size].copy_from_slice(value.as_slice());
+ Ok((metadata_size, value_size))
+}
+
+fn build_json(json: &Value, builder: &mut VariantBuilder) -> Result<(), ArrowError> {
+ match json {
+ Value::Null => builder.append_value(Variant::Null),
+ Value::Bool(b) => builder.append_value(*b),
+ Value::Number(n) => {
+ let v: Variant = n.try_into()?;
+ builder.append_value(v)
+ }
Review Comment:
```suggestion
Value::Number(n) => builder.append_value(Variant::try_from(n)?),
```
##########
parquet-variant/src/variant_buffer_manager.rs:
##########
@@ -0,0 +1,124 @@
+use arrow_schema::ArrowError;
+
+pub trait VariantBufferManager {
Review Comment:
I'm not quite sure I understand this trait.
It _almost_ seems like a good building block for eventual columnar writes, if an implementation took a mutable reference to a vec, where each `VariantBufferManager` you build with it is only capable of appending new bytes to the end (leaving existing bytes unchanged). That way, one could build up an offset array along the way as part of building up arrow binary arrays for the metadata and value columns?
But I don't understand the whole "may be called several times" part, which seems to return the whole slice on every call? I guess that's because the variant builder doesn't do its work all at once, but still expects to receive the overall slice?
Finally, why split the ensure and borrow methods, when both take `&mut self` and it's almost certainly dangerous to "borrow" without an "ensure" first (can't guess the size in advance, it's variant data)? Why not just a pair of `ensure_and_borrow_XXX_buffer` methods that return a slice of exactly the requested size (no larger, no smaller)?
##########
parquet-variant/src/variant.rs:
##########
@@ -1031,6 +1033,57 @@ impl From<VariantDecimal16> for Variant<'_, '_> {
}
}
+impl TryFrom<&Number> for Variant<'_, '_> {
+ type Error = ArrowError;
+
+ fn try_from(n: &Number) -> Result<Self, Self::Error> {
Review Comment:
You might want to take a look at the decimal parsing code in my prototype?
https://github.com/apache/arrow-rs/pull/7403/files#diff-fce219df196b3dffd6c2f6bb4924a34f299817a317943a19482289ecb8f6af69R1065-R1079
(tho that code is _not_ optimal, it might at least provide some ideas)
##########
parquet-variant/tests/test_json_to_variant.rs:
##########
@@ -0,0 +1,349 @@
+use arrow_schema::ArrowError;
+use parquet_variant::{json_to_variant, VariantBufferManager};
+
+pub struct SampleVariantBufferManager {
Review Comment:
How is this different from the box-based one above, and why not just use the vec-based one above instead?
##########
parquet-variant/src/variant.rs:
##########
@@ -1031,6 +1033,57 @@ impl From<VariantDecimal16> for Variant<'_, '_> {
}
}
+impl TryFrom<&Number> for Variant<'_, '_> {
+ type Error = ArrowError;
+
+ fn try_from(n: &Number) -> Result<Self, Self::Error> {
+ if n.is_i64() {
+ // Find minimum Integer width to fit
+ let i = n.as_i64().unwrap();
Review Comment:
```suggestion
if let Some(i) = n.is_i64() {
// Find minimum Integer width to fit
```
##########
parquet-variant/src/variant_buffer_manager.rs:
##########
@@ -0,0 +1,124 @@
+use arrow_schema::ArrowError;
+
+pub trait VariantBufferManager {
+ /// Returns the slice where the variant metadata needs to be written to. This method may be
+ /// called several times during the construction of a new `metadata` field in a variant. The
+ /// implementation must make sure that on every call, all the data written to the metadata
+ /// buffer so far are preserved.
+ fn borrow_metadata_buffer(&mut self) -> &mut [u8];
+
+ /// Returns the slice where value needs to be written to. This method may be called several
+ /// times during the construction of a new `value` field in a variant. The implementation must
+ /// make sure that on every call, all the data written to the value buffer so far are preserved.
+ fn borrow_value_buffer(&mut self) -> &mut [u8];
+
+ /// Ensures that the next call to `borrow_metadata_buffer` returns a slice having at least
+ /// `size` bytes. Also ensures that the value metadata written so far are persisted - this means
+ /// that if `borrow_metadata_buffer` is to return a new buffer from the next call onwards, the
+ /// new buffer must have the contents of the old metadata buffer.
+ fn ensure_metadata_buffer_size(&mut self, size: usize) -> Result<(), ArrowError>;
+
+ /// Ensures that the next call to `borrow_value_buffer` returns a slice having at least `size`
+ /// bytes. Also ensures that the value bytes written so far are persisted - this means that
+ /// if `borrow_value_buffer` is to return a new buffer from the next call onwards, the new
+ /// buffer must have the contents of the old value buffer.
+ fn ensure_value_buffer_size(&mut self, size: usize) -> Result<(), ArrowError>;
+
+ fn get_immutable_metadata_buffer(&self) -> &[u8] {
+ unimplemented!("Optional method not implemented in VariantBufferManager")
+ }
+
+ fn get_immutable_value_buffer(&self) -> &[u8] {
+ unimplemented!("Optional method not implemented in VariantBufferManager")
+ }
+}
+
+pub struct SampleBoxBasedVariantBufferManager {
+ pub value_buffer: Box<[u8]>,
+ pub metadata_buffer: Box<[u8]>,
Review Comment:
What purpose would this serve? It seems strictly inferior to a vec for both normal and testing uses?
##########
parquet-variant/src/from_json.rs:
##########
@@ -0,0 +1,97 @@
+use crate::variant_buffer_manager::VariantBufferManager;
+use crate::{ListBuilder, ObjectBuilder, Variant, VariantBuilder};
+use arrow_schema::ArrowError;
+use serde_json::{Map, Value};
+
+/// Eventually, internal writes should also be performed using VariantBufferManager instead of
+/// ValueBuffer and MetadataBuffer so the caller has control of the memory.
+/// Returns a pair <value_size, metadata_size>
+pub fn json_to_variant<'a, T: VariantBufferManager>(
+ json: &str,
+ variant_buffer_manager: &'a mut T,
+) -> Result<(usize, usize), ArrowError> {
+ let mut builder = VariantBuilder::new();
+ let json: Value = serde_json::from_str(json)
+ .map_err(|e| ArrowError::InvalidArgumentError(format!("JSON format error: {}", e)))?;
+
+ build_json(&json, &mut builder)?;
+ let (metadata, value) = builder.finish();
+ let value_size = value.len();
+ let metadata_size = metadata.len();
+
+ // Write to caller's buffers - Remove this when the library internally writes to the caller's
+ // buffers anyway
+ variant_buffer_manager.ensure_metadata_buffer_size(metadata_size)?;
+ variant_buffer_manager.ensure_value_buffer_size(value_size)?;
+ let caller_metadata_buffer = variant_buffer_manager.borrow_metadata_buffer();
+ caller_metadata_buffer[..metadata_size].copy_from_slice(metadata.as_slice());
+ let caller_value_buffer = variant_buffer_manager.borrow_value_buffer();
+ caller_value_buffer[..value_size].copy_from_slice(value.as_slice());
+ Ok((metadata_size, value_size))
+}
+
+fn build_json(json: &Value, builder: &mut VariantBuilder) -> Result<(), ArrowError> {
+ match json {
+ Value::Null => builder.append_value(Variant::Null),
+ Value::Bool(b) => builder.append_value(*b),
+ Value::Number(n) => {
+ let v: Variant = n.try_into()?;
+ builder.append_value(v)
+ }
+ 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)?;
+ list_builder.finish();
+ }
+ Value::Object(obj) => {
+ let mut obj_builder = builder.new_object();
+ build_object(obj, &mut obj_builder)?;
+ obj_builder.finish();
+ }
+ };
+ Ok(())
+}
+
+fn build_list(arr: &Vec<Value>, builder: &mut ListBuilder) -> Result<(), ArrowError> {
Review Comment:
I'm surprised clippy didn't flag this
```suggestion
fn build_list(arr: &[Value], builder: &mut ListBuilder) -> Result<(), ArrowError> {
```
##########
parquet-variant/src/variant.rs:
##########
@@ -1031,6 +1033,57 @@ impl From<VariantDecimal16> for Variant<'_, '_> {
}
}
+impl TryFrom<&Number> for Variant<'_, '_> {
+ type Error = ArrowError;
+
+ fn try_from(n: &Number) -> Result<Self, Self::Error> {
+ if n.is_i64() {
+ // Find minimum Integer width to fit
+ let i = n.as_i64().unwrap();
+ if i as i8 as i64 == i {
+ Ok((i as i8).into())
+ } else if i as i16 as i64 == i {
+ Ok((i as i16).into())
+ } else if i as i32 as i64 == i {
+ Ok((i as i32).into())
+ } else {
+ Ok(i.into())
+ }
Review Comment:
Interesting approach. That double `.. as .. as ..` definitely caused a double take, but I guess it could be cheaper than TryFrom?
```suggestion
let value = i8::try_from(i)
.map(Variant::from)
.or_else(|_| i16::try_from(i).map(Variant::from))
.or_else(|_| i32::try_from(i).map(Variant::from))
.unwrap_or_else(|| Variant::from(i));
Ok(value)
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3021263494
@alamb Few updates:
1. I have fixed the tests to remove raw binary values and we are comparing with Variant now.
2. I have changed the Variant decimal to string in `to_json` to attempt to use decimals. (I will add a couple more tests in a minute)
3. There is still one problem. I have included `serde_json` with the "preserve_order" feature only in the `parquet-variant` crate. However, it is also influencing tests in the `arrow-json` crate (particularly the `reader::schema::tests::test_null_field_inferred_as_null` test). The test only fails if you run it from the root and not the inner `arrow-json` library. Do you know how to isolate these features within crates?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#discussion_r2177500240
##########
parquet-variant/tests/test_json_to_variant.rs:
##########
@@ -0,0 +1,467 @@
+// 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, ShortString, Variant, VariantBuilder,
+ VariantDecimal16, VariantDecimal4, VariantDecimal8,
+};
+
+#[test]
+fn test_json_to_variant() -> Result<(), ArrowError> {
+ struct JsonToVariantTest<'a> {
+ json: &'a str,
+ expected: Variant<'a, 'a>,
+ }
+
+ impl<'a> JsonToVariantTest<'a> {
Review Comment:
❤️
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [VARIANT] Add support for the json_to_variant API [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7783:
URL: https://github.com/apache/arrow-rs/pull/7783#issuecomment-3024646157
I resolved some conflicts in this PR and I am working to sort out some of the other errors
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org