GH-39864: [C++] DataType::ToString support optionally show metadata

[Cerdore]

Rationale for this change

Support showing metadata of nested DataType which have child fields.

What changes are included in this PR?

Add an optional argument "show_metadata" to the ToString() of DataType and other DataType derived class. And we also add it to TypeHolder::ToString().

Are these changes tested?

Yes, I add tests for changes.

Are there any user-facing changes?

No.

Closes: #39864

• Closes: [C++] DataType::ToString should optionally show metadata #39864

[github-actions]

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

[Cerdore]

MapType::ToString() don't show the metadata currently. It use type().ToString() and field.name().

I didn't change here.

  const auto print_field_name = [](std::ostream& os, const Field& field,
                                   const char* std_name) {
    if (field.name() != std_name) {
      os << " ('" << field.name() << "')";
    }
  };
  const auto print_field = [&](std::ostream& os, const Field& field,
                               const char* std_name) {
    os << field.type()->ToString(show_metadata);
    print_field_name(os, field, std_name);
  };

[westonpace]

This looks like a reasonable addition to me. I think those python test failures might be relevant though (looks like the gdb extension might be calling ToString somewhere and need to explicitly pass a show_metadata arg)

[pitrou]

Can you also change AssertTypeEqual so that it shows the metadata on error?

[Cerdore]

Can you also change AssertTypeEqual so that it shows the metadata on error?

AssertTypeEqual(ty1, ty2, /*check_metadata*/true);
Should I do it above way or modify the implementation of AssertTypeEqual?

[pitrou]

I mean change these now that DataType::ToString accepts a show_metadata argument:

arrow/cpp/src/arrow/testing/gtest_util.cc

Lines 235 to 242 in 8ffc214

	template <typename T>
	std::string ToStringWithMetadata(const T& t, bool show_metadata) {
	return t.ToString(show_metadata);
	}
	std::string ToStringWithMetadata(const DataType& t, bool show_metadata) {
	return t.ToString();
	}

[Cerdore]

I mean change these now that DataType::ToString accepts a show_metadata argument:

Thank you! I see.

[pitrou]

Thanks, but this can actually be simplified now. ToStringWithMetadata was created because DataType::ToString didn't support the show_metadata argument. Now that it does, ToStringWithMetadata can probably be suppressed and all calls to ToStringWithMetadata(t, show_metadata) replaced with t.ToString(show_metadata).

[pitrou]

I've made a few minor changes. Thanks @Cerdore for tackling this, I'll be merging this PR if CI passes.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit c5f60a0.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 12 possible false positives for unstable benchmarks that are known to sometimes produce them.

[Cerdore]

I've made a few minor changes. Thanks @Cerdore for tackling this, I'll be merging this PR if CI passes.

Sorry, I've been busy with some other things lately... And thank you very much for your suggestions and subsequent changes!