Re: [PR] [C++][Parquet] Add variant type [arrow]

["wgtmac (via GitHub)" <gi...@apache.org>]

wgtmac commented on code in PR #45375:
URL: https://github.com/apache/arrow/pull/45375#discussion_r1959890857


##########
cpp/src/parquet/types.cc:
##########
@@ -1619,6 +1629,22 @@ class LogicalType::Impl::Float16 final : public LogicalType::Impl::Incompatible,
 
 GENERATE_MAKE(Float16)
 
+class LogicalType::Impl::Variant final : public LogicalType::Impl::Incompatible,
+                                         public LogicalType::Impl::SimpleApplicable {
+ public:
+  friend class VariantLogicalType;
+
+  OVERRIDE_TOSTRING(Variant)
+  OVERRIDE_TOTHRIFT(VariantType, VARIANT)
+
+ private:
+  Variant()
+      : LogicalType::Impl(LogicalType::Type::VARIANT, SortOrder::UNKNOWN),
+        LogicalType::Impl::SimpleApplicable(parquet::Type::BYTE_ARRAY) {}

Review Comment:
   IMO, `VariantLogicalType` should be similar to `MapLogicalType` and `ListLogicalType` which annotate group type. Though it (the unshredded form) is composed of two separate byte arrays, these two types are under the same group type as below. Can we model it as a `struct<binary,binary>`?
   
   ```
   optional group variant_name (VARIANT) {
     required binary metadata;
     required binary 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