You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/08/16 20:38:57 UTC
[GitHub] [arrow] progger-dev opened a new pull request, #13901: [ARROW-17255] Add JSON canonical extension type
progger-dev opened a new pull request, #13901:
URL: https://github.com/apache/arrow/pull/13901
Arrow now provides a canonical extension type for JSON data. This
extension is backed by utf8(). Parquet will recognize this extension
and appropriately propagate the LogicalType to the storage format.
--
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
[GitHub] [arrow] pradeepg26 commented on a diff in pull request #13901: ARROW-17255: [C++][Parquet] Add JSON canonical extension type
Posted by GitBox <gi...@apache.org>.
pradeepg26 commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1053638685
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -1410,6 +1417,43 @@ TEST_F(TestLargeStringParquetIO, Basics) {
this->RoundTripSingleColumn(large_array, large_array, arrow_properties);
}
+using TestJsonParquetIO = TestParquetIO<::arrow::extension::JsonExtensionType>;
+
+TEST_F(TestJsonParquetIO, JsonExtension) {
+ const char* json = R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])";
+
+ const auto json_type = ::arrow::extension::json();
+ const auto json_string_array = ::arrow::ArrayFromJSON(::arrow::utf8(), json);
+ const auto json_array = ::arrow::ExtensionType::WrapArray(json_type, json_string_array);
+
+ // When the original Arrow schema isn't stored and Arrow extensions are disabled,
+ // LogicalType::JSON is read as Binary.
+ const auto binary_array = ::arrow::ArrayFromJSON(::arrow::binary(), json);
Review Comment:
I think that should be fine. The behavior should be implementation dependent. So, if the Rust implementation reads the Parquet JSON type as `utf8` it should continue to do that.
I'm not currently planning on touching the Rust implementation. If and when it is updated to support this extension, I would expect the current behavior to be preserved if the user requests the extension to be disabled.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1752367197
##########
cpp/src/parquet/properties.h:
##########
@@ -941,6 +942,18 @@ class PARQUET_EXPORT ArrowReaderProperties {
return coerce_int96_timestamp_unit_;
}
+ /// Enable Parquet supported Arrow ExtensionTypes.
+ ///
+ /// When enabled, Parquet will use supported Arrow ExtensionTypes by mapping correctly
+ /// mapping them to Arrow types at read time. Currently only arrow::extension::json()
+ /// extension type is supported. Columns whose LogicalType is JSON will be interpreted
+ /// as arrow::extension::json() ExtensionType with storage type utf8, large_utf8 or
Review Comment:
@wgtmac do you mean cast on read?
edit: I suppose general way @pitrou is proposing would be a prerequisite.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1745155798
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -427,6 +428,13 @@ Status FieldToNode(const std::string& name, const std::shared_ptr<Field>& field,
}
case ArrowTypeId::EXTENSION: {
auto ext_type = std::static_pointer_cast<::arrow::ExtensionType>(field->type());
+ // Built-in JSON extension is handled differently.
+ if (ext_type->extension_name() == std::string("arrow.json")) {
Review Comment:
Yes. Are you just noting or proposing a change? :)
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2342974790
Thanks for the review @pitrou, responded.
--
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] ARROW-17255: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2052508576
I have some time next week and can try to move it forward if you don't 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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1735971828
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
+ DCHECK_EQ(inferred_type->id(), ::arrow::Type::BINARY);
+ inferred->field = inferred->field->WithType(origin_type);
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(origin_field, inferred));
+ } else {
+ auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
- // Apply metadata recursively to storage type
- RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
+ // Apply metadata recursively to storage type
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
- // Restore extension type, if the storage type is the same as inferred
- // from the Parquet type
- if (ex_type.storage_type()->Equals(*inferred->field->type())) {
- inferred->field = inferred->field->WithType(origin_type);
+ // Restore extension type, if the storage type is the same as inferred
+ // from the Parquet type
+ if (ex_type.storage_type()->Equals(*inferred->field->type())) {
+ inferred->field = inferred->field->WithType(origin_type);
+ }
}
modified = true;
} else {
+ if (inferred_type->id() == ::arrow::Type::EXTENSION) {
+ const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*inferred_type);
+ if (ex_type.extension_name() ==
+ ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are ENABLED in Parquet.
+ // origin_type is ::arrow::binary()
+ // inferred_type is ::arrow::extension::json()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
Review Comment:
https://github.com/apache/arrow/blob/6c17b794509d3931225cf295ae864204162c786f/cpp/src/parquet/arrow/schema.cc#L428-L433
@rok is right
I think we can support serialize whole schema here, and during read, if we don't recognize that, we can use underlying storage? ( this might be in separate patch )
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733422337
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == ::arrow::extension::JsonExtensionType::type_name()) {
Review Comment:
Changed.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1752672353
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,86 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+Status ArrowSchemaToParquetMetadata(std::shared_ptr<::arrow::Schema>& arrow_schema,
+ std::shared_ptr<KeyValueMetadata>& metadata) {
+ ARROW_ASSIGN_OR_RAISE(
+ std::shared_ptr<Buffer> serialized,
+ ::arrow::ipc::SerializeSchema(*arrow_schema, ::arrow::default_memory_pool()));
+ std::string schema_as_string = serialized->ToString();
+ std::string schema_base64 = ::arrow::util::base64_encode(schema_as_string);
+ metadata = ::arrow::key_value_metadata({"ARROW:schema"}, {schema_base64});
+ return Status::OK();
+}
+
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // If Arrow extensions are enabled, both fields should be treated as json() extension
+ // fields.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ auto arrow_schema =
+ ::arrow::schema({::arrow::field("json_1", ::arrow::extension::json(), true),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
Review Comment:
Changed (https://github.com/apache/arrow/pull/13901/commits/2648d7aad493e2d9a442301a19a68f3dc4f89772).
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,86 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+Status ArrowSchemaToParquetMetadata(std::shared_ptr<::arrow::Schema>& arrow_schema,
+ std::shared_ptr<KeyValueMetadata>& metadata) {
+ ARROW_ASSIGN_OR_RAISE(
+ std::shared_ptr<Buffer> serialized,
+ ::arrow::ipc::SerializeSchema(*arrow_schema, ::arrow::default_memory_pool()));
+ std::string schema_as_string = serialized->ToString();
+ std::string schema_base64 = ::arrow::util::base64_encode(schema_as_string);
+ metadata = ::arrow::key_value_metadata({"ARROW:schema"}, {schema_base64});
+ return Status::OK();
+}
+
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // If Arrow extensions are enabled, both fields should be treated as json() extension
+ // fields.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ auto arrow_schema =
+ ::arrow::schema({::arrow::field("json_1", ::arrow::extension::json(), true),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // Parquet logical type has precedence. Both json_1 and json_2 should be returned
+ // as a json() field even though extensions are not enabled.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ std::shared_ptr<KeyValueMetadata> field_metadata =
+ ::arrow::key_value_metadata({"foo", "bar"}, {"biz", "baz"});
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true, field_metadata),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
Review Comment:
Ditto.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733420127
##########
cpp/src/arrow/extension/json.h:
##########
@@ -0,0 +1,65 @@
+// 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 <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/visibility.h"
+
+namespace arrow::extension {
+
+/// \brief Concrete type class for variable-size JSON data, utf8-encoded.
+class ARROW_EXPORT JsonExtensionType : public ExtensionType {
+ public:
+ explicit JsonExtensionType(const std::shared_ptr<DataType>& storage_type)
+ : ExtensionType(storage_type), storage_type_(storage_type) {
+ ARROW_CHECK(storage_type->Equals(utf8()) || storage_type->Equals(large_utf8()) ||
Review Comment:
Changed.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733647343
##########
cpp/src/parquet/properties.h:
##########
@@ -941,6 +942,15 @@ class PARQUET_EXPORT ArrowReaderProperties {
return coerce_int96_timestamp_unit_;
}
+ /// Enable Parquet supported Arrow Extension Types.
+ ///
+ /// When enabled, Parquet will use supported Arrow ExtensionTypes in mapping to Arrow
+ /// schema. Currently only arrow::extension::json() extension type is supported. This
+ /// will be used for binary columns whose LogicalType is JSON.
+ void enable_known_arrow_extensions() { known_arrow_extensions_enabled_ = true; }
+ void disable_known_arrow_extensions() { known_arrow_extensions_enabled_ = false; }
+ bool known_arrow_extensions_enabled() const { return known_arrow_extensions_enabled_; }
Review Comment:
Renamed.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1741226039
##########
cpp/src/arrow/util/utf8_util_test.cc:
##########
@@ -562,5 +564,30 @@ TEST(UTF8Length, Basics) {
ASSERT_EQ(length("\xf0\x9f\x99\x8c"), 1);
}
+class UTF8ExtensionArrayTest : public ::testing::Test {
+ public:
+ static std::shared_ptr<Array> ExampleJson(
+ const std::shared_ptr<DataType>& storage_type) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(storage_type, R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])");
+ return ExtensionType::WrapArray(arrow::extension::json(storage_type), arr);
+ }
+};
+
+TEST_F(UTF8ExtensionArrayTest, JSONExtensionType) {
Review Comment:
Moved to `json_test.cc`.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2325256524
Thanks for the review @pitrou. I replied to your points.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1773099927
##########
cpp/src/arrow/extension/json.cc:
##########
@@ -0,0 +1,61 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/status.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow::extension {
+
+bool JsonExtensionType::ExtensionEquals(const ExtensionType& other) const {
+ return other.extension_name() == this->extension_name();
+}
+
+Result<std::shared_ptr<DataType>> JsonExtensionType::Deserialize(
+ std::shared_ptr<DataType> storage_type, const std::string& serialized) const {
+ if (storage_type->id() != Type::STRING && storage_type->id() != Type::STRING_VIEW &&
+ storage_type->id() != Type::LARGE_STRING) {
+ return Status::Invalid("Invalid storage type for JsonExtensionType: ",
+ storage_type->ToString());
+ }
+ return std::make_shared<JsonExtensionType>(storage_type);
+}
+
+std::string JsonExtensionType::Serialize() const { return ""; }
+
+std::shared_ptr<Array> JsonExtensionType::MakeArray(
+ std::shared_ptr<ArrayData> data) const {
+ DCHECK_EQ(data->type->id(), Type::EXTENSION);
+ DCHECK_EQ("arrow.json",
+ internal::checked_cast<const ExtensionType&>(*data->type).extension_name());
+ return std::make_shared<ExtensionArray>(data);
+}
+
+std::shared_ptr<DataType> json(const std::shared_ptr<DataType> storage_type) {
+ ARROW_CHECK(storage_type->id() != Type::STRING ||
+ storage_type->id() != Type::STRING_VIEW ||
+ storage_type->id() != Type::LARGE_STRING);
Review Comment:
This check is not correct also.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1735974412
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -1387,8 +1394,8 @@ using TestLargeBinaryParquetIO = TestParquetIO<::arrow::LargeBinaryType>;
TEST_F(TestLargeBinaryParquetIO, Basics) {
const char* json = "[\"foo\", \"\", null, \"\xff\"]";
- const auto large_type = ::arrow::large_binary();
- const auto narrow_type = ::arrow::binary();
+ const auto large_type = ::arrow::large_utf8();
+ const auto narrow_type = ::arrow::utf8();
const auto large_array = ::arrow::ArrayFromJSON(large_type, json);
Review Comment:
I suggest this change because in Parquet this is defined as "valid utf8" . But you can decide that?
https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift#L154
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2330997229
Ping @pitrou
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1745778317
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
+ DCHECK_EQ(inferred_type->id(), ::arrow::Type::BINARY);
+ inferred->field = inferred->field->WithType(origin_type);
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(origin_field, inferred));
+ } else {
+ auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
- // Apply metadata recursively to storage type
- RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
+ // Apply metadata recursively to storage type
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
- // Restore extension type, if the storage type is the same as inferred
- // from the Parquet type
- if (ex_type.storage_type()->Equals(*inferred->field->type())) {
- inferred->field = inferred->field->WithType(origin_type);
+ // Restore extension type, if the storage type is the same as inferred
+ // from the Parquet type
+ if (ex_type.storage_type()->Equals(*inferred->field->type())) {
+ inferred->field = inferred->field->WithType(origin_type);
+ }
}
modified = true;
} else {
+ if (inferred_type->id() == ::arrow::Type::EXTENSION) {
+ const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*inferred_type);
+ if (ex_type.extension_name() ==
+ ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are ENABLED in Parquet.
+ // origin_type is ::arrow::binary()
+ // inferred_type is ::arrow::extension::json()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
Review Comment:
Which is essentially a no-op, isn't it? (`inferred_type` is already `inferred->field`'s type)
So we can just remove this entire code path, it seems.
--
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] ARROW-17255: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2073180907
I'll take a round this week
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #13901: [ARROW-17255] Add JSON canonical extension type
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-1217136744
<!--
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.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/master/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
Opening JIRAs ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1745722573
##########
cpp/src/arrow/extension/json_test.cc:
##########
@@ -0,0 +1,109 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include "arrow/array/validate.h"
+#include "arrow/ipc/test_common.h"
+#include "arrow/record_batch.h"
+#include "arrow/testing/gtest_util.h"
+#include "parquet/exception.h"
+
+namespace arrow {
+
+using arrow::ipc::test::RoundtripBatch;
+using extension::json;
+
+class TestJsonExtensionType : public ::testing::Test {};
+
+std::shared_ptr<Array> ExampleJson(const std::shared_ptr<DataType>& storage_type) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(storage_type, R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])");
+ return ExtensionType::WrapArray(arrow::extension::json(storage_type), arr);
+}
+
+TEST_F(TestJsonExtensionType, JsonRoundtrip) {
+ for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) {
+ std::shared_ptr<Array> ext_arr = ExampleJson(storage_type);
+ auto batch =
+ RecordBatch::Make(schema({field("f0", json(storage_type))}), 8, {ext_arr});
+
+ std::shared_ptr<RecordBatch> read_batch;
+ ASSERT_OK(RoundtripBatch(batch, &read_batch));
+ ASSERT_OK(read_batch->ValidateFull());
+ CompareBatch(*batch, *read_batch, /*compare_metadata*/ true);
+
+ auto read_ext_arr = read_batch->column(0);
+ ASSERT_OK(internal::ValidateUTF8(*read_ext_arr));
+ ASSERT_OK(read_ext_arr->ValidateFull());
+ }
+}
+
+TEST_F(TestJsonExtensionType, InvalidUTF8) {
+ for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) {
+ auto json_type = json(storage_type);
+ auto invalid_input = ArrayFromJSON(storage_type, "[\"Ⱥa\xFFⱭ\", \"Ɽ\xe1\xbdⱤaA\"]");
+ auto ext_arr =
+ ExtensionType::WrapArray(arrow::extension::json(storage_type), invalid_input);
Review Comment:
Let's just reuse the type you created above
```suggestion
auto ext_arr = ExtensionType::WrapArray(json_type, invalid_input);
```
##########
cpp/src/arrow/extension/json.cc:
##########
@@ -0,0 +1,62 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/status.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow::extension {
+
+bool JsonExtensionType::ExtensionEquals(const ExtensionType& other) const {
+ const auto& other_ext = static_cast<const ExtensionType&>(other);
Review Comment:
This isn't doing anything, right? :-)
##########
cpp/src/arrow/extension/json_test.cc:
##########
@@ -0,0 +1,109 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include "arrow/array/validate.h"
+#include "arrow/ipc/test_common.h"
+#include "arrow/record_batch.h"
+#include "arrow/testing/gtest_util.h"
+#include "parquet/exception.h"
+
+namespace arrow {
+
+using arrow::ipc::test::RoundtripBatch;
+using extension::json;
+
+class TestJsonExtensionType : public ::testing::Test {};
+
+std::shared_ptr<Array> ExampleJson(const std::shared_ptr<DataType>& storage_type) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(storage_type, R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])");
+ return ExtensionType::WrapArray(arrow::extension::json(storage_type), arr);
+}
+
+TEST_F(TestJsonExtensionType, JsonRoundtrip) {
+ for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) {
+ std::shared_ptr<Array> ext_arr = ExampleJson(storage_type);
+ auto batch =
+ RecordBatch::Make(schema({field("f0", json(storage_type))}), 8, {ext_arr});
+
+ std::shared_ptr<RecordBatch> read_batch;
+ ASSERT_OK(RoundtripBatch(batch, &read_batch));
+ ASSERT_OK(read_batch->ValidateFull());
+ CompareBatch(*batch, *read_batch, /*compare_metadata*/ true);
+
+ auto read_ext_arr = read_batch->column(0);
+ ASSERT_OK(internal::ValidateUTF8(*read_ext_arr));
+ ASSERT_OK(read_ext_arr->ValidateFull());
+ }
+}
+
+TEST_F(TestJsonExtensionType, InvalidUTF8) {
+ for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) {
+ auto json_type = json(storage_type);
+ auto invalid_input = ArrayFromJSON(storage_type, "[\"Ⱥa\xFFⱭ\", \"Ɽ\xe1\xbdⱤaA\"]");
+ auto ext_arr =
+ ExtensionType::WrapArray(arrow::extension::json(storage_type), invalid_input);
+
+ ASSERT_RAISES_WITH_MESSAGE(Invalid,
+ "Invalid: Invalid UTF8 sequence at string index 0",
+ ext_arr->ValidateFull());
+ ASSERT_RAISES_WITH_MESSAGE(Invalid,
+ "Invalid: Invalid UTF8 sequence at string index 0",
+ arrow::internal::ValidateUTF8(*ext_arr));
+
+ auto batch = RecordBatch::Make(schema({field("f0", json_type)}), 2, {ext_arr});
+ std::shared_ptr<RecordBatch> read_batch;
+ ASSERT_OK(RoundtripBatch(batch, &read_batch));
+ }
+}
+
+class UTF8ExtensionArrayTest : public ::testing::Test {
Review Comment:
This tests morally the same thing as `TestJsonExtensionType` (except for the invalid utf8 part), right? Unless I'm misunderstanding, this one can be removed.
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
+ DCHECK_EQ(inferred_type->id(), ::arrow::Type::BINARY);
+ inferred->field = inferred->field->WithType(origin_type);
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(origin_field, inferred));
+ } else {
+ auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
- // Apply metadata recursively to storage type
- RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
+ // Apply metadata recursively to storage type
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
- // Restore extension type, if the storage type is the same as inferred
- // from the Parquet type
- if (ex_type.storage_type()->Equals(*inferred->field->type())) {
- inferred->field = inferred->field->WithType(origin_type);
+ // Restore extension type, if the storage type is the same as inferred
+ // from the Parquet type
+ if (ex_type.storage_type()->Equals(*inferred->field->type())) {
+ inferred->field = inferred->field->WithType(origin_type);
+ }
}
modified = true;
} else {
+ if (inferred_type->id() == ::arrow::Type::EXTENSION) {
+ const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*inferred_type);
+ if (ex_type.extension_name() ==
+ ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are ENABLED in Parquet.
+ // origin_type is ::arrow::binary()
+ // inferred_type is ::arrow::extension::json()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
Review Comment:
> The example I can think of is a another implementation without canonical types writing this.
Then would we not want to have a JSON array, rather than plain UTF8?
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2310459495
@pitrou I've rebased this on the UUID PR and it's ready for review :)
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733795248
##########
cpp/src/parquet/arrow/schema_internal.cc:
##########
@@ -107,17 +110,22 @@ Result<std::shared_ptr<ArrowType>> MakeArrowTimestamp(const LogicalType& logical
}
}
-Result<std::shared_ptr<ArrowType>> FromByteArray(const LogicalType& logical_type) {
+Result<std::shared_ptr<ArrowType>> FromByteArray(
+ const LogicalType& logical_type, const ArrowReaderProperties& reader_properties) {
switch (logical_type.type()) {
case LogicalType::Type::STRING:
return ::arrow::utf8();
case LogicalType::Type::DECIMAL:
return MakeArrowDecimal(logical_type);
case LogicalType::Type::NONE:
case LogicalType::Type::ENUM:
- case LogicalType::Type::JSON:
case LogicalType::Type::BSON:
return ::arrow::binary();
+ case LogicalType::Type::JSON:
+ if (reader_properties.known_arrow_extensions_enabled()) {
+ return ::arrow::extension::json(::arrow::utf8());
+ }
+ return ::arrow::binary();
Review Comment:
Let's better doing this, thanks! Though I don't so like the idea to use binary, I think `utf8` is better, in parquet-spec, json is "UTF8 column": https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift#L154
I also want to hear what you think @pitrou @wgtmac
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1735943503
##########
cpp/src/arrow/array/validate.cc:
##########
@@ -985,10 +985,23 @@ Status ValidateArrayFull(const Array& array) { return ValidateArrayFull(*array.d
ARROW_EXPORT
Status ValidateUTF8(const ArrayData& data) {
- DCHECK(data.type->id() == Type::STRING || data.type->id() == Type::STRING_VIEW ||
- data.type->id() == Type::LARGE_STRING);
- UTF8DataValidator validator{data};
- return VisitTypeInline(*data.type, &validator);
+ if (data.type->id() == Type::EXTENSION) {
+ const auto& storage_type =
+ checked_pointer_cast<ExtensionType>(data.type)->storage_type();
Review Comment:
```suggestion
const auto& storage_type =
checked_cast<const ExtensionType&>(*data.type).storage_type();
```
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,85 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata = ::arrow::key_value_metadata({}, {});
Review Comment:
Is this useful?
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
+ DCHECK_EQ(inferred_type->id(), ::arrow::Type::BINARY);
+ inferred->field = inferred->field->WithType(origin_type);
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(origin_field, inferred));
+ } else {
+ auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
- // Apply metadata recursively to storage type
- RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
+ // Apply metadata recursively to storage type
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
- // Restore extension type, if the storage type is the same as inferred
- // from the Parquet type
- if (ex_type.storage_type()->Equals(*inferred->field->type())) {
- inferred->field = inferred->field->WithType(origin_type);
+ // Restore extension type, if the storage type is the same as inferred
+ // from the Parquet type
+ if (ex_type.storage_type()->Equals(*inferred->field->type())) {
+ inferred->field = inferred->field->WithType(origin_type);
+ }
}
modified = true;
} else {
+ if (inferred_type->id() == ::arrow::Type::EXTENSION) {
+ const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*inferred_type);
+ if (ex_type.extension_name() ==
+ ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are ENABLED in Parquet.
+ // origin_type is ::arrow::binary()
+ // inferred_type is ::arrow::extension::json()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
Review Comment:
We can just ignore this case here IMHO.
##########
cpp/src/arrow/array/validate.cc:
##########
@@ -985,10 +985,23 @@ Status ValidateArrayFull(const Array& array) { return ValidateArrayFull(*array.d
ARROW_EXPORT
Status ValidateUTF8(const ArrayData& data) {
- DCHECK(data.type->id() == Type::STRING || data.type->id() == Type::STRING_VIEW ||
- data.type->id() == Type::LARGE_STRING);
- UTF8DataValidator validator{data};
- return VisitTypeInline(*data.type, &validator);
+ if (data.type->id() == Type::EXTENSION) {
Review Comment:
Did you add tests for this change?
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -1428,6 +1435,42 @@ TEST_F(TestLargeStringParquetIO, Basics) {
this->RoundTripSingleColumn(large_array, large_array, arrow_properties);
}
+using TestJsonParquetIO = TestParquetIO<::arrow::extension::JsonExtensionType>;
+
+TEST_F(TestJsonParquetIO, JsonExtension) {
+ const char* json = R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])";
+
+ const auto json_type = ::arrow::extension::json();
+ const auto json_string_array = ::arrow::ArrayFromJSON(::arrow::utf8(), json);
+ const auto json_array = ::arrow::ExtensionType::WrapArray(json_type, json_string_array);
+
+ // When the original Arrow schema isn't stored and Arrow extensions are disabled,
+ // LogicalType::JSON is read as utf8.
+ const auto utf8_array = ::arrow::ArrayFromJSON(::arrow::utf8(), json);
Review Comment:
This is the same as `json_string_array`, right?
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -1410,8 +1417,8 @@ using TestLargeStringParquetIO = TestParquetIO<::arrow::LargeStringType>;
TEST_F(TestLargeStringParquetIO, Basics) {
const char* json = R"(["foo", "", null, "bar"])";
- const auto large_type = ::arrow::large_utf8();
- const auto narrow_type = ::arrow::utf8();
+ const auto large_type = ::arrow::large_binary();
+ const auto narrow_type = ::arrow::binary();
Review Comment:
Why the change?
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +990,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ const auto& inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == std::string("arrow.json")) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
+ DCHECK_EQ(inferred_type->id(), ::arrow::Type::STRING);
Review Comment:
This is coming from an arbitrary Parquet file, so we should not use DCHECK. Instead let's make it part of the condition above:
```c++
if (inferred_type->id() == ::arrow::Type::STRING &&
ex_type.extension_name() == std::string("arrow.json")) {
// Schema mismatch. [...]
```
##########
cpp/src/arrow/extension/json_test.cc:
##########
@@ -0,0 +1,58 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include "arrow/ipc/test_common.h"
+#include "arrow/record_batch.h"
+#include "arrow/testing/gtest_util.h"
+
+namespace arrow {
+
+using arrow::ipc::test::RoundtripBatch;
+using extension::json;
+
+class TestJsonExtensionType : public ::testing::Test {};
+
+std::shared_ptr<Array> ExampleJson(const std::shared_ptr<DataType>& storage_type) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(storage_type, R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])");
+ return ExtensionType::WrapArray(arrow::extension::json(storage_type), arr);
+}
+
+TEST_F(TestJsonExtensionType, JsonRoundtrip) {
+ for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) {
+ auto ext_arr = ExampleJson(storage_type);
+
+ auto batch =
+ RecordBatch::Make(schema({field("f0", json(storage_type))}), 8, {ext_arr});
+ std::shared_ptr<RecordBatch> read_batch;
+ RoundtripBatch(batch, &read_batch);
+ ASSERT_OK(read_batch->ValidateFull());
+ CompareBatch(*batch, *read_batch, /*compare_metadata*/ true);
+ }
+}
Review Comment:
Can you add a test where you call `ValidateFull` and `ValidateUTF8` on a JSON array?
Also check with non-valid UTF8.
##########
cpp/src/arrow/extension/json.h:
##########
@@ -0,0 +1,66 @@
+// 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 <stdexcept>
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/visibility.h"
+
+namespace arrow::extension {
+
+/// \brief Concrete type class for variable-size JSON data, utf8-encoded.
+class ARROW_EXPORT JsonExtensionType : public ExtensionType {
+ public:
+ explicit JsonExtensionType(const std::shared_ptr<DataType>& storage_type)
+ : ExtensionType(storage_type), storage_type_(storage_type) {
+ if (storage_type->id() != Type::STRING && storage_type->id() != Type::STRING_VIEW &&
+ storage_type->id() != Type::LARGE_STRING) {
+ throw std::invalid_argument("Invalid storage type for JsonExtensionType: " +
Review Comment:
We don't throw exceptions in Arrow C++. Can you just do the check in `json.cc`?
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,85 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata = ::arrow::key_value_metadata({}, {});
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // If Arrow extensions are enabled, both fields should be treated as json() extension
+ // fields.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled();
+ auto arrow_schema =
+ ::arrow::schema({::arrow::field("json_1", ::arrow::extension::json(), true),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
+ std::shared_ptr<KeyValueMetadata> metadata = ::arrow::key_value_metadata({}, {});
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // Arrow schema has precedence. json_1 should be returned as a json() field even
+ // though extensions are not enabled.
+ std::shared_ptr<KeyValueMetadata> field_metadata =
+ ::arrow::key_value_metadata({"foo", "bar"}, {"biz", "baz"});
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true, field_metadata),
+ ::arrow::field("json_2", UTF8, true)});
+
+ ASSERT_OK_AND_ASSIGN(
+ std::shared_ptr<Buffer> serialized,
+ ::arrow::ipc::SerializeSchema(*arrow_schema, ::arrow::default_memory_pool()));
+ std::string schema_as_string = serialized->ToString();
+ std::string schema_base64 = ::arrow::util::base64_encode(schema_as_string);
+ std::shared_ptr<KeyValueMetadata> metadata =
+ ::arrow::key_value_metadata({"ARROW:schema"}, {schema_base64});
Review Comment:
Please make a helper function for the Arrow schema -> Parquet metadata conversion.
##########
cpp/src/arrow/array/validate.cc:
##########
@@ -985,10 +985,23 @@ Status ValidateArrayFull(const Array& array) { return ValidateArrayFull(*array.d
ARROW_EXPORT
Status ValidateUTF8(const ArrayData& data) {
- DCHECK(data.type->id() == Type::STRING || data.type->id() == Type::STRING_VIEW ||
- data.type->id() == Type::LARGE_STRING);
- UTF8DataValidator validator{data};
- return VisitTypeInline(*data.type, &validator);
+ if (data.type->id() == Type::EXTENSION) {
+ const auto& storage_type =
+ checked_pointer_cast<ExtensionType>(data.type)->storage_type();
+ DCHECK(storage_type->id() == Type::STRING ||
+ storage_type->id() == Type::STRING_VIEW ||
+ storage_type->id() == Type::LARGE_STRING);
+ auto ext_array_data =
+ std::make_shared<ArrayData>(storage_type, data.length, data.buffers,
+ data.child_data, data.null_count, data.offset);
+ UTF8DataValidator validator{*ext_array_data};
Review Comment:
What happens if you simply construct the validator with `data`?
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -1428,6 +1435,42 @@ TEST_F(TestLargeStringParquetIO, Basics) {
this->RoundTripSingleColumn(large_array, large_array, arrow_properties);
}
+using TestJsonParquetIO = TestParquetIO<::arrow::extension::JsonExtensionType>;
+
+TEST_F(TestJsonParquetIO, JsonExtension) {
+ const char* json = R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])";
+
+ const auto json_type = ::arrow::extension::json();
+ const auto json_string_array = ::arrow::ArrayFromJSON(::arrow::utf8(), json);
+ const auto json_array = ::arrow::ExtensionType::WrapArray(json_type, json_string_array);
+
+ // When the original Arrow schema isn't stored and Arrow extensions are disabled,
+ // LogicalType::JSON is read as utf8.
+ const auto utf8_array = ::arrow::ArrayFromJSON(::arrow::utf8(), json);
+ this->RoundTripSingleColumn(json_array, utf8_array, default_arrow_writer_properties());
+
+ // When the original Arrow schema isn't stored and Arrow extensions are enabled,
+ // LogicalType::JSON is read as JsonExtensionType.
+ ::parquet::ArrowReaderProperties reader_properties;
+ reader_properties.set_arrow_extensions_enabled();
+ this->RoundTripSingleColumn(json_array, json_array, default_arrow_writer_properties(),
+ reader_properties);
+
+ // When the original Arrow schema is stored, the stored Arrow type is always respected.
+ const auto arrow_properties =
Review Comment:
Please call this `writer_properties`
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -1387,8 +1394,8 @@ using TestLargeBinaryParquetIO = TestParquetIO<::arrow::LargeBinaryType>;
TEST_F(TestLargeBinaryParquetIO, Basics) {
const char* json = "[\"foo\", \"\", null, \"\xff\"]";
- const auto large_type = ::arrow::large_binary();
- const auto narrow_type = ::arrow::binary();
+ const auto large_type = ::arrow::large_utf8();
+ const auto narrow_type = ::arrow::utf8();
const auto large_array = ::arrow::ArrayFromJSON(large_type, json);
Review Comment:
Why the change?
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,85 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata = ::arrow::key_value_metadata({}, {});
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // If Arrow extensions are enabled, both fields should be treated as json() extension
+ // fields.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled();
+ auto arrow_schema =
+ ::arrow::schema({::arrow::field("json_1", ::arrow::extension::json(), true),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
+ std::shared_ptr<KeyValueMetadata> metadata = ::arrow::key_value_metadata({}, {});
Review Comment:
Same question.
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +990,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ const auto& inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == std::string("arrow.json")) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
Review Comment:
```suggestion
// inferred_type is ::arrow::utf8()
```
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,85 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata = ::arrow::key_value_metadata({}, {});
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // If Arrow extensions are enabled, both fields should be treated as json() extension
+ // fields.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled();
+ auto arrow_schema =
+ ::arrow::schema({::arrow::field("json_1", ::arrow::extension::json(), true),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
+ std::shared_ptr<KeyValueMetadata> metadata = ::arrow::key_value_metadata({}, {});
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // Arrow schema has precedence. json_1 should be returned as a json() field even
+ // though extensions are not enabled.
+ std::shared_ptr<KeyValueMetadata> field_metadata =
+ ::arrow::key_value_metadata({"foo", "bar"}, {"biz", "baz"});
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true, field_metadata),
+ ::arrow::field("json_2", UTF8, true)});
+
+ ASSERT_OK_AND_ASSIGN(
+ std::shared_ptr<Buffer> serialized,
+ ::arrow::ipc::SerializeSchema(*arrow_schema, ::arrow::default_memory_pool()));
+ std::string schema_as_string = serialized->ToString();
+ std::string schema_base64 = ::arrow::util::base64_encode(schema_as_string);
+ std::shared_ptr<KeyValueMetadata> metadata =
+ ::arrow::key_value_metadata({"ARROW:schema"}, {schema_base64});
+
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema, true /* check_metadata */);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // A contrived example. Parquet believes both columns are JSON. Arrow believes json_1
+ // is a JSON column and json_2 is a utf8 column. json_2 should be treated as a
+ // utf8 column even if the get_arrow_extensions_enabled is true.
Review Comment:
It seems like this could only happen if a Arrow writer decided to serialize a regular utf8 array as a Parquet JSON column. Why would it do that?
What do you think @jorisvandenbossche ?
##########
cpp/src/parquet/properties.h:
##########
@@ -941,6 +942,15 @@ class PARQUET_EXPORT ArrowReaderProperties {
return coerce_int96_timestamp_unit_;
}
+ /// Enable Parquet supported Arrow Extension Types.
+ ///
+ /// When enabled, Parquet will use supported Arrow ExtensionTypes in mapping to Arrow
+ /// schema. Currently only arrow::extension::json() extension type is supported. This
+ /// will be used for utf8 columns whose LogicalType is JSON.
+ void set_arrow_extensions_enabled() { arrow_extensions_enabled_ = true; }
+ void set_arrow_extensions_disabled() { arrow_extensions_enabled_ = false; }
+ bool get_arrow_extensions_enabled() const { return arrow_extensions_enabled_; }
+
Review Comment:
Contrast with:
```c++
void set_pre_buffer(bool pre_buffer) { pre_buffer_ = pre_buffer; }
/// Return whether read coalescing is enabled.
bool pre_buffer() const { return pre_buffer_; }
```
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1737557865
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -1428,6 +1435,42 @@ TEST_F(TestLargeStringParquetIO, Basics) {
this->RoundTripSingleColumn(large_array, large_array, arrow_properties);
}
+using TestJsonParquetIO = TestParquetIO<::arrow::extension::JsonExtensionType>;
+
+TEST_F(TestJsonParquetIO, JsonExtension) {
+ const char* json = R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])";
+
+ const auto json_type = ::arrow::extension::json();
+ const auto json_string_array = ::arrow::ArrayFromJSON(::arrow::utf8(), json);
+ const auto json_array = ::arrow::ExtensionType::WrapArray(json_type, json_string_array);
+
+ // When the original Arrow schema isn't stored and Arrow extensions are disabled,
+ // LogicalType::JSON is read as utf8.
+ const auto utf8_array = ::arrow::ArrayFromJSON(::arrow::utf8(), json);
Review Comment:
It is. Removing.
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -1428,6 +1435,42 @@ TEST_F(TestLargeStringParquetIO, Basics) {
this->RoundTripSingleColumn(large_array, large_array, arrow_properties);
}
+using TestJsonParquetIO = TestParquetIO<::arrow::extension::JsonExtensionType>;
+
+TEST_F(TestJsonParquetIO, JsonExtension) {
+ const char* json = R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])";
+
+ const auto json_type = ::arrow::extension::json();
+ const auto json_string_array = ::arrow::ArrayFromJSON(::arrow::utf8(), json);
+ const auto json_array = ::arrow::ExtensionType::WrapArray(json_type, json_string_array);
+
+ // When the original Arrow schema isn't stored and Arrow extensions are disabled,
+ // LogicalType::JSON is read as utf8.
+ const auto utf8_array = ::arrow::ArrayFromJSON(::arrow::utf8(), json);
+ this->RoundTripSingleColumn(json_array, utf8_array, default_arrow_writer_properties());
+
+ // When the original Arrow schema isn't stored and Arrow extensions are enabled,
+ // LogicalType::JSON is read as JsonExtensionType.
+ ::parquet::ArrowReaderProperties reader_properties;
+ reader_properties.set_arrow_extensions_enabled();
+ this->RoundTripSingleColumn(json_array, json_array, default_arrow_writer_properties(),
+ reader_properties);
+
+ // When the original Arrow schema is stored, the stored Arrow type is always respected.
+ const auto arrow_properties =
Review Comment:
Changed.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1737558825
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +990,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ const auto& inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == std::string("arrow.json")) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
+ DCHECK_EQ(inferred_type->id(), ::arrow::Type::STRING);
Review Comment:
Moved.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2321594469
@pitrou @mapleFU I've addressed your comments, could you please do another pass?
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1735934227
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
+ DCHECK_EQ(inferred_type->id(), ::arrow::Type::BINARY);
+ inferred->field = inferred->field->WithType(origin_type);
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(origin_field, inferred));
+ } else {
+ auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
- // Apply metadata recursively to storage type
- RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
+ // Apply metadata recursively to storage type
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
- // Restore extension type, if the storage type is the same as inferred
- // from the Parquet type
- if (ex_type.storage_type()->Equals(*inferred->field->type())) {
- inferred->field = inferred->field->WithType(origin_type);
+ // Restore extension type, if the storage type is the same as inferred
+ // from the Parquet type
+ if (ex_type.storage_type()->Equals(*inferred->field->type())) {
+ inferred->field = inferred->field->WithType(origin_type);
+ }
}
modified = true;
} else {
+ if (inferred_type->id() == ::arrow::Type::EXTENSION) {
+ const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*inferred_type);
+ if (ex_type.extension_name() ==
+ ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are ENABLED in Parquet.
+ // origin_type is ::arrow::binary()
+ // inferred_type is ::arrow::extension::json()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
Review Comment:
I'm not sure what to do here @pitrou. What would the preferred behavior be?
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1736036117
##########
cpp/src/parquet/arrow/schema_internal.cc:
##########
@@ -107,17 +110,24 @@ Result<std::shared_ptr<ArrowType>> MakeArrowTimestamp(const LogicalType& logical
}
}
-Result<std::shared_ptr<ArrowType>> FromByteArray(const LogicalType& logical_type) {
+Result<std::shared_ptr<ArrowType>> FromByteArray(
+ const LogicalType& logical_type, const ArrowReaderProperties& reader_properties) {
switch (logical_type.type()) {
case LogicalType::Type::STRING:
return ::arrow::utf8();
case LogicalType::Type::DECIMAL:
return MakeArrowDecimal(logical_type);
case LogicalType::Type::NONE:
case LogicalType::Type::ENUM:
- case LogicalType::Type::JSON:
case LogicalType::Type::BSON:
return ::arrow::binary();
+ case LogicalType::Type::JSON:
+ if (reader_properties.get_arrow_extensions_enabled()) {
+ return ::arrow::extension::json(::arrow::utf8());
+ }
+ // When the original Arrow schema isn't stored and Arrow extensions are disabled,
+ // LogicalType::JSON is read as utf8().
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1752986047
##########
cpp/src/parquet/properties.h:
##########
@@ -941,6 +942,18 @@ class PARQUET_EXPORT ArrowReaderProperties {
return coerce_int96_timestamp_unit_;
}
+ /// Enable Parquet supported Arrow ExtensionTypes.
+ ///
+ /// When enabled, Parquet will use supported Arrow ExtensionTypes by mapping correctly
+ /// mapping them to Arrow types at read time. Currently only arrow::extension::json()
+ /// extension type is supported. Columns whose LogicalType is JSON will be interpreted
+ /// as arrow::extension::json() ExtensionType with storage type utf8, large_utf8 or
Review Comment:
Yes, I think this is a more general question for all string types.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2343228845
Thanks for the reviews all!
--
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] ARROW-17255: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2060830634
Yes, please make a separate PR for the format proposal.
--
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
[GitHub] [arrow] pradeepg26 commented on a diff in pull request #13901: ARROW-17255: [C++][Parquet] Add JSON canonical extension type
Posted by GitBox <gi...@apache.org>.
pradeepg26 commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1053631966
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -1410,6 +1417,43 @@ TEST_F(TestLargeStringParquetIO, Basics) {
this->RoundTripSingleColumn(large_array, large_array, arrow_properties);
}
+using TestJsonParquetIO = TestParquetIO<::arrow::extension::JsonExtensionType>;
+
+TEST_F(TestJsonParquetIO, JsonExtension) {
+ const char* json = R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])";
+
+ const auto json_type = ::arrow::extension::json();
+ const auto json_string_array = ::arrow::ArrayFromJSON(::arrow::utf8(), json);
+ const auto json_array = ::arrow::ExtensionType::WrapArray(json_type, json_string_array);
+
+ // When the original Arrow schema isn't stored and Arrow extensions are disabled,
+ // LogicalType::JSON is read as Binary.
+ const auto binary_array = ::arrow::ArrayFromJSON(::arrow::binary(), json);
Review Comment:
This is to preserve the existing behavior.
We currently read `LogicalType::JSON` as `arrow::binary`. So, if there is no schema information and the extensions are disabled, then we use the current behavior.
--
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] ARROW-17255: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "progger-dev (via GitHub)" <gi...@apache.org>.
progger-dev commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2052502373
I expected someone else at BigQuery to take over this work. I would love to finish this PR, but I don't think I'll have time for a while.
--
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] ARROW-17255: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2060736393
I would now perhaps split out the format definition part (docs) and call a vote on the ML.
@pitrou any thoughs at this point?
--
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] ARROW-17255: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1567786555
##########
cpp/src/arrow/extension_type_test.cc:
##########
@@ -26,6 +26,7 @@
#include "arrow/array/array_nested.h"
#include "arrow/array/util.h"
+#include "arrow/extension/json.h"
Review Comment:
Removed.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1753556684
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,88 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+Status ArrowSchemaToParquetMetadata(std::shared_ptr<::arrow::Schema>& arrow_schema,
+ std::shared_ptr<KeyValueMetadata>& metadata) {
+ ARROW_ASSIGN_OR_RAISE(
+ std::shared_ptr<Buffer> serialized,
+ ::arrow::ipc::SerializeSchema(*arrow_schema, ::arrow::default_memory_pool()));
+ std::string schema_as_string = serialized->ToString();
+ std::string schema_base64 = ::arrow::util::base64_encode(schema_as_string);
+ metadata = ::arrow::key_value_metadata({"ARROW:schema"}, {schema_base64});
+ return Status::OK();
+}
+
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // If Arrow extensions are enabled, both fields should be treated as json() extension
+ // fields.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true),
+ ::arrow::field("json_2", ::arrow::extension::json(::arrow::large_utf8()),
+ true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // Parquet logical type has precedence. Both json_1 and json_2 should be returned
+ // as a json() field even though extensions are not enabled.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(false);
+ std::shared_ptr<KeyValueMetadata> field_metadata =
+ ::arrow::key_value_metadata({"foo", "bar"}, {"biz", "baz"});
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true, field_metadata),
+ ::arrow::field("json_2", ::arrow::extension::json(::arrow::large_utf8()),
+ true)});
+
+ std::shared_ptr<KeyValueMetadata> metadata;
+ ASSERT_OK(ArrowSchemaToParquetMetadata(arrow_schema, metadata));
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema, true /* check_metadata */);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // A contrived example. Parquet believes both columns are JSON. Arrow believes json_1
+ // is a JSON column and json_2 is an utf8 column. json_2 should be treated as an
+ // JSON column even if arrow extensions are enabled.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ std::shared_ptr<KeyValueMetadata> field_metadata =
+ ::arrow::key_value_metadata({"foo", "bar"}, {"biz", "baz"});
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true, field_metadata),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
Review Comment:
Changed.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733419899
##########
cpp/src/arrow/extension/json.h:
##########
@@ -0,0 +1,65 @@
+// 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 <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
Review Comment:
Removed and switched to `throw std::invalid_argument` to throw when wrong type is supplied to the `JsonExtensionType` constructor.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733439871
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
+ DCHECK_EQ(inferred_type->id(), ::arrow::Type::BINARY);
+ inferred->field = inferred->field->WithType(origin_type);
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(origin_field, inferred));
+ } else {
+ auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
- // Apply metadata recursively to storage type
- RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
+ // Apply metadata recursively to storage type
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
- // Restore extension type, if the storage type is the same as inferred
- // from the Parquet type
- if (ex_type.storage_type()->Equals(*inferred->field->type())) {
- inferred->field = inferred->field->WithType(origin_type);
+ // Restore extension type, if the storage type is the same as inferred
+ // from the Parquet type
+ if (ex_type.storage_type()->Equals(*inferred->field->type())) {
+ inferred->field = inferred->field->WithType(origin_type);
+ }
}
modified = true;
} else {
+ if (inferred_type->id() == ::arrow::Type::EXTENSION) {
+ const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*inferred_type);
+ if (ex_type.extension_name() ==
+ ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are ENABLED in Parquet.
+ // origin_type is ::arrow::binary()
+ // inferred_type is ::arrow::extension::json()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
Review Comment:
Perhaps we can raise for 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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1735815423
##########
cpp/src/arrow/extension/json.cc:
##########
@@ -0,0 +1,62 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/status.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow::extension {
+
+bool JsonExtensionType::ExtensionEquals(const ExtensionType& other) const {
+ const auto& other_ext = static_cast<const ExtensionType&>(other);
+ return other_ext.extension_name() == this->extension_name();
+}
+
+Result<std::shared_ptr<DataType>> JsonExtensionType::Deserialize(
+ std::shared_ptr<DataType> storage_type, const std::string& serialized) const {
+ if (!serialized.empty()) {
+ return Status::Invalid("Unexpected serialized metadata: '", serialized, "'");
+ }
Review Comment:
Indeed! Removed this check.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1735935523
##########
cpp/src/parquet/arrow/schema_internal.h:
##########
@@ -28,18 +29,19 @@ namespace parquet::arrow {
using ::arrow::Result;
-Result<std::shared_ptr<::arrow::DataType>> FromByteArray(const LogicalType& logical_type);
+Result<std::shared_ptr<::arrow::DataType>> FromByteArray(const LogicalType& logical_type,
+ bool use_known_arrow_extensions);
Result<std::shared_ptr<::arrow::DataType>> FromFLBA(const LogicalType& logical_type,
int32_t physical_length);
Result<std::shared_ptr<::arrow::DataType>> FromInt32(const LogicalType& logical_type);
Result<std::shared_ptr<::arrow::DataType>> FromInt64(const LogicalType& logical_type);
Result<std::shared_ptr<::arrow::DataType>> GetArrowType(
Type::type physical_type, const LogicalType& logical_type, int type_length,
- ::arrow::TimeUnit::type int96_arrow_time_unit = ::arrow::TimeUnit::NANO);
+ const ArrowReaderProperties& reader_properties);
Review Comment:
I don't really understand @mapleFU 's suggestion. It sounds unrelated to 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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1753438030
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,88 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+Status ArrowSchemaToParquetMetadata(std::shared_ptr<::arrow::Schema>& arrow_schema,
+ std::shared_ptr<KeyValueMetadata>& metadata) {
+ ARROW_ASSIGN_OR_RAISE(
+ std::shared_ptr<Buffer> serialized,
+ ::arrow::ipc::SerializeSchema(*arrow_schema, ::arrow::default_memory_pool()));
+ std::string schema_as_string = serialized->ToString();
+ std::string schema_base64 = ::arrow::util::base64_encode(schema_as_string);
+ metadata = ::arrow::key_value_metadata({"ARROW:schema"}, {schema_base64});
+ return Status::OK();
+}
+
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // If Arrow extensions are enabled, both fields should be treated as json() extension
+ // fields.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true),
+ ::arrow::field("json_2", ::arrow::extension::json(::arrow::large_utf8()),
+ true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // Parquet logical type has precedence. Both json_1 and json_2 should be returned
+ // as a json() field even though extensions are not enabled.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(false);
+ std::shared_ptr<KeyValueMetadata> field_metadata =
+ ::arrow::key_value_metadata({"foo", "bar"}, {"biz", "baz"});
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true, field_metadata),
+ ::arrow::field("json_2", ::arrow::extension::json(::arrow::large_utf8()),
+ true)});
+
+ std::shared_ptr<KeyValueMetadata> metadata;
+ ASSERT_OK(ArrowSchemaToParquetMetadata(arrow_schema, metadata));
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema, true /* check_metadata */);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // A contrived example. Parquet believes both columns are JSON. Arrow believes json_1
+ // is a JSON column and json_2 is an utf8 column. json_2 should be treated as an
+ // JSON column even if arrow extensions are enabled.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ std::shared_ptr<KeyValueMetadata> field_metadata =
+ ::arrow::key_value_metadata({"foo", "bar"}, {"biz", "baz"});
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true, field_metadata),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
Review Comment:
large_utf8 here? (does it get restored as large_utf8 as well?)
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,88 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+Status ArrowSchemaToParquetMetadata(std::shared_ptr<::arrow::Schema>& arrow_schema,
+ std::shared_ptr<KeyValueMetadata>& metadata) {
+ ARROW_ASSIGN_OR_RAISE(
+ std::shared_ptr<Buffer> serialized,
+ ::arrow::ipc::SerializeSchema(*arrow_schema, ::arrow::default_memory_pool()));
+ std::string schema_as_string = serialized->ToString();
+ std::string schema_base64 = ::arrow::util::base64_encode(schema_as_string);
+ metadata = ::arrow::key_value_metadata({"ARROW:schema"}, {schema_base64});
+ return Status::OK();
+}
+
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // If Arrow extensions are enabled, both fields should be treated as json() extension
+ // fields.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true),
+ ::arrow::field("json_2", ::arrow::extension::json(::arrow::large_utf8()),
+ true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // Parquet logical type has precedence. Both json_1 and json_2 should be returned
+ // as a json() field even though extensions are not enabled.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(false);
+ std::shared_ptr<KeyValueMetadata> field_metadata =
+ ::arrow::key_value_metadata({"foo", "bar"}, {"biz", "baz"});
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true, field_metadata),
+ ::arrow::field("json_2", ::arrow::extension::json(::arrow::large_utf8()),
+ true)});
+
+ std::shared_ptr<KeyValueMetadata> metadata;
+ ASSERT_OK(ArrowSchemaToParquetMetadata(arrow_schema, metadata));
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema, true /* check_metadata */);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // A contrived example. Parquet believes both columns are JSON. Arrow believes json_1
Review Comment:
Can you update the comment?
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,88 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+Status ArrowSchemaToParquetMetadata(std::shared_ptr<::arrow::Schema>& arrow_schema,
+ std::shared_ptr<KeyValueMetadata>& metadata) {
+ ARROW_ASSIGN_OR_RAISE(
+ std::shared_ptr<Buffer> serialized,
+ ::arrow::ipc::SerializeSchema(*arrow_schema, ::arrow::default_memory_pool()));
+ std::string schema_as_string = serialized->ToString();
+ std::string schema_base64 = ::arrow::util::base64_encode(schema_as_string);
+ metadata = ::arrow::key_value_metadata({"ARROW:schema"}, {schema_base64});
+ return Status::OK();
+}
+
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // If Arrow extensions are enabled, both fields should be treated as json() extension
+ // fields.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true),
+ ::arrow::field("json_2", ::arrow::extension::json(::arrow::large_utf8()),
+ true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // Parquet logical type has precedence. Both json_1 and json_2 should be returned
+ // as a json() field even though extensions are not enabled.
Review Comment:
```suggestion
// Both json_1 and json_2 should be returned as a json() field
// even though extensions are not enabled.
```
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1740924412
##########
cpp/src/arrow/extension/json_test.cc:
##########
@@ -0,0 +1,91 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include "arrow/array/validate.h"
+#include "arrow/ipc/test_common.h"
+#include "arrow/record_batch.h"
+#include "arrow/testing/gtest_util.h"
+#include "parquet/exception.h"
+
+namespace arrow {
+
+using arrow::ipc::test::RoundtripBatch;
+using extension::json;
+
+class TestJsonExtensionType : public ::testing::Test {};
+
+std::shared_ptr<Array> ExampleJson(const std::shared_ptr<DataType>& storage_type) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(storage_type, R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])");
+ return ExtensionType::WrapArray(arrow::extension::json(storage_type), arr);
+}
+
+static std::shared_ptr<Array> ExampleJsonInvalidUTF8(
+ const std::shared_ptr<DataType>& storage_type) {
+ return ArrayFromJSON(storage_type,
+ "["
+ R"(
Review Comment:
Can you put the `R(` around the entire string? It would be slightly easier to read (as above).
##########
cpp/src/arrow/extension/json_test.cc:
##########
@@ -0,0 +1,91 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include "arrow/array/validate.h"
+#include "arrow/ipc/test_common.h"
+#include "arrow/record_batch.h"
+#include "arrow/testing/gtest_util.h"
+#include "parquet/exception.h"
+
+namespace arrow {
+
+using arrow::ipc::test::RoundtripBatch;
+using extension::json;
+
+class TestJsonExtensionType : public ::testing::Test {};
+
+std::shared_ptr<Array> ExampleJson(const std::shared_ptr<DataType>& storage_type) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(storage_type, R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])");
+ return ExtensionType::WrapArray(arrow::extension::json(storage_type), arr);
+}
+
+static std::shared_ptr<Array> ExampleJsonInvalidUTF8(
+ const std::shared_ptr<DataType>& storage_type) {
+ return ArrayFromJSON(storage_type,
+ "["
+ R"(
+ "Hi",
+ "olá mundo",
+ "你好世界",
+ "",
+ )"
+ "\"\xa0\xa1\""
+ "]");
+}
+
+TEST_F(TestJsonExtensionType, JsonRoundtrip) {
+ for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) {
+ std::shared_ptr<Array> ext_arr = ExampleJson(storage_type);
+ auto batch =
+ RecordBatch::Make(schema({field("f0", json(storage_type))}), 8, {ext_arr});
+
+ std::shared_ptr<RecordBatch> read_batch;
+ ASSERT_OK(RoundtripBatch(batch, &read_batch));
+ ASSERT_OK(read_batch->ValidateFull());
+ CompareBatch(*batch, *read_batch, /*compare_metadata*/ true);
+
+ auto read_ext_arr = read_batch->column(0);
+ ASSERT_OK(internal::ValidateUTF8(*read_ext_arr));
+ ASSERT_OK(read_ext_arr->ValidateFull());
+ }
+}
+
+TEST_F(TestJsonExtensionType, InvalidUTF8) {
+ for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) {
+ std::shared_ptr<Array> ext_arr = ExampleJsonInvalidUTF8(storage_type);
+ auto batch =
+ RecordBatch::Make(schema({field("f0", json(storage_type))}), 8, {ext_arr});
+
+ std::shared_ptr<RecordBatch> read_batch;
+ ASSERT_RAISES_WITH_MESSAGE(IOError,
+ "IOError: Array length did not match record batch length",
+ RoundtripBatch(batch, &read_batch));
Review Comment:
I don't understand this test. This should ideally check that `ext_arr->ValidateFull()` returns an error, and ditto for `ValidateUTF8`.
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
+ DCHECK_EQ(inferred_type->id(), ::arrow::Type::BINARY);
+ inferred->field = inferred->field->WithType(origin_type);
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(origin_field, inferred));
+ } else {
+ auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
- // Apply metadata recursively to storage type
- RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
+ // Apply metadata recursively to storage type
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
- // Restore extension type, if the storage type is the same as inferred
- // from the Parquet type
- if (ex_type.storage_type()->Equals(*inferred->field->type())) {
- inferred->field = inferred->field->WithType(origin_type);
+ // Restore extension type, if the storage type is the same as inferred
+ // from the Parquet type
+ if (ex_type.storage_type()->Equals(*inferred->field->type())) {
+ inferred->field = inferred->field->WithType(origin_type);
+ }
}
modified = true;
} else {
+ if (inferred_type->id() == ::arrow::Type::EXTENSION) {
+ const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*inferred_type);
+ if (ex_type.extension_name() ==
+ ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are ENABLED in Parquet.
+ // origin_type is ::arrow::binary()
+ // inferred_type is ::arrow::extension::json()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
Review Comment:
This would only happen if a Parquet writer wrote a Arrow utf8 array (_not_ JSON) as a JSON Parquet logical type. Why would it do that?
##########
cpp/src/arrow/util/utf8_util_test.cc:
##########
@@ -562,5 +564,30 @@ TEST(UTF8Length, Basics) {
ASSERT_EQ(length("\xf0\x9f\x99\x8c"), 1);
}
+class UTF8ExtensionArrayTest : public ::testing::Test {
+ public:
+ static std::shared_ptr<Array> ExampleJson(
+ const std::shared_ptr<DataType>& storage_type) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(storage_type, R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])");
+ return ExtensionType::WrapArray(arrow::extension::json(storage_type), arr);
+ }
+};
+
+TEST_F(UTF8ExtensionArrayTest, JSONExtensionType) {
Review Comment:
IMHO, this should simply go in `json_test.cc`.
##########
cpp/src/arrow/extension/json.h:
##########
@@ -0,0 +1,60 @@
+// 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 <stdexcept>
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/visibility.h"
+
+namespace arrow::extension {
+
+/// \brief Concrete type class for variable-size JSON data, utf8-encoded.
+class ARROW_EXPORT JsonExtensionType : public ExtensionType {
+ public:
+ explicit JsonExtensionType(const std::shared_ptr<DataType>& storage_type)
+ : ExtensionType(storage_type), storage_type_(storage_type) {}
+
+ std::string extension_name() const override { return "arrow.json"; }
+
+ std::string ToString(bool show_metadata = false) const override {
Review Comment:
I don't think you need to override this at all, `ExtensionType::ToString` already does the required thing.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1741225739
##########
cpp/src/arrow/extension/json_test.cc:
##########
@@ -0,0 +1,91 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include "arrow/array/validate.h"
+#include "arrow/ipc/test_common.h"
+#include "arrow/record_batch.h"
+#include "arrow/testing/gtest_util.h"
+#include "parquet/exception.h"
+
+namespace arrow {
+
+using arrow::ipc::test::RoundtripBatch;
+using extension::json;
+
+class TestJsonExtensionType : public ::testing::Test {};
+
+std::shared_ptr<Array> ExampleJson(const std::shared_ptr<DataType>& storage_type) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(storage_type, R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])");
+ return ExtensionType::WrapArray(arrow::extension::json(storage_type), arr);
+}
+
+static std::shared_ptr<Array> ExampleJsonInvalidUTF8(
+ const std::shared_ptr<DataType>& storage_type) {
+ return ArrayFromJSON(storage_type,
+ "["
+ R"(
+ "Hi",
+ "olá mundo",
+ "你好世界",
+ "",
+ )"
+ "\"\xa0\xa1\""
+ "]");
+}
+
+TEST_F(TestJsonExtensionType, JsonRoundtrip) {
+ for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) {
+ std::shared_ptr<Array> ext_arr = ExampleJson(storage_type);
+ auto batch =
+ RecordBatch::Make(schema({field("f0", json(storage_type))}), 8, {ext_arr});
+
+ std::shared_ptr<RecordBatch> read_batch;
+ ASSERT_OK(RoundtripBatch(batch, &read_batch));
+ ASSERT_OK(read_batch->ValidateFull());
+ CompareBatch(*batch, *read_batch, /*compare_metadata*/ true);
+
+ auto read_ext_arr = read_batch->column(0);
+ ASSERT_OK(internal::ValidateUTF8(*read_ext_arr));
+ ASSERT_OK(read_ext_arr->ValidateFull());
+ }
+}
+
+TEST_F(TestJsonExtensionType, InvalidUTF8) {
+ for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) {
+ std::shared_ptr<Array> ext_arr = ExampleJsonInvalidUTF8(storage_type);
+ auto batch =
+ RecordBatch::Make(schema({field("f0", json(storage_type))}), 8, {ext_arr});
+
+ std::shared_ptr<RecordBatch> read_batch;
+ ASSERT_RAISES_WITH_MESSAGE(IOError,
+ "IOError: Array length did not match record batch length",
+ RoundtripBatch(batch, &read_batch));
Review Comment:
Indeed, that was not really testing anything relevant to this PR. Changed to:
```cpp
auto json_type = json(storage_type);
auto invalid_input = ArrayFromJSON(storage_type, "[\"Ⱥa\xFFⱭ\", \"Ɽ\xe1\xbdⱤaA\"]");
auto ext_arr =
ExtensionType::WrapArray(arrow::extension::json(storage_type), invalid_input);
ASSERT_RAISES_WITH_MESSAGE(Invalid,
"Invalid: Invalid UTF8 sequence at string index 0",
ext_arr->ValidateFull());
ASSERT_RAISES_WITH_MESSAGE(Invalid,
"Invalid: Invalid UTF8 sequence at string index 0",
arrow::internal::ValidateUTF8(*ext_arr));
```
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733328189
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
Review Comment:
```suggestion
const auto& inferred_type = inferred->field->type();
```
--
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] ARROW-17255: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1567784002
##########
cpp/src/arrow/extension/CMakeLists.txt:
##########
@@ -15,10 +15,19 @@
# specific language governing permissions and limitations
# under the License.
-add_arrow_test(test
- SOURCES
- fixed_shape_tensor_test.cc
- PREFIX
- "arrow-fixed-shape-tensor")
+if(ARROW_JSON)
+ add_arrow_test(test
+ SOURCES
+ fixed_shape_tensor_test.cc
+ json_test.cc
+ PREFIX
+ "arrow-canonical-extensions")
+else()
+ add_arrow_test(test
+ SOURCES
+ json_test.cc
Review Comment:
`fixed_shape_tensor` requires rapidjson. I'm actually not sure if `ARROW_JSON` controls if rapidjson gets linked or not, will test.
--
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] ARROW-17255: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1567721665
##########
cpp/src/arrow/extension/json.h:
##########
@@ -0,0 +1,60 @@
+// 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 <memory>
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/visibility.h"
+
+namespace arrow {
+namespace extension {
+
+/// \brief Concrete type class for variable-size JSON data, utf8-encoded.
+class ARROW_EXPORT JsonExtensionType : public ExtensionType {
+ public:
+ JsonExtensionType() : ExtensionType(::arrow::utf8()) {}
Review Comment:
(quick question: could this be large utf8?)
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733312779
##########
cpp/src/parquet/arrow/schema_internal.cc:
##########
@@ -107,17 +110,22 @@ Result<std::shared_ptr<ArrowType>> MakeArrowTimestamp(const LogicalType& logical
}
}
-Result<std::shared_ptr<ArrowType>> FromByteArray(const LogicalType& logical_type) {
+Result<std::shared_ptr<ArrowType>> FromByteArray(
+ const LogicalType& logical_type, const ArrowReaderProperties& reader_properties) {
switch (logical_type.type()) {
case LogicalType::Type::STRING:
return ::arrow::utf8();
case LogicalType::Type::DECIMAL:
return MakeArrowDecimal(logical_type);
case LogicalType::Type::NONE:
case LogicalType::Type::ENUM:
- case LogicalType::Type::JSON:
case LogicalType::Type::BSON:
return ::arrow::binary();
+ case LogicalType::Type::JSON:
+ if (reader_properties.known_arrow_extensions_enabled()) {
+ return ::arrow::extension::json(::arrow::utf8());
+ }
+ return ::arrow::binary();
Review Comment:
I'm a bit curious here, would this a binary or a string? If it's a binary for back-compatible, can we add a comment here?
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1734355742
##########
cpp/src/arrow/extension/json.cc:
##########
@@ -0,0 +1,62 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/status.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow::extension {
+
+bool JsonExtensionType::ExtensionEquals(const ExtensionType& other) const {
+ const auto& other_ext = static_cast<const ExtensionType&>(other);
+ return other_ext.extension_name() == this->extension_name();
+}
+
+Result<std::shared_ptr<DataType>> JsonExtensionType::Deserialize(
+ std::shared_ptr<DataType> storage_type, const std::string& serialized) const {
+ if (!serialized.empty()) {
+ return Status::Invalid("Unexpected serialized metadata: '", serialized, "'");
+ }
Review Comment:
Good point @jorisvandenbossche !
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2341939466
Thanks for reviews @pitrou and @wgtmac. I've addressed the comments, please proceed with the review.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2343510069
Created https://github.com/apache/arrow/issues/44066 for the Python wrapper.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1737540417
##########
cpp/src/parquet/properties.h:
##########
@@ -941,6 +942,15 @@ class PARQUET_EXPORT ArrowReaderProperties {
return coerce_int96_timestamp_unit_;
}
+ /// Enable Parquet supported Arrow Extension Types.
+ ///
+ /// When enabled, Parquet will use supported Arrow ExtensionTypes in mapping to Arrow
+ /// schema. Currently only arrow::extension::json() extension type is supported. This
+ /// will be used for utf8 columns whose LogicalType is JSON.
+ void set_arrow_extensions_enabled() { arrow_extensions_enabled_ = true; }
+ void set_arrow_extensions_disabled() { arrow_extensions_enabled_ = false; }
+ bool get_arrow_extensions_enabled() const { return arrow_extensions_enabled_; }
Review Comment:
Changed.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1737557423
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -1387,8 +1394,8 @@ using TestLargeBinaryParquetIO = TestParquetIO<::arrow::LargeBinaryType>;
TEST_F(TestLargeBinaryParquetIO, Basics) {
const char* json = "[\"foo\", \"\", null, \"\xff\"]";
- const auto large_type = ::arrow::large_binary();
- const auto narrow_type = ::arrow::binary();
+ const auto large_type = ::arrow::large_utf8();
+ const auto narrow_type = ::arrow::utf8();
const auto large_array = ::arrow::ArrayFromJSON(large_type, json);
Review Comment:
Just a mistake. Reverting.
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -1410,8 +1417,8 @@ using TestLargeStringParquetIO = TestParquetIO<::arrow::LargeStringType>;
TEST_F(TestLargeStringParquetIO, Basics) {
const char* json = R"(["foo", "", null, "bar"])";
- const auto large_type = ::arrow::large_utf8();
- const auto narrow_type = ::arrow::utf8();
+ const auto large_type = ::arrow::large_binary();
+ const auto narrow_type = ::arrow::binary();
Review Comment:
Mistake. Reverting.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1737543363
##########
cpp/src/arrow/array/validate.cc:
##########
@@ -985,10 +985,23 @@ Status ValidateArrayFull(const Array& array) { return ValidateArrayFull(*array.d
ARROW_EXPORT
Status ValidateUTF8(const ArrayData& data) {
- DCHECK(data.type->id() == Type::STRING || data.type->id() == Type::STRING_VIEW ||
- data.type->id() == Type::LARGE_STRING);
- UTF8DataValidator validator{data};
- return VisitTypeInline(*data.type, &validator);
+ if (data.type->id() == Type::EXTENSION) {
Review Comment:
Added a `TEST_F(UTF8ExtensionArrayTest, JSONExtensionType)` test to `utf8_util_test.cc`.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1734032226
##########
cpp/src/arrow/extension/json.cc:
##########
@@ -0,0 +1,62 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/status.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow::extension {
+
+bool JsonExtensionType::ExtensionEquals(const ExtensionType& other) const {
+ const auto& other_ext = static_cast<const ExtensionType&>(other);
+ return other_ext.extension_name() == this->extension_name();
+}
+
+Result<std::shared_ptr<DataType>> JsonExtensionType::Deserialize(
+ std::shared_ptr<DataType> storage_type, const std::string& serialized) const {
+ if (!serialized.empty()) {
+ return Status::Invalid("Unexpected serialized metadata: '", serialized, "'");
+ }
Review Comment:
Should we allow but ignore non-empty metadata, for future compatibility? The spec says _"In the future, additional fields may be added, but they are not required to interpret the array."_
--
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
[GitHub] [arrow] tustvold commented on a diff in pull request #13901: ARROW-17255: [C++][Parquet] Add JSON canonical extension type
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1053633363
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -1410,6 +1417,43 @@ TEST_F(TestLargeStringParquetIO, Basics) {
this->RoundTripSingleColumn(large_array, large_array, arrow_properties);
}
+using TestJsonParquetIO = TestParquetIO<::arrow::extension::JsonExtensionType>;
+
+TEST_F(TestJsonParquetIO, JsonExtension) {
+ const char* json = R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])";
+
+ const auto json_type = ::arrow::extension::json();
+ const auto json_string_array = ::arrow::ArrayFromJSON(::arrow::utf8(), json);
+ const auto json_array = ::arrow::ExtensionType::WrapArray(json_type, json_string_array);
+
+ // When the original Arrow schema isn't stored and Arrow extensions are disabled,
+ // LogicalType::JSON is read as Binary.
+ const auto binary_array = ::arrow::ArrayFromJSON(::arrow::binary(), json);
Review Comment:
Do you foresee an issue with my changing the Rust parquet reader to infer as UTF-8 in https://github.com/apache/arrow-rs/pull/3376, at least until such a time as this extension type is stabilised?
--
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
[GitHub] [arrow] pitrou commented on pull request #13901: ARROW-17255: [C++][Parquet] Add JSON canonical extension type
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-1218262516
We don't have a way to standardize extension types yet so I posted a proposal of how to do it:
https://lists.apache.org/thread/qxc1g7h9ow79qt6r7sqtgbj8mdbdgnhb
Until that is decided I think this PR should be converted to draft. Does that sound ok @progger-dev ?
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733142900
##########
cpp/src/arrow/extension/json.h:
##########
@@ -0,0 +1,65 @@
+// 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 <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
Review Comment:
`logging.h` should normally not be included in public headers.
##########
cpp/src/arrow/extension/json.h:
##########
@@ -0,0 +1,65 @@
+// 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 <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/visibility.h"
+
+namespace arrow::extension {
+
+/// \brief Concrete type class for variable-size JSON data, utf8-encoded.
+class ARROW_EXPORT JsonExtensionType : public ExtensionType {
+ public:
+ explicit JsonExtensionType(const std::shared_ptr<DataType>& storage_type)
+ : ExtensionType(storage_type), storage_type_(storage_type) {
+ ARROW_CHECK(storage_type->Equals(utf8()) || storage_type->Equals(large_utf8()) ||
Review Comment:
Can instead use a more light-weight id check (`storage_type->id() == Type::STRING`).
##########
cpp/src/arrow/extension/json_test.cc:
##########
@@ -0,0 +1,57 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include "arrow/ipc/test_common.h"
+#include "arrow/record_batch.h"
+#include "arrow/testing/gtest_util.h"
+
+namespace arrow {
+
+using arrow::ipc::test::RoundtripBatch;
+using extension::json;
+
+class TestJsonExtensionType : public ::testing::Test {};
+
+std::shared_ptr<Array> ExampleJson(const std::shared_ptr<DataType>& storage_type) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(storage_type, R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])");
+ return ExtensionType::WrapArray(arrow::extension::json(storage_type), arr);
+}
+
+TEST_F(TestJsonExtensionType, JsonRoundtrip) {
+ for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) {
+ auto ext_arr = ExampleJson(storage_type);
+
+ auto batch =
+ RecordBatch::Make(schema({field("f0", json(storage_type))}), 8, {ext_arr});
+ std::shared_ptr<RecordBatch> read_batch;
+ RoundtripBatch(batch, &read_batch);
+ CompareBatch(*batch, *read_batch, false /* compare_metadata */);
Review Comment:
Why leave `compare_metadata` to `false`?
##########
cpp/src/parquet/properties.h:
##########
@@ -941,6 +942,15 @@ class PARQUET_EXPORT ArrowReaderProperties {
return coerce_int96_timestamp_unit_;
}
+ /// Enable Parquet supported Arrow Extension Types.
+ ///
+ /// When enabled, Parquet will use supported Arrow ExtensionTypes in mapping to Arrow
+ /// schema. Currently only arrow::extension::json() extension type is supported. This
+ /// will be used for binary columns whose LogicalType is JSON.
+ void enable_known_arrow_extensions() { known_arrow_extensions_enabled_ = true; }
+ void disable_known_arrow_extensions() { known_arrow_extensions_enabled_ = false; }
+ bool known_arrow_extensions_enabled() const { return known_arrow_extensions_enabled_; }
Review Comment:
Other boolean setters use a single `void set_XXX(bool xxx)` method. Perhaps we should do that here? What do you think @wgtmac ?
##########
cpp/src/arrow/extension/json.cc:
##########
@@ -0,0 +1,62 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/status.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow::extension {
+
+bool JsonExtensionType::ExtensionEquals(const ExtensionType& other) const {
+ const auto& other_ext = static_cast<const ExtensionType&>(other);
+ return other_ext.extension_name() == this->extension_name();
+}
+
+Result<std::shared_ptr<DataType>> JsonExtensionType::Deserialize(
+ std::shared_ptr<DataType> storage_type, const std::string& serialized) const {
+ if (!serialized.empty()) {
+ return Status::Invalid("Unexpected serialized metadata: '", serialized, "'");
+ }
+ if (!(storage_type->Equals(*utf8()) || storage_type->Equals(large_utf8()) ||
+ storage_type->Equals(utf8_view()))) {
Review Comment:
Better use `id()` comparison here as well.
##########
cpp/src/arrow/extension/json_test.cc:
##########
@@ -0,0 +1,57 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include "arrow/ipc/test_common.h"
+#include "arrow/record_batch.h"
+#include "arrow/testing/gtest_util.h"
+
+namespace arrow {
+
+using arrow::ipc::test::RoundtripBatch;
+using extension::json;
+
+class TestJsonExtensionType : public ::testing::Test {};
+
+std::shared_ptr<Array> ExampleJson(const std::shared_ptr<DataType>& storage_type) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(storage_type, R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])");
+ return ExtensionType::WrapArray(arrow::extension::json(storage_type), arr);
+}
+
+TEST_F(TestJsonExtensionType, JsonRoundtrip) {
+ for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) {
+ auto ext_arr = ExampleJson(storage_type);
+
+ auto batch =
+ RecordBatch::Make(schema({field("f0", json(storage_type))}), 8, {ext_arr});
+ std::shared_ptr<RecordBatch> read_batch;
+ RoundtripBatch(batch, &read_batch);
Review Comment:
Does this also validate the batch? Or perhaps:
```suggestion
RoundtripBatch(batch, &read_batch);
ASSERT_OK(read_batch->ValidateFull());
```
##########
cpp/src/arrow/extension/json.h:
##########
@@ -0,0 +1,65 @@
+// 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 <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/visibility.h"
+
+namespace arrow::extension {
+
+/// \brief Concrete type class for variable-size JSON data, utf8-encoded.
+class ARROW_EXPORT JsonExtensionType : public ExtensionType {
+ public:
+ explicit JsonExtensionType(const std::shared_ptr<DataType>& storage_type)
+ : ExtensionType(storage_type), storage_type_(storage_type) {
+ ARROW_CHECK(storage_type->Equals(utf8()) || storage_type->Equals(large_utf8()) ||
+ storage_type->Equals(utf8_view()));
+ }
+
+ static constexpr const char* type_name() { return "arrow.json"; }
Review Comment:
No, this should remain the same as for `ExtensionType` ("extension")
##########
cpp/src/parquet/arrow/schema_internal.h:
##########
@@ -18,6 +18,7 @@
#pragma once
#include "arrow/result.h"
+#include "parquet/properties.h"
Review Comment:
Could include `parquet/type_fwd.h` instead.
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
+ DCHECK_EQ(inferred_type->id(), ::arrow::Type::BINARY);
+ inferred->field = inferred->field->WithType(origin_type);
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(origin_field, inferred));
+ } else {
+ auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
- // Apply metadata recursively to storage type
- RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
+ // Apply metadata recursively to storage type
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
- // Restore extension type, if the storage type is the same as inferred
- // from the Parquet type
- if (ex_type.storage_type()->Equals(*inferred->field->type())) {
- inferred->field = inferred->field->WithType(origin_type);
+ // Restore extension type, if the storage type is the same as inferred
+ // from the Parquet type
+ if (ex_type.storage_type()->Equals(*inferred->field->type())) {
+ inferred->field = inferred->field->WithType(origin_type);
+ }
}
modified = true;
} else {
+ if (inferred_type->id() == ::arrow::Type::EXTENSION) {
+ const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*inferred_type);
+ if (ex_type.extension_name() ==
+ ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are ENABLED in Parquet.
+ // origin_type is ::arrow::binary()
+ // inferred_type is ::arrow::extension::json()
Review Comment:
In which case does this happen?
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == ::arrow::extension::JsonExtensionType::type_name()) {
Review Comment:
Cannot use `type_name()` for this (see other comment about it in `json.h`).
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
+ DCHECK_EQ(inferred_type->id(), ::arrow::Type::BINARY);
+ inferred->field = inferred->field->WithType(origin_type);
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(origin_field, inferred));
+ } else {
+ auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
- // Apply metadata recursively to storage type
- RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
+ // Apply metadata recursively to storage type
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
- // Restore extension type, if the storage type is the same as inferred
- // from the Parquet type
- if (ex_type.storage_type()->Equals(*inferred->field->type())) {
- inferred->field = inferred->field->WithType(origin_type);
+ // Restore extension type, if the storage type is the same as inferred
+ // from the Parquet type
+ if (ex_type.storage_type()->Equals(*inferred->field->type())) {
+ inferred->field = inferred->field->WithType(origin_type);
+ }
}
modified = true;
} else {
+ if (inferred_type->id() == ::arrow::Type::EXTENSION) {
+ const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*inferred_type);
+ if (ex_type.extension_name() ==
+ ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are ENABLED in Parquet.
+ // origin_type is ::arrow::binary()
+ // inferred_type is ::arrow::extension::json()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
Review Comment:
I'm unsure that it is really desirable, since this is also losing information about the logical type.
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -427,6 +428,14 @@ Status FieldToNode(const std::string& name, const std::shared_ptr<Field>& field,
}
case ArrowTypeId::EXTENSION: {
auto ext_type = std::static_pointer_cast<::arrow::ExtensionType>(field->type());
+ // Built-in JSON extension is handled differently.
+ if (ext_type->extension_name() ==
+ std::static_pointer_cast<::arrow::ExtensionType>(::arrow::extension::json())
+ ->extension_name()) {
Review Comment:
You could just hard-code `std::string("arrow.json")` here.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733448312
##########
cpp/src/parquet/arrow/schema_internal.cc:
##########
@@ -107,17 +110,22 @@ Result<std::shared_ptr<ArrowType>> MakeArrowTimestamp(const LogicalType& logical
}
}
-Result<std::shared_ptr<ArrowType>> FromByteArray(const LogicalType& logical_type) {
+Result<std::shared_ptr<ArrowType>> FromByteArray(
+ const LogicalType& logical_type, const ArrowReaderProperties& reader_properties) {
switch (logical_type.type()) {
case LogicalType::Type::STRING:
return ::arrow::utf8();
case LogicalType::Type::DECIMAL:
return MakeArrowDecimal(logical_type);
case LogicalType::Type::NONE:
case LogicalType::Type::ENUM:
- case LogicalType::Type::JSON:
case LogicalType::Type::BSON:
return ::arrow::binary();
+ case LogicalType::Type::JSON:
+ if (reader_properties.known_arrow_extensions_enabled()) {
+ return ::arrow::extension::json(::arrow::utf8());
+ }
+ return ::arrow::binary();
Review Comment:
Should I add a 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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733436735
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
+ DCHECK_EQ(inferred_type->id(), ::arrow::Type::BINARY);
+ inferred->field = inferred->field->WithType(origin_type);
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(origin_field, inferred));
+ } else {
+ auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
- // Apply metadata recursively to storage type
- RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
+ // Apply metadata recursively to storage type
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
- // Restore extension type, if the storage type is the same as inferred
- // from the Parquet type
- if (ex_type.storage_type()->Equals(*inferred->field->type())) {
- inferred->field = inferred->field->WithType(origin_type);
+ // Restore extension type, if the storage type is the same as inferred
+ // from the Parquet type
+ if (ex_type.storage_type()->Equals(*inferred->field->type())) {
+ inferred->field = inferred->field->WithType(origin_type);
+ }
}
modified = true;
} else {
+ if (inferred_type->id() == ::arrow::Type::EXTENSION) {
+ const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*inferred_type);
+ if (ex_type.extension_name() ==
+ ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are ENABLED in Parquet.
+ // origin_type is ::arrow::binary()
+ // inferred_type is ::arrow::extension::json()
Review Comment:
If the system that wrote the Parquet file did not have JSONExtensionType but had a user created ExtensionType with type name "arrow.json"?
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733312779
##########
cpp/src/parquet/arrow/schema_internal.cc:
##########
@@ -107,17 +110,22 @@ Result<std::shared_ptr<ArrowType>> MakeArrowTimestamp(const LogicalType& logical
}
}
-Result<std::shared_ptr<ArrowType>> FromByteArray(const LogicalType& logical_type) {
+Result<std::shared_ptr<ArrowType>> FromByteArray(
+ const LogicalType& logical_type, const ArrowReaderProperties& reader_properties) {
switch (logical_type.type()) {
case LogicalType::Type::STRING:
return ::arrow::utf8();
case LogicalType::Type::DECIMAL:
return MakeArrowDecimal(logical_type);
case LogicalType::Type::NONE:
case LogicalType::Type::ENUM:
- case LogicalType::Type::JSON:
case LogicalType::Type::BSON:
return ::arrow::binary();
+ case LogicalType::Type::JSON:
+ if (reader_properties.known_arrow_extensions_enabled()) {
+ return ::arrow::extension::json(::arrow::utf8());
+ }
+ return ::arrow::binary();
Review Comment:
I'm a bit curious here, would this a binary or a string?
##########
cpp/src/parquet/arrow/schema_internal.h:
##########
@@ -28,18 +29,19 @@ namespace parquet::arrow {
using ::arrow::Result;
-Result<std::shared_ptr<::arrow::DataType>> FromByteArray(const LogicalType& logical_type);
+Result<std::shared_ptr<::arrow::DataType>> FromByteArray(const LogicalType& logical_type,
+ bool use_known_arrow_extensions);
Result<std::shared_ptr<::arrow::DataType>> FromFLBA(const LogicalType& logical_type,
int32_t physical_length);
Result<std::shared_ptr<::arrow::DataType>> FromInt32(const LogicalType& logical_type);
Result<std::shared_ptr<::arrow::DataType>> FromInt64(const LogicalType& logical_type);
Result<std::shared_ptr<::arrow::DataType>> GetArrowType(
Type::type physical_type, const LogicalType& logical_type, int type_length,
- ::arrow::TimeUnit::type int96_arrow_time_unit = ::arrow::TimeUnit::NANO);
+ const ArrowReaderProperties& reader_properties);
Review Comment:
👍
Separate issue: @pitrou should we also introduce a subpart like `struct type_coerce`? I'm currently considering read large binary / binary view / dict[xx], currently the configurations would be a lot
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733421171
##########
cpp/src/arrow/extension/json.cc:
##########
@@ -0,0 +1,62 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/status.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow::extension {
+
+bool JsonExtensionType::ExtensionEquals(const ExtensionType& other) const {
+ const auto& other_ext = static_cast<const ExtensionType&>(other);
+ return other_ext.extension_name() == this->extension_name();
+}
+
+Result<std::shared_ptr<DataType>> JsonExtensionType::Deserialize(
+ std::shared_ptr<DataType> storage_type, const std::string& serialized) const {
+ if (!serialized.empty()) {
+ return Status::Invalid("Unexpected serialized metadata: '", serialized, "'");
+ }
+ if (!(storage_type->Equals(*utf8()) || storage_type->Equals(large_utf8()) ||
+ storage_type->Equals(utf8_view()))) {
Review Comment:
Changed.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1741225426
##########
cpp/src/arrow/extension/json.h:
##########
@@ -0,0 +1,60 @@
+// 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 <stdexcept>
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/visibility.h"
+
+namespace arrow::extension {
+
+/// \brief Concrete type class for variable-size JSON data, utf8-encoded.
+class ARROW_EXPORT JsonExtensionType : public ExtensionType {
+ public:
+ explicit JsonExtensionType(const std::shared_ptr<DataType>& storage_type)
+ : ExtensionType(storage_type), storage_type_(storage_type) {}
+
+ std::string extension_name() const override { return "arrow.json"; }
+
+ std::string ToString(bool show_metadata = false) const override {
Review Comment:
Removed.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1746177294
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
+ DCHECK_EQ(inferred_type->id(), ::arrow::Type::BINARY);
+ inferred->field = inferred->field->WithType(origin_type);
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(origin_field, inferred));
+ } else {
+ auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
- // Apply metadata recursively to storage type
- RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
+ // Apply metadata recursively to storage type
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
- // Restore extension type, if the storage type is the same as inferred
- // from the Parquet type
- if (ex_type.storage_type()->Equals(*inferred->field->type())) {
- inferred->field = inferred->field->WithType(origin_type);
+ // Restore extension type, if the storage type is the same as inferred
+ // from the Parquet type
+ if (ex_type.storage_type()->Equals(*inferred->field->type())) {
+ inferred->field = inferred->field->WithType(origin_type);
+ }
}
modified = true;
} else {
+ if (inferred_type->id() == ::arrow::Type::EXTENSION) {
+ const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*inferred_type);
+ if (ex_type.extension_name() ==
+ ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are ENABLED in Parquet.
+ // origin_type is ::arrow::binary()
+ // inferred_type is ::arrow::extension::json()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
Review Comment:
True, changed further and adjusted the tests and comments https://github.com/apache/arrow/pull/13901/commits/0dde2bbc155d35754531d43e4864d09c6857a319
--
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] ARROW-17255: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1567806738
##########
cpp/src/arrow/extension/json.h:
##########
@@ -0,0 +1,60 @@
+// 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 <memory>
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/visibility.h"
+
+namespace arrow {
+namespace extension {
+
+/// \brief Concrete type class for variable-size JSON data, utf8-encoded.
+class ARROW_EXPORT JsonExtensionType : public ExtensionType {
+ public:
+ JsonExtensionType() : ExtensionType(::arrow::utf8()) {}
Review Comment:
I was wondering about that too. We could make it parametric and enable both plus `string_view`.
--
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] ARROW-17255: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1567805294
##########
cpp/src/arrow/extension/CMakeLists.txt:
##########
@@ -15,10 +15,19 @@
# specific language governing permissions and limitations
# under the License.
-add_arrow_test(test
- SOURCES
- fixed_shape_tensor_test.cc
- PREFIX
- "arrow-fixed-shape-tensor")
+if(ARROW_JSON)
+ add_arrow_test(test
+ SOURCES
+ fixed_shape_tensor_test.cc
+ json_test.cc
+ PREFIX
+ "arrow-canonical-extensions")
+else()
+ add_arrow_test(test
+ SOURCES
+ json_test.cc
Review Comment:
It does. Changed to be more ideomatic.
--
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] ARROW-17255: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2048556985
@pradeepg26 did you want to continue work on this? There was some work done on canonical extension types lately (see [cpp/src/arrow/extension](https://github.com/apache/arrow/tree/main/cpp/src/arrow/extension), [docs/source/format/CanonicalExtensions.rst](https://github.com/apache/arrow/blob/main/docs/source/format/CanonicalExtensions.rst)) that could perhaps help here.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1737558446
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,85 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata = ::arrow::key_value_metadata({}, {});
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // If Arrow extensions are enabled, both fields should be treated as json() extension
+ // fields.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled();
+ auto arrow_schema =
+ ::arrow::schema({::arrow::field("json_1", ::arrow::extension::json(), true),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
+ std::shared_ptr<KeyValueMetadata> metadata = ::arrow::key_value_metadata({}, {});
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // Arrow schema has precedence. json_1 should be returned as a json() field even
+ // though extensions are not enabled.
+ std::shared_ptr<KeyValueMetadata> field_metadata =
+ ::arrow::key_value_metadata({"foo", "bar"}, {"biz", "baz"});
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true, field_metadata),
+ ::arrow::field("json_2", UTF8, true)});
+
+ ASSERT_OK_AND_ASSIGN(
+ std::shared_ptr<Buffer> serialized,
+ ::arrow::ipc::SerializeSchema(*arrow_schema, ::arrow::default_memory_pool()));
+ std::string schema_as_string = serialized->ToString();
+ std::string schema_base64 = ::arrow::util::base64_encode(schema_as_string);
+ std::shared_ptr<KeyValueMetadata> metadata =
+ ::arrow::key_value_metadata({"ARROW:schema"}, {schema_base64});
Review Comment:
Done.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1752671661
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,86 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+Status ArrowSchemaToParquetMetadata(std::shared_ptr<::arrow::Schema>& arrow_schema,
+ std::shared_ptr<KeyValueMetadata>& metadata) {
+ ARROW_ASSIGN_OR_RAISE(
+ std::shared_ptr<Buffer> serialized,
+ ::arrow::ipc::SerializeSchema(*arrow_schema, ::arrow::default_memory_pool()));
+ std::string schema_as_string = serialized->ToString();
+ std::string schema_base64 = ::arrow::util::base64_encode(schema_as_string);
+ metadata = ::arrow::key_value_metadata({"ARROW:schema"}, {schema_base64});
+ return Status::OK();
+}
+
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // If Arrow extensions are enabled, both fields should be treated as json() extension
+ // fields.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ auto arrow_schema =
+ ::arrow::schema({::arrow::field("json_1", ::arrow::extension::json(), true),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // Parquet logical type has precedence. Both json_1 and json_2 should be returned
+ // as a json() field even though extensions are not enabled.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ std::shared_ptr<KeyValueMetadata> field_metadata =
+ ::arrow::key_value_metadata({"foo", "bar"}, {"biz", "baz"});
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true, field_metadata),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
+
+ std::shared_ptr<KeyValueMetadata> metadata;
+ ASSERT_OK(ArrowSchemaToParquetMetadata(arrow_schema, metadata));
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema, true /* check_metadata */);
+ }
+
+ {
Review Comment:
They were effectively the same, I've now set one to `props.set_arrow_extensions_enabled(true);` and the other to `props.set_arrow_extensions_enabled(false);`, so it might still be worth keeping both.
--
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] ARROW-17255: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2059261166
JNI failure seems unrelated.
--
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] ARROW-17255: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1567806458
##########
cpp/src/arrow/extension_type.cc:
##########
@@ -144,13 +145,16 @@ namespace internal {
static void CreateGlobalRegistry() {
g_registry = std::make_shared<ExtensionTypeRegistryImpl>();
+ std::vector<std::shared_ptr<DataType>> ext_types{::arrow::extension::json()};
#ifdef ARROW_JSON
- // Register canonical extension types
- auto ext_type =
- checked_pointer_cast<ExtensionType>(extension::fixed_shape_tensor(int64(), {}));
-
- ARROW_CHECK_OK(g_registry->RegisterType(ext_type));
+ ext_types.push_back(extension::fixed_shape_tensor(int64(), {}));
Review Comment:
As in alphabetical order? Changed to:
```cpp
#ifdef ARROW_JSON
std::vector<std::shared_ptr<DataType>> ext_types{
extension::fixed_shape_tensor(int64(), {}), ::arrow::extension::json()};
#else
std::vector<std::shared_ptr<DataType>> ext_types{::arrow::extension::json()};
#endif
```
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1772998665
##########
cpp/src/arrow/extension/json.cc:
##########
@@ -0,0 +1,61 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/status.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow::extension {
+
+bool JsonExtensionType::ExtensionEquals(const ExtensionType& other) const {
+ return other.extension_name() == this->extension_name();
+}
Review Comment:
I didn't start yet
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1738923863
##########
cpp/src/arrow/array/validate.cc:
##########
@@ -985,10 +985,22 @@ Status ValidateArrayFull(const Array& array) { return ValidateArrayFull(*array.d
ARROW_EXPORT
Status ValidateUTF8(const ArrayData& data) {
- DCHECK(data.type->id() == Type::STRING || data.type->id() == Type::STRING_VIEW ||
- data.type->id() == Type::LARGE_STRING);
- UTF8DataValidator validator{data};
- return VisitTypeInline(*data.type, &validator);
+ const auto& storage_type =
+ (data.type->id() == Type::EXTENSION)
+ ? checked_cast<const ExtensionType&>(*data.type).storage_type()
+ : data.type;
+ DCHECK(storage_type->id() == Type::STRING || storage_type->id() == Type::STRING_VIEW ||
+ storage_type->id() == Type::LARGE_STRING);
+
+ if (data.type->id() == Type::EXTENSION) {
+ const auto& ext_data = std::make_shared<ArrayData>(data);
Review Comment:
```suggestion
ArrayData ext_data(data);
ext_data.type = storage_type;
```
can we do like 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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2302038088
Now that we have [JSON in Go](https://github.com/apache/arrow/pull/43679) we could perhaps proceed here? :)
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2304554639
@pitrou if you can spare time this could also use a review! :)
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2332037545
Thanks for the suggestions @pitrou, I've applied and commented back, please have another look.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1752275863
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,86 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+Status ArrowSchemaToParquetMetadata(std::shared_ptr<::arrow::Schema>& arrow_schema,
+ std::shared_ptr<KeyValueMetadata>& metadata) {
+ ARROW_ASSIGN_OR_RAISE(
+ std::shared_ptr<Buffer> serialized,
+ ::arrow::ipc::SerializeSchema(*arrow_schema, ::arrow::default_memory_pool()));
+ std::string schema_as_string = serialized->ToString();
+ std::string schema_base64 = ::arrow::util::base64_encode(schema_as_string);
+ metadata = ::arrow::key_value_metadata({"ARROW:schema"}, {schema_base64});
+ return Status::OK();
+}
+
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // If Arrow extensions are enabled, both fields should be treated as json() extension
+ // fields.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ auto arrow_schema =
+ ::arrow::schema({::arrow::field("json_1", ::arrow::extension::json(), true),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // Parquet logical type has precedence. Both json_1 and json_2 should be returned
+ // as a json() field even though extensions are not enabled.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ std::shared_ptr<KeyValueMetadata> field_metadata =
+ ::arrow::key_value_metadata({"foo", "bar"}, {"biz", "baz"});
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true, field_metadata),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
+
+ std::shared_ptr<KeyValueMetadata> metadata;
+ ASSERT_OK(ArrowSchemaToParquetMetadata(arrow_schema, metadata));
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema, true /* check_metadata */);
+ }
+
+ {
Review Comment:
This looks like the exact same case as above, am I mistaken? Perhaps remove this?
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -1428,6 +1435,42 @@ TEST_F(TestLargeStringParquetIO, Basics) {
this->RoundTripSingleColumn(large_array, large_array, arrow_properties);
}
+using TestJsonParquetIO = TestParquetIO<::arrow::extension::JsonExtensionType>;
+
+TEST_F(TestJsonParquetIO, JsonExtension) {
+ const char* json = R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])";
+
+ const auto json_type = ::arrow::extension::json();
+ const auto json_string_array = ::arrow::ArrayFromJSON(::arrow::utf8(), json);
Review Comment:
The other discussion got me thinking: should we also have a test where the original storage type is `large_utf8`?
##########
cpp/src/parquet/properties.h:
##########
@@ -941,6 +942,18 @@ class PARQUET_EXPORT ArrowReaderProperties {
return coerce_int96_timestamp_unit_;
}
+ /// Enable Parquet supported Arrow ExtensionTypes.
+ ///
+ /// When enabled, Parquet will use supported Arrow ExtensionTypes by mapping correctly
+ /// mapping them to Arrow types at read time. Currently only arrow::extension::json()
Review Comment:
```suggestion
/// When enabled, Parquet logical types will be mapped to their corresponding Arrow
/// extension types at read time, if such exist. Currently only arrow::extension::json()
```
##########
cpp/src/parquet/properties.h:
##########
@@ -941,6 +942,18 @@ class PARQUET_EXPORT ArrowReaderProperties {
return coerce_int96_timestamp_unit_;
}
+ /// Enable Parquet supported Arrow ExtensionTypes.
+ ///
+ /// When enabled, Parquet will use supported Arrow ExtensionTypes by mapping correctly
+ /// mapping them to Arrow types at read time. Currently only arrow::extension::json()
+ /// extension type is supported. Columns whose LogicalType is JSON will be interpreted
+ /// as arrow::extension::json() ExtensionType with storage type utf8, large_utf8 or
+ /// utf8_view at parquet read time.
Review Comment:
```suggestion
/// extension type is supported. Columns whose LogicalType is JSON will be interpreted
/// as arrow::extension::json(), with storage type inferred from the serialized Arrow
/// schema if present, or `utf8` by default.
```
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,86 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+Status ArrowSchemaToParquetMetadata(std::shared_ptr<::arrow::Schema>& arrow_schema,
+ std::shared_ptr<KeyValueMetadata>& metadata) {
+ ARROW_ASSIGN_OR_RAISE(
+ std::shared_ptr<Buffer> serialized,
+ ::arrow::ipc::SerializeSchema(*arrow_schema, ::arrow::default_memory_pool()));
+ std::string schema_as_string = serialized->ToString();
+ std::string schema_base64 = ::arrow::util::base64_encode(schema_as_string);
+ metadata = ::arrow::key_value_metadata({"ARROW:schema"}, {schema_base64});
+ return Status::OK();
+}
+
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // If Arrow extensions are enabled, both fields should be treated as json() extension
+ // fields.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ auto arrow_schema =
+ ::arrow::schema({::arrow::field("json_1", ::arrow::extension::json(), true),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
Review Comment:
To spice things up, perhaps `json_2` should be backed by a `large_utf8` storage type?
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,86 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+Status ArrowSchemaToParquetMetadata(std::shared_ptr<::arrow::Schema>& arrow_schema,
+ std::shared_ptr<KeyValueMetadata>& metadata) {
+ ARROW_ASSIGN_OR_RAISE(
+ std::shared_ptr<Buffer> serialized,
+ ::arrow::ipc::SerializeSchema(*arrow_schema, ::arrow::default_memory_pool()));
+ std::string schema_as_string = serialized->ToString();
+ std::string schema_base64 = ::arrow::util::base64_encode(schema_as_string);
+ metadata = ::arrow::key_value_metadata({"ARROW:schema"}, {schema_base64});
+ return Status::OK();
+}
+
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // If Arrow extensions are enabled, both fields should be treated as json() extension
+ // fields.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ auto arrow_schema =
+ ::arrow::schema({::arrow::field("json_1", ::arrow::extension::json(), true),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // Parquet logical type has precedence. Both json_1 and json_2 should be returned
+ // as a json() field even though extensions are not enabled.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ std::shared_ptr<KeyValueMetadata> field_metadata =
+ ::arrow::key_value_metadata({"foo", "bar"}, {"biz", "baz"});
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true, field_metadata),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
Review Comment:
Same here.
##########
cpp/src/parquet/properties.h:
##########
@@ -941,6 +942,18 @@ class PARQUET_EXPORT ArrowReaderProperties {
return coerce_int96_timestamp_unit_;
}
+ /// Enable Parquet supported Arrow ExtensionTypes.
Review Comment:
Nit :-)
```suggestion
/// Enable Parquet-supported Arrow extension types.
```
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733421731
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -427,6 +428,14 @@ Status FieldToNode(const std::string& name, const std::shared_ptr<Field>& field,
}
case ArrowTypeId::EXTENSION: {
auto ext_type = std::static_pointer_cast<::arrow::ExtensionType>(field->type());
+ // Built-in JSON extension is handled differently.
+ if (ext_type->extension_name() ==
+ std::static_pointer_cast<::arrow::ExtensionType>(::arrow::extension::json())
+ ->extension_name()) {
Review Comment:
Done.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1737541940
##########
cpp/src/arrow/array/validate.cc:
##########
@@ -985,10 +985,23 @@ Status ValidateArrayFull(const Array& array) { return ValidateArrayFull(*array.d
ARROW_EXPORT
Status ValidateUTF8(const ArrayData& data) {
- DCHECK(data.type->id() == Type::STRING || data.type->id() == Type::STRING_VIEW ||
- data.type->id() == Type::LARGE_STRING);
- UTF8DataValidator validator{data};
- return VisitTypeInline(*data.type, &validator);
+ if (data.type->id() == Type::EXTENSION) {
+ const auto& storage_type =
+ checked_pointer_cast<ExtensionType>(data.type)->storage_type();
+ DCHECK(storage_type->id() == Type::STRING ||
+ storage_type->id() == Type::STRING_VIEW ||
+ storage_type->id() == Type::LARGE_STRING);
+ auto ext_array_data =
+ std::make_shared<ArrayData>(storage_type, data.length, data.buffers,
+ data.child_data, data.null_count, data.offset);
+ UTF8DataValidator validator{*ext_array_data};
Review Comment:
Storage type apparently wouldn't be correct.
Simplified this to:
```cpp
const auto& ext_data = std::make_shared<ArrayData>(data);
ext_data->type = storage_type;
```
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1737572395
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,85 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata = ::arrow::key_value_metadata({}, {});
Review Comment:
Do you mean the test in general or the way metadata is generated? This was changed to use a helper now.
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,85 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata = ::arrow::key_value_metadata({}, {});
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // If Arrow extensions are enabled, both fields should be treated as json() extension
+ // fields.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled();
+ auto arrow_schema =
+ ::arrow::schema({::arrow::field("json_1", ::arrow::extension::json(), true),
+ ::arrow::field("json_2", ::arrow::extension::json(), true)});
+ std::shared_ptr<KeyValueMetadata> metadata = ::arrow::key_value_metadata({}, {});
Review Comment:
Same as above.
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #13901: ARROW-17255: [C++][Parquet] Add JSON canonical extension type
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-1217144743
https://issues.apache.org/jira/browse/ARROW-17255
--
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
[GitHub] [arrow] tustvold commented on a diff in pull request #13901: ARROW-17255: [C++][Parquet] Add JSON canonical extension type
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1053430210
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -1410,6 +1417,43 @@ TEST_F(TestLargeStringParquetIO, Basics) {
this->RoundTripSingleColumn(large_array, large_array, arrow_properties);
}
+using TestJsonParquetIO = TestParquetIO<::arrow::extension::JsonExtensionType>;
+
+TEST_F(TestJsonParquetIO, JsonExtension) {
+ const char* json = R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])";
+
+ const auto json_type = ::arrow::extension::json();
+ const auto json_string_array = ::arrow::ArrayFromJSON(::arrow::utf8(), json);
+ const auto json_array = ::arrow::ExtensionType::WrapArray(json_type, json_string_array);
+
+ // When the original Arrow schema isn't stored and Arrow extensions are disabled,
+ // LogicalType::JSON is read as Binary.
+ const auto binary_array = ::arrow::ArrayFromJSON(::arrow::binary(), json);
Review Comment:
The PR description says
> This extension is backed by utf8()
So I would have naively expected this to be inferred as `arrow::utf8`?
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1749043634
##########
cpp/src/parquet/properties.h:
##########
@@ -941,6 +942,18 @@ class PARQUET_EXPORT ArrowReaderProperties {
return coerce_int96_timestamp_unit_;
}
+ /// Enable Parquet supported Arrow ExtensionTypes.
+ ///
+ /// When enabled, Parquet will use supported Arrow ExtensionTypes by mapping correctly
+ /// mapping them to Arrow types at read time. Currently only arrow::extension::json()
+ /// extension type is supported. Columns whose LogicalType is JSON will be interpreted
+ /// as arrow::extension::json() ExtensionType with storage type utf8, large_utf8 or
Review Comment:
Is it possible for users to pick one from `utf8, large_utf8 or utf8_view` on read?
##########
cpp/src/parquet/arrow/schema_internal.cc:
##########
@@ -107,17 +110,24 @@ Result<std::shared_ptr<ArrowType>> MakeArrowTimestamp(const LogicalType& logical
}
}
-Result<std::shared_ptr<ArrowType>> FromByteArray(const LogicalType& logical_type) {
+Result<std::shared_ptr<ArrowType>> FromByteArray(
+ const LogicalType& logical_type, const ArrowReaderProperties& reader_properties) {
switch (logical_type.type()) {
case LogicalType::Type::STRING:
return ::arrow::utf8();
case LogicalType::Type::DECIMAL:
return MakeArrowDecimal(logical_type);
case LogicalType::Type::NONE:
case LogicalType::Type::ENUM:
- case LogicalType::Type::JSON:
case LogicalType::Type::BSON:
return ::arrow::binary();
+ case LogicalType::Type::JSON:
+ if (reader_properties.get_arrow_extensions_enabled()) {
Review Comment:
Sorry that I'm late on the review. Is this property too board? Should we restrict it to json specifically?
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733438958
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
+ DCHECK_EQ(inferred_type->id(), ::arrow::Type::BINARY);
+ inferred->field = inferred->field->WithType(origin_type);
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(origin_field, inferred));
+ } else {
+ auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
- // Apply metadata recursively to storage type
- RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
+ // Apply metadata recursively to storage type
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
- // Restore extension type, if the storage type is the same as inferred
- // from the Parquet type
- if (ex_type.storage_type()->Equals(*inferred->field->type())) {
- inferred->field = inferred->field->WithType(origin_type);
+ // Restore extension type, if the storage type is the same as inferred
+ // from the Parquet type
+ if (ex_type.storage_type()->Equals(*inferred->field->type())) {
+ inferred->field = inferred->field->WithType(origin_type);
+ }
}
modified = true;
} else {
+ if (inferred_type->id() == ::arrow::Type::EXTENSION) {
+ const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*inferred_type);
+ if (ex_type.extension_name() ==
+ ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are ENABLED in Parquet.
+ // origin_type is ::arrow::binary()
+ // inferred_type is ::arrow::extension::json()
Review Comment:
Hm or perhaps not. Not sure, I'm ok to remove 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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2304700504
Let's do that after the UUID PR is in, then!
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2345017521
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 27acf8bf6e16d4c53e1ed59d5ef46ac7db0306ea.
There was 1 benchmark result indicating a performance regression:
- Commit Run on `amd64-m5-4xlarge-linux` at [2024-09-11 12:10:49Z](https://conbench.ursa.dev/compare/runs/4bd86a563f194b89a04de319eda32f03...d37fb42dec664ee3b60b0779f425afa0/)
- [`dataset-serialize` (Python) with dataset=nyctaxi_multi_parquet_s3, format=feather, selectivity=1pc](https://conbench.ursa.dev/compare/benchmarks/066e150c70337d7980006905bab3ff78...066e19d0479372088000985551231b40)
The [full Conbench report](https://github.com/apache/arrow/runs/30024458944) has more details. It also includes information about 274 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1753555791
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -724,6 +727,88 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+Status ArrowSchemaToParquetMetadata(std::shared_ptr<::arrow::Schema>& arrow_schema,
+ std::shared_ptr<KeyValueMetadata>& metadata) {
+ ARROW_ASSIGN_OR_RAISE(
+ std::shared_ptr<Buffer> serialized,
+ ::arrow::ipc::SerializeSchema(*arrow_schema, ::arrow::default_memory_pool()));
+ std::string schema_as_string = serialized->ToString();
+ std::string schema_base64 = ::arrow::util::base64_encode(schema_as_string);
+ metadata = ::arrow::key_value_metadata({"ARROW:schema"}, {schema_base64});
+ return Status::OK();
+}
+
+TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
+ std::vector<NodePtr> parquet_fields;
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_1", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+ parquet_fields.push_back(PrimitiveNode::Make(
+ "json_2", Repetition::OPTIONAL, ParquetType::BYTE_ARRAY, ConvertedType::JSON));
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // By default, both fields should be treated as utf8() fields in Arrow.
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", UTF8, true), ::arrow::field("json_2", UTF8, true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file does not contain Arrow schema.
+ // If Arrow extensions are enabled, both fields should be treated as json() extension
+ // fields.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(true);
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true),
+ ::arrow::field("json_2", ::arrow::extension::json(::arrow::large_utf8()),
+ true)});
+ std::shared_ptr<KeyValueMetadata> metadata{};
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // Parquet logical type has precedence. Both json_1 and json_2 should be returned
+ // as a json() field even though extensions are not enabled.
+ ArrowReaderProperties props;
+ props.set_arrow_extensions_enabled(false);
+ std::shared_ptr<KeyValueMetadata> field_metadata =
+ ::arrow::key_value_metadata({"foo", "bar"}, {"biz", "baz"});
+ auto arrow_schema = ::arrow::schema(
+ {::arrow::field("json_1", ::arrow::extension::json(), true, field_metadata),
+ ::arrow::field("json_2", ::arrow::extension::json(::arrow::large_utf8()),
+ true)});
+
+ std::shared_ptr<KeyValueMetadata> metadata;
+ ASSERT_OK(ArrowSchemaToParquetMetadata(arrow_schema, metadata));
+ ASSERT_OK(ConvertSchema(parquet_fields, metadata, props));
+ CheckFlatSchema(arrow_schema, true /* check_metadata */);
+ }
+
+ {
+ // Parquet file contains Arrow schema.
+ // A contrived example. Parquet believes both columns are JSON. Arrow believes json_1
Review Comment:
Updated.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #13901:
URL: https://github.com/apache/arrow/pull/13901
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1752672122
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -1428,6 +1435,42 @@ TEST_F(TestLargeStringParquetIO, Basics) {
this->RoundTripSingleColumn(large_array, large_array, arrow_properties);
}
+using TestJsonParquetIO = TestParquetIO<::arrow::extension::JsonExtensionType>;
+
+TEST_F(TestJsonParquetIO, JsonExtension) {
+ const char* json = R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])";
+
+ const auto json_type = ::arrow::extension::json();
+ const auto json_string_array = ::arrow::ArrayFromJSON(::arrow::utf8(), json);
Review Comment:
Added test for `large_utf8` (https://github.com/apache/arrow/pull/13901/commits/2648d7aad493e2d9a442301a19a68f3dc4f89772).
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1735933214
##########
cpp/src/parquet/arrow/schema_internal.h:
##########
@@ -28,18 +29,19 @@ namespace parquet::arrow {
using ::arrow::Result;
-Result<std::shared_ptr<::arrow::DataType>> FromByteArray(const LogicalType& logical_type);
+Result<std::shared_ptr<::arrow::DataType>> FromByteArray(const LogicalType& logical_type,
+ bool use_known_arrow_extensions);
Result<std::shared_ptr<::arrow::DataType>> FromFLBA(const LogicalType& logical_type,
int32_t physical_length);
Result<std::shared_ptr<::arrow::DataType>> FromInt32(const LogicalType& logical_type);
Result<std::shared_ptr<::arrow::DataType>> FromInt64(const LogicalType& logical_type);
Result<std::shared_ptr<::arrow::DataType>> GetArrowType(
Type::type physical_type, const LogicalType& logical_type, int type_length,
- ::arrow::TimeUnit::type int96_arrow_time_unit = ::arrow::TimeUnit::NANO);
+ const ArrowReaderProperties& reader_properties);
Review Comment:
Do we need to do something about this here or is this a separate discussion? (I don't mind discussing it here if it's too small for a new issue, just checking if I should do something here).
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1735937242
##########
cpp/src/parquet/arrow/schema_internal.h:
##########
@@ -28,18 +29,19 @@ namespace parquet::arrow {
using ::arrow::Result;
-Result<std::shared_ptr<::arrow::DataType>> FromByteArray(const LogicalType& logical_type);
+Result<std::shared_ptr<::arrow::DataType>> FromByteArray(const LogicalType& logical_type,
+ bool use_known_arrow_extensions);
Result<std::shared_ptr<::arrow::DataType>> FromFLBA(const LogicalType& logical_type,
int32_t physical_length);
Result<std::shared_ptr<::arrow::DataType>> FromInt32(const LogicalType& logical_type);
Result<std::shared_ptr<::arrow::DataType>> FromInt64(const LogicalType& logical_type);
Result<std::shared_ptr<::arrow::DataType>> GetArrowType(
Type::type physical_type, const LogicalType& logical_type, int type_length,
- ::arrow::TimeUnit::type int96_arrow_time_unit = ::arrow::TimeUnit::NANO);
+ const ArrowReaderProperties& reader_properties);
Review Comment:
I've opened https://github.com/apache/arrow/issues/43810
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1741225907
##########
cpp/src/arrow/extension/json_test.cc:
##########
@@ -0,0 +1,91 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include "arrow/array/validate.h"
+#include "arrow/ipc/test_common.h"
+#include "arrow/record_batch.h"
+#include "arrow/testing/gtest_util.h"
+#include "parquet/exception.h"
+
+namespace arrow {
+
+using arrow::ipc::test::RoundtripBatch;
+using extension::json;
+
+class TestJsonExtensionType : public ::testing::Test {};
+
+std::shared_ptr<Array> ExampleJson(const std::shared_ptr<DataType>& storage_type) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(storage_type, R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])");
+ return ExtensionType::WrapArray(arrow::extension::json(storage_type), arr);
+}
+
+static std::shared_ptr<Array> ExampleJsonInvalidUTF8(
+ const std::shared_ptr<DataType>& storage_type) {
+ return ArrayFromJSON(storage_type,
+ "["
+ R"(
Review Comment:
Using a different example now, this was removed altogether.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1737560117
##########
cpp/src/arrow/extension/json_test.cc:
##########
@@ -0,0 +1,58 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include "arrow/ipc/test_common.h"
+#include "arrow/record_batch.h"
+#include "arrow/testing/gtest_util.h"
+
+namespace arrow {
+
+using arrow::ipc::test::RoundtripBatch;
+using extension::json;
+
+class TestJsonExtensionType : public ::testing::Test {};
+
+std::shared_ptr<Array> ExampleJson(const std::shared_ptr<DataType>& storage_type) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(storage_type, R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])");
+ return ExtensionType::WrapArray(arrow::extension::json(storage_type), arr);
+}
+
+TEST_F(TestJsonExtensionType, JsonRoundtrip) {
+ for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) {
+ auto ext_arr = ExampleJson(storage_type);
+
+ auto batch =
+ RecordBatch::Make(schema({field("f0", json(storage_type))}), 8, {ext_arr});
+ std::shared_ptr<RecordBatch> read_batch;
+ RoundtripBatch(batch, &read_batch);
+ ASSERT_OK(read_batch->ValidateFull());
+ CompareBatch(*batch, *read_batch, /*compare_metadata*/ true);
+ }
+}
Review Comment:
Added `ValidateFull` and `ValidateUTF8` will add non-valid utf8.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733420456
##########
cpp/src/arrow/extension/json_test.cc:
##########
@@ -0,0 +1,57 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include "arrow/ipc/test_common.h"
+#include "arrow/record_batch.h"
+#include "arrow/testing/gtest_util.h"
+
+namespace arrow {
+
+using arrow::ipc::test::RoundtripBatch;
+using extension::json;
+
+class TestJsonExtensionType : public ::testing::Test {};
+
+std::shared_ptr<Array> ExampleJson(const std::shared_ptr<DataType>& storage_type) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(storage_type, R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])");
+ return ExtensionType::WrapArray(arrow::extension::json(storage_type), arr);
+}
+
+TEST_F(TestJsonExtensionType, JsonRoundtrip) {
+ for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) {
+ auto ext_arr = ExampleJson(storage_type);
+
+ auto batch =
+ RecordBatch::Make(schema({field("f0", json(storage_type))}), 8, {ext_arr});
+ std::shared_ptr<RecordBatch> read_batch;
+ RoundtripBatch(batch, &read_batch);
+ CompareBatch(*batch, *read_batch, false /* compare_metadata */);
Review Comment:
Switched to `true`.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733448031
##########
cpp/src/parquet/arrow/schema_internal.cc:
##########
@@ -107,17 +110,22 @@ Result<std::shared_ptr<ArrowType>> MakeArrowTimestamp(const LogicalType& logical
}
}
-Result<std::shared_ptr<ArrowType>> FromByteArray(const LogicalType& logical_type) {
+Result<std::shared_ptr<ArrowType>> FromByteArray(
+ const LogicalType& logical_type, const ArrowReaderProperties& reader_properties) {
switch (logical_type.type()) {
case LogicalType::Type::STRING:
return ::arrow::utf8();
case LogicalType::Type::DECIMAL:
return MakeArrowDecimal(logical_type);
case LogicalType::Type::NONE:
case LogicalType::Type::ENUM:
- case LogicalType::Type::JSON:
case LogicalType::Type::BSON:
return ::arrow::binary();
+ case LogicalType::Type::JSON:
+ if (reader_properties.known_arrow_extensions_enabled()) {
+ return ::arrow::extension::json(::arrow::utf8());
+ }
+ return ::arrow::binary();
Review Comment:
I think, as per @progger-dev's comment in tests:
> // When the original Arrow schema isn't stored and Arrow extensions are disabled,
> // LogicalType::JSON is read as Binary.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2313407804
Thanks for the reviews @pitrou & @mapleFU. I've addressed most issues and commented on remaining. Could you please take another look?
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1735954499
##########
cpp/src/parquet/properties.h:
##########
@@ -941,6 +942,15 @@ class PARQUET_EXPORT ArrowReaderProperties {
return coerce_int96_timestamp_unit_;
}
+ /// Enable Parquet supported Arrow Extension Types.
+ ///
+ /// When enabled, Parquet will use supported Arrow ExtensionTypes in mapping to Arrow
+ /// schema. Currently only arrow::extension::json() extension type is supported. This
+ /// will be used for utf8 columns whose LogicalType is JSON.
+ void set_arrow_extensions_enabled() { arrow_extensions_enabled_ = true; }
+ void set_arrow_extensions_disabled() { arrow_extensions_enabled_ = false; }
+ bool get_arrow_extensions_enabled() const { return arrow_extensions_enabled_; }
Review Comment:
I don't know whether get/set is proper...
##########
cpp/src/parquet/properties.h:
##########
@@ -941,6 +942,15 @@ class PARQUET_EXPORT ArrowReaderProperties {
return coerce_int96_timestamp_unit_;
}
+ /// Enable Parquet supported Arrow Extension Types.
+ ///
+ /// When enabled, Parquet will use supported Arrow ExtensionTypes in mapping to Arrow
+ /// schema. Currently only arrow::extension::json() extension type is supported. This
Review Comment:
Can we decribe it more specifically? Like arrow extension type and parquet type mapping?
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -427,6 +428,12 @@ Status FieldToNode(const std::string& name, const std::shared_ptr<Field>& field,
}
case ArrowTypeId::EXTENSION: {
auto ext_type = std::static_pointer_cast<::arrow::ExtensionType>(field->type());
+ // Built-in JSON extension is handled differently.
+ if (ext_type->extension_name() == std::string("arrow.json")) {
+ type = ParquetType::BYTE_ARRAY;
+ logical_type = LogicalType::JSON();
+ break;
Review Comment:
what does break means? should we return directly?
##########
cpp/src/arrow/array/validate.cc:
##########
@@ -985,10 +985,23 @@ Status ValidateArrayFull(const Array& array) { return ValidateArrayFull(*array.d
ARROW_EXPORT
Status ValidateUTF8(const ArrayData& data) {
- DCHECK(data.type->id() == Type::STRING || data.type->id() == Type::STRING_VIEW ||
- data.type->id() == Type::LARGE_STRING);
- UTF8DataValidator validator{data};
- return VisitTypeInline(*data.type, &validator);
+ if (data.type->id() == Type::EXTENSION) {
+ const auto& storage_type =
+ checked_pointer_cast<ExtensionType>(data.type)->storage_type();
Review Comment:
what about `checked_cast<ExtensionType*>(data.type.get())->storage_type();`?
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1735931143
##########
cpp/src/parquet/arrow/schema_internal.cc:
##########
@@ -107,17 +110,22 @@ Result<std::shared_ptr<ArrowType>> MakeArrowTimestamp(const LogicalType& logical
}
}
-Result<std::shared_ptr<ArrowType>> FromByteArray(const LogicalType& logical_type) {
+Result<std::shared_ptr<ArrowType>> FromByteArray(
+ const LogicalType& logical_type, const ArrowReaderProperties& reader_properties) {
switch (logical_type.type()) {
case LogicalType::Type::STRING:
return ::arrow::utf8();
case LogicalType::Type::DECIMAL:
return MakeArrowDecimal(logical_type);
case LogicalType::Type::NONE:
case LogicalType::Type::ENUM:
- case LogicalType::Type::JSON:
case LogicalType::Type::BSON:
return ::arrow::binary();
+ case LogicalType::Type::JSON:
+ if (reader_properties.known_arrow_extensions_enabled()) {
+ return ::arrow::extension::json(::arrow::utf8());
+ }
+ return ::arrow::binary();
Review Comment:
Yeah, string seems to make more sense. Changed https://github.com/apache/arrow/pull/13901/commits/23b325ef0afdf4796c9e284e84772f9a3b98a26d, please see if this looks better @mapleFU.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1737559345
##########
cpp/src/parquet/properties.h:
##########
@@ -941,6 +942,15 @@ class PARQUET_EXPORT ArrowReaderProperties {
return coerce_int96_timestamp_unit_;
}
+ /// Enable Parquet supported Arrow Extension Types.
+ ///
+ /// When enabled, Parquet will use supported Arrow ExtensionTypes in mapping to Arrow
+ /// schema. Currently only arrow::extension::json() extension type is supported. This
+ /// will be used for utf8 columns whose LogicalType is JSON.
+ void set_arrow_extensions_enabled() { arrow_extensions_enabled_ = true; }
+ void set_arrow_extensions_disabled() { arrow_extensions_enabled_ = false; }
+ bool get_arrow_extensions_enabled() const { return arrow_extensions_enabled_; }
+
Review Comment:
Changed.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1737540860
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -427,6 +428,12 @@ Status FieldToNode(const std::string& name, const std::shared_ptr<Field>& field,
}
case ArrowTypeId::EXTENSION: {
auto ext_type = std::static_pointer_cast<::arrow::ExtensionType>(field->type());
+ // Built-in JSON extension is handled differently.
+ if (ext_type->extension_name() == std::string("arrow.json")) {
+ type = ParquetType::BYTE_ARRAY;
+ logical_type = LogicalType::JSON();
+ break;
Review Comment:
Oh, I'm not sure how it got there. Removed.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1735974412
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -1387,8 +1394,8 @@ using TestLargeBinaryParquetIO = TestParquetIO<::arrow::LargeBinaryType>;
TEST_F(TestLargeBinaryParquetIO, Basics) {
const char* json = "[\"foo\", \"\", null, \"\xff\"]";
- const auto large_type = ::arrow::large_binary();
- const auto narrow_type = ::arrow::binary();
+ const auto large_type = ::arrow::large_utf8();
+ const auto narrow_type = ::arrow::utf8();
const auto large_array = ::arrow::ArrayFromJSON(large_type, json);
Review Comment:
I suggest this change because in Parquet this is defined as "valid utf8" . But you can decide that?
https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift#L154
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1772794551
##########
cpp/src/arrow/extension/json.cc:
##########
@@ -0,0 +1,61 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/status.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow::extension {
+
+bool JsonExtensionType::ExtensionEquals(const ExtensionType& other) const {
+ return other.extension_name() == this->extension_name();
+}
Review Comment:
No, that's certainly a bug. Sorry for not spotting this, and feel free to submit a fix :-)
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1772765813
##########
cpp/src/arrow/extension/json.cc:
##########
@@ -0,0 +1,61 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/status.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow::extension {
+
+bool JsonExtensionType::ExtensionEquals(const ExtensionType& other) const {
+ return other.extension_name() == this->extension_name();
+}
Review Comment:
This equality check does not take into account the storage type, but only the name.
As a consequence, a `JsonExtensionType<string>` type will be seen as equal to `JsonExtensionType<large_string>`. Was that intentional?
While from a user point of view, it certainly makes sense to have those seen as equal, but the same is true for string vs large_string itself. And in general in Arrow C++, the types are concrete types where variants of the same "logical" type (eg string vs large_string) are not seen as equal. So should the same logic be followed here?
I assume that such type equality will for example be used to check if schemas are equal to see if a set of batches can be concatenated or written to the same IPC stream, etc, and for those cases we require exact equality?
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2371061566
Created and issue https://github.com/apache/arrow/issues/44214 and opened a PR https://github.com/apache/arrow/pull/44215 addressing both of these.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1752271358
##########
cpp/src/parquet/properties.h:
##########
@@ -941,6 +942,18 @@ class PARQUET_EXPORT ArrowReaderProperties {
return coerce_int96_timestamp_unit_;
}
+ /// Enable Parquet supported Arrow ExtensionTypes.
+ ///
+ /// When enabled, Parquet will use supported Arrow ExtensionTypes by mapping correctly
+ /// mapping them to Arrow types at read time. Currently only arrow::extension::json()
+ /// extension type is supported. Columns whose LogicalType is JSON will be interpreted
+ /// as arrow::extension::json() ExtensionType with storage type utf8, large_utf8 or
Review Comment:
I don't think it makes sense to expose such a setting specifically for JSON (rather than generically for all string-like columns). Also, if you store the Arrow schema, the original storage type should be restored (but that's not implemented for `utf8_view` AFAICT).
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1752362505
##########
cpp/src/parquet/arrow/schema_internal.cc:
##########
@@ -107,17 +110,24 @@ Result<std::shared_ptr<ArrowType>> MakeArrowTimestamp(const LogicalType& logical
}
}
-Result<std::shared_ptr<ArrowType>> FromByteArray(const LogicalType& logical_type) {
+Result<std::shared_ptr<ArrowType>> FromByteArray(
+ const LogicalType& logical_type, const ArrowReaderProperties& reader_properties) {
switch (logical_type.type()) {
case LogicalType::Type::STRING:
return ::arrow::utf8();
case LogicalType::Type::DECIMAL:
return MakeArrowDecimal(logical_type);
case LogicalType::Type::NONE:
case LogicalType::Type::ENUM:
- case LogicalType::Type::JSON:
case LogicalType::Type::BSON:
return ::arrow::binary();
+ case LogicalType::Type::JSON:
+ if (reader_properties.get_arrow_extensions_enabled()) {
Review Comment:
I'm not sure why would we want a more specific property. Could you say more?
Thinking about it - `json` extension doesn't have any extra requirements so we'll likely always have it enabled. Same for `bool8` and `uuid`. On the other hand `fixed_shape_tensor` and `opaque` depend on `rapidjson` so they would not always be available, but they are not Parquet logical types.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1752367197
##########
cpp/src/parquet/properties.h:
##########
@@ -941,6 +942,18 @@ class PARQUET_EXPORT ArrowReaderProperties {
return coerce_int96_timestamp_unit_;
}
+ /// Enable Parquet supported Arrow ExtensionTypes.
+ ///
+ /// When enabled, Parquet will use supported Arrow ExtensionTypes by mapping correctly
+ /// mapping them to Arrow types at read time. Currently only arrow::extension::json()
+ /// extension type is supported. Columns whose LogicalType is JSON will be interpreted
+ /// as arrow::extension::json() ExtensionType with storage type utf8, large_utf8 or
Review Comment:
@wgtmac do you mean cast on read?
--
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] ARROW-17255: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "progger-dev (via GitHub)" <gi...@apache.org>.
progger-dev commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1567701968
##########
cpp/src/arrow/extension/CMakeLists.txt:
##########
@@ -15,10 +15,19 @@
# specific language governing permissions and limitations
# under the License.
-add_arrow_test(test
- SOURCES
- fixed_shape_tensor_test.cc
- PREFIX
- "arrow-fixed-shape-tensor")
+if(ARROW_JSON)
+ add_arrow_test(test
+ SOURCES
+ fixed_shape_tensor_test.cc
+ json_test.cc
+ PREFIX
+ "arrow-canonical-extensions")
+else()
+ add_arrow_test(test
+ SOURCES
+ json_test.cc
Review Comment:
Should this `fixed_shape_tensor_test.cc`?
##########
cpp/src/arrow/extension_type.cc:
##########
@@ -144,13 +145,16 @@ namespace internal {
static void CreateGlobalRegistry() {
g_registry = std::make_shared<ExtensionTypeRegistryImpl>();
+ std::vector<std::shared_ptr<DataType>> ext_types{::arrow::extension::json()};
#ifdef ARROW_JSON
- // Register canonical extension types
- auto ext_type =
- checked_pointer_cast<ExtensionType>(extension::fixed_shape_tensor(int64(), {}));
-
- ARROW_CHECK_OK(g_registry->RegisterType(ext_type));
+ ext_types.push_back(extension::fixed_shape_tensor(int64(), {}));
Review Comment:
Is the placement of `json()` and `fixed_shape_tensor()` correct?
##########
cpp/src/arrow/extension_type_test.cc:
##########
@@ -26,6 +26,7 @@
#include "arrow/array/array_nested.h"
#include "arrow/array/util.h"
+#include "arrow/extension/json.h"
Review Comment:
unused import?
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #13901: ARROW-17255: [C++][Parquet] Add JSON canonical extension type
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-1217144774
:warning: Ticket **has not been started in JIRA**, please click 'Start Progress'.
--
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
[GitHub] [arrow] progger-dev commented on pull request #13901: ARROW-17255: [C++][Parquet] Add JSON canonical extension type
Posted by GitBox <gi...@apache.org>.
progger-dev commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-1218304314
@pitrou Absolutely! That's fine. I was hoping that this PR would trigger that conversation.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1737540152
##########
cpp/src/arrow/array/validate.cc:
##########
@@ -985,10 +985,23 @@ Status ValidateArrayFull(const Array& array) { return ValidateArrayFull(*array.d
ARROW_EXPORT
Status ValidateUTF8(const ArrayData& data) {
- DCHECK(data.type->id() == Type::STRING || data.type->id() == Type::STRING_VIEW ||
- data.type->id() == Type::LARGE_STRING);
- UTF8DataValidator validator{data};
- return VisitTypeInline(*data.type, &validator);
+ if (data.type->id() == Type::EXTENSION) {
+ const auto& storage_type =
+ checked_pointer_cast<ExtensionType>(data.type)->storage_type();
Review Comment:
Changed.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1737544431
##########
cpp/src/arrow/extension/json.h:
##########
@@ -0,0 +1,66 @@
+// 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 <stdexcept>
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/visibility.h"
+
+namespace arrow::extension {
+
+/// \brief Concrete type class for variable-size JSON data, utf8-encoded.
+class ARROW_EXPORT JsonExtensionType : public ExtensionType {
+ public:
+ explicit JsonExtensionType(const std::shared_ptr<DataType>& storage_type)
+ : ExtensionType(storage_type), storage_type_(storage_type) {
+ if (storage_type->id() != Type::STRING && storage_type->id() != Type::STRING_VIEW &&
+ storage_type->id() != Type::LARGE_STRING) {
+ throw std::invalid_argument("Invalid storage type for JsonExtensionType: " +
Review Comment:
Moved to `json.cc` into `std::shared_ptr<DataType> json(const std::shared_ptr<DataType> storage_type)`.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1738386480
##########
cpp/src/parquet/properties.h:
##########
@@ -941,6 +942,15 @@ class PARQUET_EXPORT ArrowReaderProperties {
return coerce_int96_timestamp_unit_;
}
+ /// Enable Parquet supported Arrow Extension Types.
+ ///
+ /// When enabled, Parquet will use supported Arrow ExtensionTypes in mapping to Arrow
+ /// schema. Currently only arrow::extension::json() extension type is supported. This
Review Comment:
I'v updated this text a little, but perhaps we should add more?
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1733421346
##########
cpp/src/parquet/arrow/schema_internal.h:
##########
@@ -18,6 +18,7 @@
#pragma once
#include "arrow/result.h"
+#include "parquet/properties.h"
Review Comment:
Changed.
##########
cpp/src/arrow/extension/json.h:
##########
@@ -0,0 +1,65 @@
+// 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 <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/visibility.h"
+
+namespace arrow::extension {
+
+/// \brief Concrete type class for variable-size JSON data, utf8-encoded.
+class ARROW_EXPORT JsonExtensionType : public ExtensionType {
+ public:
+ explicit JsonExtensionType(const std::shared_ptr<DataType>& storage_type)
+ : ExtensionType(storage_type), storage_type_(storage_type) {
+ ARROW_CHECK(storage_type->Equals(utf8()) || storage_type->Equals(large_utf8()) ||
+ storage_type->Equals(utf8_view()));
+ }
+
+ static constexpr const char* type_name() { return "arrow.json"; }
Review Comment:
Removed.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1735940704
##########
cpp/src/parquet/arrow/schema_internal.h:
##########
@@ -28,18 +29,19 @@ namespace parquet::arrow {
using ::arrow::Result;
-Result<std::shared_ptr<::arrow::DataType>> FromByteArray(const LogicalType& logical_type);
+Result<std::shared_ptr<::arrow::DataType>> FromByteArray(const LogicalType& logical_type,
+ bool use_known_arrow_extensions);
Result<std::shared_ptr<::arrow::DataType>> FromFLBA(const LogicalType& logical_type,
int32_t physical_length);
Result<std::shared_ptr<::arrow::DataType>> FromInt32(const LogicalType& logical_type);
Result<std::shared_ptr<::arrow::DataType>> FromInt64(const LogicalType& logical_type);
Result<std::shared_ptr<::arrow::DataType>> GetArrowType(
Type::type physical_type, const LogicalType& logical_type, int type_length,
- ::arrow::TimeUnit::type int96_arrow_time_unit = ::arrow::TimeUnit::NANO);
+ const ArrowReaderProperties& reader_properties);
Review Comment:
@pitrou
> It sounds unrelated to this PR.
Maybe yes. Now that we have a lots of different properties to read "string" out. I think we need some better design on 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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1745104817
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -427,6 +428,13 @@ Status FieldToNode(const std::string& name, const std::shared_ptr<Field>& field,
}
case ArrowTypeId::EXTENSION: {
auto ext_type = std::static_pointer_cast<::arrow::ExtensionType>(field->type());
+ // Built-in JSON extension is handled differently.
+ if (ext_type->extension_name() == std::string("arrow.json")) {
Review Comment:
So the writer-side, this is forced to:
```
type = ParquetType::BYTE_ARRAY;
logical_type = LogicalType::JSON();
```
On reader side, it's set by `get_arrow_extensions_enabled()`
--
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] ARROW-17255: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2061271290
I've opened a more generic extension type proposal: https://github.com/apache/arrow/pull/41257/files.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13901:
URL: https://github.com/apache/arrow/pull/13901#issuecomment-2143911570
:warning: GitHub issue #32538 **has been automatically assigned in GitHub** to PR creator.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1738888302
##########
cpp/src/arrow/extension/json_test.cc:
##########
@@ -0,0 +1,58 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include "arrow/ipc/test_common.h"
+#include "arrow/record_batch.h"
+#include "arrow/testing/gtest_util.h"
+
+namespace arrow {
+
+using arrow::ipc::test::RoundtripBatch;
+using extension::json;
+
+class TestJsonExtensionType : public ::testing::Test {};
+
+std::shared_ptr<Array> ExampleJson(const std::shared_ptr<DataType>& storage_type) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(storage_type, R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])");
+ return ExtensionType::WrapArray(arrow::extension::json(storage_type), arr);
+}
+
+TEST_F(TestJsonExtensionType, JsonRoundtrip) {
+ for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) {
+ auto ext_arr = ExampleJson(storage_type);
+
+ auto batch =
+ RecordBatch::Make(schema({field("f0", json(storage_type))}), 8, {ext_arr});
+ std::shared_ptr<RecordBatch> read_batch;
+ RoundtripBatch(batch, &read_batch);
+ ASSERT_OK(read_batch->ValidateFull());
+ CompareBatch(*batch, *read_batch, /*compare_metadata*/ true);
+ }
+}
Review Comment:
Added invalid utf8 test too.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1738894355
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -427,6 +428,12 @@ Status FieldToNode(const std::string& name, const std::shared_ptr<Field>& field,
}
case ArrowTypeId::EXTENSION: {
auto ext_type = std::static_pointer_cast<::arrow::ExtensionType>(field->type());
+ // Built-in JSON extension is handled differently.
+ if (ext_type->extension_name() == std::string("arrow.json")) {
+ type = ParquetType::BYTE_ARRAY;
+ logical_type = LogicalType::JSON();
+ break;
Review Comment:
Reintroduced this break. It breaks `case` and is necessary.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1741226634
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
+ DCHECK_EQ(inferred_type->id(), ::arrow::Type::BINARY);
+ inferred->field = inferred->field->WithType(origin_type);
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(origin_field, inferred));
+ } else {
+ auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
- // Apply metadata recursively to storage type
- RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
+ // Apply metadata recursively to storage type
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
- // Restore extension type, if the storage type is the same as inferred
- // from the Parquet type
- if (ex_type.storage_type()->Equals(*inferred->field->type())) {
- inferred->field = inferred->field->WithType(origin_type);
+ // Restore extension type, if the storage type is the same as inferred
+ // from the Parquet type
+ if (ex_type.storage_type()->Equals(*inferred->field->type())) {
+ inferred->field = inferred->field->WithType(origin_type);
+ }
}
modified = true;
} else {
+ if (inferred_type->id() == ::arrow::Type::EXTENSION) {
+ const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*inferred_type);
+ if (ex_type.extension_name() ==
+ ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are ENABLED in Parquet.
+ // origin_type is ::arrow::binary()
+ // inferred_type is ::arrow::extension::json()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
Review Comment:
I don't have a strong opinion on this either way. The example I can think of is a another implementation without canonical types writing 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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1745768980
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -984,21 +992,51 @@ Result<bool> ApplyOriginalMetadata(const Field& origin_field, SchemaField* infer
bool modified = false;
auto& origin_type = origin_field.type();
+ auto inferred_type = inferred->field->type();
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*origin_type);
- auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
+ if (inferred_type->id() != ::arrow::Type::EXTENSION &&
+ ex_type.extension_name() == ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are DISABLED in Parquet.
+ // origin_type is ::arrow::extension::json()
+ // inferred_type is ::arrow::binary()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
+ DCHECK_EQ(inferred_type->id(), ::arrow::Type::BINARY);
+ inferred->field = inferred->field->WithType(origin_type);
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(origin_field, inferred));
+ } else {
+ auto origin_storage_field = origin_field.WithType(ex_type.storage_type());
- // Apply metadata recursively to storage type
- RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
+ // Apply metadata recursively to storage type
+ RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
- // Restore extension type, if the storage type is the same as inferred
- // from the Parquet type
- if (ex_type.storage_type()->Equals(*inferred->field->type())) {
- inferred->field = inferred->field->WithType(origin_type);
+ // Restore extension type, if the storage type is the same as inferred
+ // from the Parquet type
+ if (ex_type.storage_type()->Equals(*inferred->field->type())) {
+ inferred->field = inferred->field->WithType(origin_type);
+ }
}
modified = true;
} else {
+ if (inferred_type->id() == ::arrow::Type::EXTENSION) {
+ const auto& ex_type = checked_cast<const ::arrow::ExtensionType&>(*inferred_type);
+ if (ex_type.extension_name() ==
+ ::arrow::extension::JsonExtensionType::type_name()) {
+ // Schema mismatch.
+ //
+ // Arrow extensions are ENABLED in Parquet.
+ // origin_type is ::arrow::binary()
+ // inferred_type is ::arrow::extension::json()
+ //
+ // Origin type is restored as Arrow should be considered the source of truth.
Review Comment:
> Then would we not want to have a JSON array, rather than plain UTF8?
I suppose so! Changed:
```diff
- inferred->field = inferred->field->WithType(origin_type);
+ inferred->field = inferred->field->WithType(inferred_type);
```
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1745764195
##########
cpp/src/arrow/extension/json.cc:
##########
@@ -0,0 +1,62 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/status.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow::extension {
+
+bool JsonExtensionType::ExtensionEquals(const ExtensionType& other) const {
+ const auto& other_ext = static_cast<const ExtensionType&>(other);
Review Comment:
You mean the cast? Removing.
##########
cpp/src/arrow/extension/json_test.cc:
##########
@@ -0,0 +1,109 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include "arrow/array/validate.h"
+#include "arrow/ipc/test_common.h"
+#include "arrow/record_batch.h"
+#include "arrow/testing/gtest_util.h"
+#include "parquet/exception.h"
+
+namespace arrow {
+
+using arrow::ipc::test::RoundtripBatch;
+using extension::json;
+
+class TestJsonExtensionType : public ::testing::Test {};
+
+std::shared_ptr<Array> ExampleJson(const std::shared_ptr<DataType>& storage_type) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(storage_type, R"([
+ "null",
+ "1234",
+ "3.14159",
+ "true",
+ "false",
+ "\"a json string\"",
+ "[\"a\", \"json\", \"array\"]",
+ "{\"obj\": \"a simple json object\"}"
+ ])");
+ return ExtensionType::WrapArray(arrow::extension::json(storage_type), arr);
+}
+
+TEST_F(TestJsonExtensionType, JsonRoundtrip) {
+ for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) {
+ std::shared_ptr<Array> ext_arr = ExampleJson(storage_type);
+ auto batch =
+ RecordBatch::Make(schema({field("f0", json(storage_type))}), 8, {ext_arr});
+
+ std::shared_ptr<RecordBatch> read_batch;
+ ASSERT_OK(RoundtripBatch(batch, &read_batch));
+ ASSERT_OK(read_batch->ValidateFull());
+ CompareBatch(*batch, *read_batch, /*compare_metadata*/ true);
+
+ auto read_ext_arr = read_batch->column(0);
+ ASSERT_OK(internal::ValidateUTF8(*read_ext_arr));
+ ASSERT_OK(read_ext_arr->ValidateFull());
+ }
+}
+
+TEST_F(TestJsonExtensionType, InvalidUTF8) {
+ for (const auto& storage_type : {utf8(), large_utf8(), utf8_view()}) {
+ auto json_type = json(storage_type);
+ auto invalid_input = ArrayFromJSON(storage_type, "[\"Ⱥa\xFFⱭ\", \"Ɽ\xe1\xbdⱤaA\"]");
+ auto ext_arr =
+ ExtensionType::WrapArray(arrow::extension::json(storage_type), invalid_input);
+
+ ASSERT_RAISES_WITH_MESSAGE(Invalid,
+ "Invalid: Invalid UTF8 sequence at string index 0",
+ ext_arr->ValidateFull());
+ ASSERT_RAISES_WITH_MESSAGE(Invalid,
+ "Invalid: Invalid UTF8 sequence at string index 0",
+ arrow::internal::ValidateUTF8(*ext_arr));
+
+ auto batch = RecordBatch::Make(schema({field("f0", json_type)}), 2, {ext_arr});
+ std::shared_ptr<RecordBatch> read_batch;
+ ASSERT_OK(RoundtripBatch(batch, &read_batch));
+ }
+}
+
+class UTF8ExtensionArrayTest : public ::testing::Test {
Review Comment:
Indeed. Removed.
--
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] GH-32538: [C++][Parquet] Add JSON canonical extension type [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #13901:
URL: https://github.com/apache/arrow/pull/13901#discussion_r1772996515
##########
cpp/src/arrow/extension/json.cc:
##########
@@ -0,0 +1,61 @@
+// 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.
+
+#include "arrow/extension/json.h"
+
+#include <string>
+
+#include "arrow/extension_type.h"
+#include "arrow/result.h"
+#include "arrow/status.h"
+#include "arrow/type_fwd.h"
+#include "arrow/util/logging.h"
+
+namespace arrow::extension {
+
+bool JsonExtensionType::ExtensionEquals(const ExtensionType& other) const {
+ return other.extension_name() == this->extension_name();
+}
Review Comment:
Oh, I suppose I missed that when switching from string only to it being a parametric type. I can make a fix later today if no one started on it yet.
--
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