GH-44214: [C++] JsonExtensionType equality check ignores storage type #44215

rok · 2024-09-24T11:58:13Z

Rationale for this change

bool JsonExtensionType::ExtensionEquals(const ExtensionType& other) const {
  return other.extension_name() == this->extension_name();
}

This equality check does not take into account the storage type, but only the name.
As a consequence, a JsonExtensionType type will be seen as equal to JsonExtensionType<large_string>.

What changes are included in this PR?

This change introduces storage equality check into JsonExtensionType equality check.

This also fixes a storage type check in JsonExtensionType::Make.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

GitHub Issue: [C++] JsonExtensionType equality check ignores storage type #44214

github-actions · 2024-09-24T11:58:40Z

⚠️ GitHub issue #44214 has been automatically assigned in GitHub to PR creator.

cpp/src/arrow/extension/json.h

pitrou · 2024-09-24T12:27:55Z

cpp/src/parquet/arrow/arrow_schema_test.cc

@@ -763,7 +763,7 @@ TEST_F(TestConvertParquetSchema, ParquetSchemaArrowExtensions) {
    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()),
+         ::arrow::field("json_2", ::arrow::extension::json(::arrow::utf8()),


Instead of making the test easier, can you keep large_utf8 and ensure it passes?

Can we reconstruct storage type if it is not stored? In this case we know data is of JSON logical type, but we don't know what was it's storage type at write time. Am I missing something?

(There was a test where arrow schema was available and wasn't used at read time. I've addressed that)

Can we reconstruct storage type if it is not stored? In this case we know data is of JSON logical type, but we don't know what was it's storage type at write time. Am I missing something?

No, I'm asking you to write the test so that it reflects that.

Something like this? e522efb

Added another assertion to justify changing arrow_schema https://github.com/apache/arrow/pull/44215/files/94006f92085377fc4b72c2a2da844e36fb4d86d3..77c964b04972fc08f04957190866fb475b453c3c.
Let me know if something else should be changed.

jorisvandenbossche · 2024-09-25T14:16:01Z

cpp/src/parquet/arrow/schema.cc

-      if (ex_type.storage_type()->Equals(*inferred->field->type())) {
+      if (ex_type.storage_type()->Equals(*inferred->field->type()) ||
+          (ex_type.extension_name() == "arrow.json" &&
+           !ex_type.storage_type()->Equals(*inferred->field->type()))) {


This "types are not equal" check is superfluous? Because if they were equal, the first part of the if check would have passed and we wouldn't get here?

I assume that in case of JSON extension type, we either just have to assume that the data will correctly use string, or we check explicitly that the inferred type is string type?

It is superfluous indeed. Changed as per your suggestion: 3364c0f

pitrou · 2024-09-25T15:50:26Z

cpp/src/arrow/extension/json.cc

  return std::make_shared<JsonExtensionType>(storage_type);
 }

+std::shared_ptr<DataType> json(std::shared_ptr<DataType> storage_type) {
+  return JsonExtensionType::Make(storage_type).ValueOrDie();


Suggested change

return JsonExtensionType::Make(storage_type).ValueOrDie();

return JsonExtensionType::Make(std::move(storage_type)).ValueOrDie();

Also, please ensure that JsonExtensionType::Make takes a std::shared_ptr<DataType>.

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

pitrou

Remaining nits

cpp/src/arrow/extension/json.cc

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

rok · 2024-09-25T16:58:22Z

Remaining nits

@pitrou added your changes.

wgtmac · 2024-09-26T02:03:26Z

cpp/src/arrow/extension/json.cc

-  return std::make_shared<JsonExtensionType>(storage_type);
+Result<std::shared_ptr<DataType>> JsonExtensionType::Make(
+    std::shared_ptr<DataType> storage_type) {
+  if (storage_type->id() != Type::STRING && storage_type->id() != Type::STRING_VIEW &&


Should we add the following static function to JsonExtensionType to remove repeated code here and in cpp/src/parquet/arrow/schema.cc below?

static bool IsSupportedStorageType(Type::type type_id) { return type_id == Type::STRING || type_id == Type::STRING_VIEW || type_id == Type::LARGE_STRING; }

Changed as suggested: 6dff127

rok added this to the 18.0.0 milestone Sep 24, 2024

rok linked an issue Sep 24, 2024 that may be closed by this pull request

[C++] JsonExtensionType equality check ignores storage type #44214

Open

github-actions bot added Component: C++ awaiting committer review Awaiting committer review labels Sep 24, 2024

rok mentioned this pull request Sep 24, 2024

GH-32538: [C++][Parquet] Add JSON canonical extension type #13901

Merged

rok requested review from pitrou and jorisvandenbossche September 24, 2024 11:59

Stricter equality check

4d591e5

rok force-pushed the json_support_followup branch from 72bb26c to 4d591e5 Compare September 24, 2024 12:19

rok requested a review from wgtmac as a code owner September 24, 2024 12:19

github-actions bot added the Component: Parquet label Sep 24, 2024

pitrou requested changes Sep 24, 2024

View reviewed changes

rok added 2 commits September 24, 2024 14:57

Review feedback

60eda4a

Change how extension type storage is restored after reading parquet

1ba3ba7

rok force-pushed the json_support_followup branch from 0c265ab to 1ba3ba7 Compare September 24, 2024 20:10

lint

94006f9

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Sep 24, 2024

rok requested a review from pitrou September 24, 2024 20:33

Add test case

e522efb

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Sep 25, 2024

Add another assertion

b1e9241

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 25, 2024

rok added 2 commits September 25, 2024 11:49

lint

4806d9f

Reintroduce assert

77c964b

jorisvandenbossche reviewed Sep 25, 2024

View reviewed changes

Review feedback

3364c0f

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Sep 25, 2024

pitrou reviewed Sep 25, 2024

View reviewed changes

Update cpp/src/arrow/extension/json.cc

b2bca96

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Sep 25, 2024

Review feedback

fdf8fb6

rok force-pushed the json_support_followup branch from cb763e0 to fdf8fb6 Compare September 25, 2024 16:07

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 25, 2024

rok requested a review from pitrou September 25, 2024 16:55

pitrou reviewed Sep 25, 2024

View reviewed changes

cpp/src/arrow/extension/json.cc Outdated Show resolved Hide resolved

cpp/src/arrow/extension/json.cc Outdated Show resolved Hide resolved

Apply suggestions from code review

99d8ca7

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

rok requested a review from pitrou September 25, 2024 17:27

wgtmac reviewed Sep 26, 2024

View reviewed changes

rok requested a review from jorisvandenbossche September 26, 2024 09:32

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 26, 2024

rok force-pushed the json_support_followup branch from 1dd3ec2 to a415aaf Compare September 26, 2024 13:31

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 26, 2024

Review feedback

6dff127

rok force-pushed the json_support_followup branch from a415aaf to 6dff127 Compare September 26, 2024 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-44214: [C++] JsonExtensionType equality check ignores storage type #44215

GH-44214: [C++] JsonExtensionType equality check ignores storage type #44215

rok commented Sep 24, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Sep 24, 2024

pitrou Sep 24, 2024

rok Sep 24, 2024

rok Sep 24, 2024

pitrou Sep 25, 2024

rok Sep 25, 2024

pitrou Sep 25, 2024

rok Sep 25, 2024

jorisvandenbossche Sep 25, 2024

rok Sep 25, 2024

pitrou Sep 25, 2024

rok Sep 25, 2024

pitrou left a comment

rok commented Sep 25, 2024

wgtmac Sep 26, 2024

rok Sep 26, 2024 •

edited

Loading

	return JsonExtensionType::Make(storage_type).ValueOrDie();
	return JsonExtensionType::Make(std::move(storage_type)).ValueOrDie();

GH-44214: [C++] JsonExtensionType equality check ignores storage type #44215

Are you sure you want to change the base?

GH-44214: [C++] JsonExtensionType equality check ignores storage type #44215

Conversation

rok commented Sep 24, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou left a comment

Choose a reason for hiding this comment

rok commented Sep 25, 2024

Choose a reason for hiding this comment

rok Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

rok commented Sep 24, 2024 •

edited by github-actions bot

Loading

rok Sep 26, 2024 •

edited

Loading