[Variant] Improve documentation and make kernels consistent #8536
Conversation
github-actions
bot
added
parquet
| use parquet_variant::{Variant, VariantBuilder}; | ||
| use parquet_variant_compute::variant_get::{variant_get, GetOptions}; | ||
| use parquet_variant_compute::{json_to_variant, VariantArray, VariantArrayBuilder}; | ||
| use parquet_variant_compute::{ |
There was a problem hiding this comment.
I moved all the functions into the main parquet_variant_compute module
| /// # use arrow::array::{Array, ArrayRef, Int64Array}; | ||
| /// # use parquet_variant::Variant; | ||
| /// # use parquet_variant_compute::cast_to_variant::cast_to_variant; | ||
| /// # use parquet_variant_compute::cast_to_variant; |
There was a problem hiding this comment.
likewise, I removed this extra level of namespacing
| //! * [`VariantType`] Arrow ExtensionType for Parquet Variant logical type. | ||
| //! [`variant_get`] to extracting a value by path and functions to convert | ||
| //! between `Variant` and JSON. | ||
| //! * Representation of [`Variant`], and [`VariantArray`] for working with |
There was a problem hiding this comment.
this was largely a copy of what was in parquet_variant and parquet_variant_compute so I changed the docs to highlight the most important functions and structures and link to the rest of the docs
There was a problem hiding this comment.
Pull Request Overview
This PR improves documentation for the Variant feature in parquet-rs and makes kernel exports more consistent by reducing public module visibility while maintaining access to necessary functions.
- Refactored documentation to better describe features and kernels available for Variant operations
- Made internal modules private and exposed only the required functions through the main module interface
- Updated import paths in examples and benchmarks to reflect the new module structure
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| parquet/src/variant.rs | Updated module-level documentation to better describe Variant features and refer to sub-modules |
| parquet-variant-compute/src/lib.rs | Improved documentation structure, made modules private, and exported specific functions |
| parquet-variant-compute/src/cast_to_variant.rs | Updated example import path to use function directly instead of module path |
| parquet-variant-compute/benches/variant_kernels.rs | Consolidated imports to use functions from main module instead of sub-modules |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Ryan Johnson <[email protected]>
Which issue does this PR close?
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.
Rationale for this change
Basically, I was confused about the casting / conversion functions available so I want to improve the documentation
What changes are included in this PR?
pubcrates in favor of exporting only the functions/ structs neededAre these changes tested?
Yes by CI
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
Are there any user-facing changes?
If there are user-facing changes then we may require documentation to be updated before approving the PR.
If there are any breaking changes to public APIs, please call them out.