wgtmac commented on code in PR #50252:
URL: https://github.com/apache/arrow/pull/50252#discussion_r3478890378
##########
cpp/src/parquet/properties.h:
##########
@@ -1458,6 +1470,7 @@ class PARQUET_EXPORT ArrowWriterProperties {
::arrow::internal::Executor* executor_;
bool write_time_adjusted_to_utc_;
+ bool variant_validation_enabled_;
Review Comment:
Why this belongs to `ArrowWriterProperties` but not `WriterProperties`? If
users the low-level parquet writer without Arrow API, serialized variant values
cannot be validated any more?
##########
cpp/src/parquet/arrow/variant_validate_internal.h:
##########
@@ -0,0 +1,36 @@
+// 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.
+
+#pragma once
+
+#include "arrow/status.h"
+#include "parquet/platform.h"
+
+namespace arrow {
+
+class ChunkedArray;
+class MemoryPool;
+
+} // namespace arrow
+
+namespace parquet::arrow::internal {
+
+PARQUET_EXPORT
+::arrow::Status ValidateVariants(const ::arrow::ChunkedArray& data,
Review Comment:
Why making them internal? It seems useful if users want to validate values
on their side.
##########
cpp/src/arrow/extension/variant/encoding.h:
##########
@@ -0,0 +1,157 @@
+// 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.
+
+#pragma once
+
+#include <cstdint>
+#include <optional>
+#include <string_view>
+#include <utility>
+#include <variant>
+#include <vector>
+
+#include "arrow/result.h"
+#include "arrow/status.h"
+#include "arrow/util/visibility.h"
+
+namespace arrow::extension {
+
+enum class VariantBasicType : uint8_t {
+ Primitive = 0,
Review Comment:
Why not adding `k` prefix like `kPrimitive`?
##########
cpp/src/arrow/extension/variant/encoding.h:
##########
@@ -0,0 +1,157 @@
+// 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.
+
+#pragma once
+
+#include <cstdint>
+#include <optional>
+#include <string_view>
+#include <utility>
+#include <variant>
+#include <vector>
+
+#include "arrow/result.h"
+#include "arrow/status.h"
+#include "arrow/util/visibility.h"
+
+namespace arrow::extension {
Review Comment:
It seems that all types defined in this file are not related to arrow so
perhaps we can move them to `cpp/src/parquet/variant` folder and use `namespace
parquet` or `namesapce parquet::variant`?
##########
cpp/src/arrow/extension/parquet_variant.cc:
##########
@@ -57,50 +59,148 @@ std::string VariantExtensionType::Serialize() const {
return ""; }
std::shared_ptr<Array> VariantExtensionType::MakeArray(
std::shared_ptr<ArrayData> data) const {
DCHECK_EQ(data->type->id(), Type::EXTENSION);
- DCHECK_EQ("arrow.parquet.variant",
+ DCHECK_EQ(kVariantExtensionName,
internal::checked_cast<const
ExtensionType&>(*data->type).extension_name());
return std::make_shared<VariantArray>(data);
}
namespace {
-bool IsBinaryField(const std::shared_ptr<Field> field) {
- return field->type()->storage_id() == Type::BINARY ||
- field->type()->storage_id() == Type::LARGE_BINARY;
+
+bool IsSupportedPrimitiveTypedValue(const std::shared_ptr<DataType>& type) {
Review Comment:
It seems that some Arrow primitive types are missing from here:
https://arrow.apache.org/docs/format/CanonicalExtensions.html#primitive-type-mappings
##########
cpp/src/arrow/extension/CMakeLists.txt:
##########
@@ -27,4 +27,6 @@ add_arrow_test(test
PREFIX
"arrow-canonical-extensions")
+add_subdirectory(variant)
Review Comment:
Should these files be relocated to `cpp/src/parquet/arrow/variant` instead
(just like what we did for geospatial types)? I think `cpp/src/arrow/extension`
is for metadata of extension type and array. If `ARROW_PARQUET` is OFF, we
don't need even these files to be compiled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
