GH-46087: [FlightSQL] Allow returning column remarks in FlightSQL's CommandGetTables

[mateuszrzeszutek]

Resolves #46087

Rationale for this change

FlightSQL allows returning various column metadata in CommandGetTables, but one thing that's missing is human-readable column description. This PR proposes adding a new ARROW:FLIGHT:SQL:REMARKS metadata property taht will contain a comment describing a column. This is inspired by JDBC's DatabaseMetaData#getColumns() method, and later on I'm planning on adding this change to arrow-java as well.

What changes are included in this PR?

• A new column metadata property ARROW:FLIGHT:SQL:REMARKS
• C++ ColumnMetadata implementation

Please tell me if there's anything else in the other languages that I should add.

Are these changes tested?

Covered by existing tests; no new test cases added.

Are there any user-facing changes?

Yes, a couple new constants/methods added to the ColumnMetadata class and its builder

• GitHub Issue: [FlightSQL] Allow returning column remarks in FlightSQL's CommandGetTables #46087

[github-actions]

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

[raulcd]

macOS 13 and MinGW failures are unrelated and failing on other PRs. See:

• [CI][C++] macOS 13 job fails to build utf8.cc #46077
• [CI][C++] PR jobs for MINGW fail with 'boost/process/v1.hpp' file not found #46111

[mateuszrzeszutek]

Looks like the integration tests have failed on the C++ vs Go/Java comparison:

 panic: expected: schema:
  	  fields: 1
  	    - id: type=int64, nullable
  	    metadata: ["ARROW:FLIGHT:SQL:TABLE_NAME": "test", "ARROW:FLIGHT:SQL:IS_AUTO_INCREMENT": "1", "ARROW:FLIGHT:SQL:IS_CASE_SENSITIVE": "0", "ARROW:FLIGHT:SQL:TYPE_NAME": "type_test", "ARROW:FLIGHT:SQL:SCHEMA_NAME": "schema_test", "ARROW:FLIGHT:SQL:IS_SEARCHABLE": "1", "ARROW:FLIGHT:SQL:CATALOG_NAME": "catalog_test", "ARROW:FLIGHT:SQL:PRECISION": "100"], got: schema:
  	  fields: 1
  	    - id: type=int64, nullable
  	    metadata: ["ARROW:FLIGHT:SQL:TABLE_NAME": "test", "ARROW:FLIGHT:SQL:IS_AUTO_INCREMENT": "1", "ARROW:FLIGHT:SQL:IS_CASE_SENSITIVE": "0", "ARROW:FLIGHT:SQL:TYPE_NAME": "type_test", "ARROW:FLIGHT:SQL:SCHEMA_NAME": "schema_test", "ARROW:FLIGHT:SQL:IS_SEARCHABLE": "1", "ARROW:FLIGHT:SQL:CATALOG_NAME": "catalog_test", "ARROW:FLIGHT:SQL:PRECISION": "100", "ARROW:FLIGHT:SQL:REMARKS": "test column"]

I've reverted the changes to the integration test code here.
I assume there's a dependency between this repo and arrow-go/arrow-java -- should they be updated in some sort of order? Or should the assertions in the other repos be relaxed first?

[mateuszrzeszutek]

Hey @lidavidm , any chance you can take a look at this PR?

[lidavidm]

I believe if we want to add this to the spec we should have implementations in at least one other language (preferably both Go/Java if possible, though) and vote on it, as trivial as it is

[lidavidm]

The proposal itself seems reasonable, though

[lidavidm]

nit: maybe "An explanatory comment" or something (following the JDBC documentation)? Or was this meant to say "A comment describing the column"?

[mateuszrzeszutek]

Yup, sorry for the typo -- I changed that to "A comment describing the column"

[mateuszrzeszutek]

I believe if we want to add this to the spec we should have implementations in at least one other language (preferably both Go/Java if possible, though) and vote on it, as trivial as it is

Sure, makes sense 👍 . I'll work on the draft PRs and post links here.

[lidavidm]

CC @zeroshade, I assume no objections :)

[mateuszrzeszutek]

@lidavidm here's the draft implementation for Java apache/arrow-java#727
I'll prepare a PR for Go as well

[mateuszrzeszutek]

And here's the Go PR: apache/arrow-go#361

[lidavidm]

One thing that I was thinking about, though, should we note that this field was added after the others and clients should be prepared to find it missing?

[mateuszrzeszutek]

Done

[lidavidm]

Thanks for the PRs. They look reasonable to me. I'd like to let zeroshade take a look and then we can call a vote.

[zeroshade]

No objections from me here. I'll review the Go PR, but I'd say we can move forward with a vote.

[lidavidm]

Ah, should we restore the integration test as well? We've required that for votes before. It's OK if it's failing for now. As long as the test is present we can vote and then we can merge the PRs in order.

[mateuszrzeszutek]

It's OK if it's failing for now. As long as the test is present we can vote and then we can merge the PRs in order.

Okay -- I'll add the changes needed to make the tests work to the Java/Go PRs as well

[lidavidm]

Thanks - vote in progress

[singh1203]

LGTM here.
Thank you!

[lidavidm]

I've closed the vote.

In terms of how to merge this, I think we can merge Go/Java first, then if we rebase here, the integration tests should pass?

[mateuszrzeszutek]

In terms of how to merge this, I think we can merge Go/Java first, then if we rebase here, the integration tests should pass?

Sounds good to me 👍

Both Java and Go PRs are ready to review, PTAL at them 🙏

[lidavidm]

Go/Java merged. Re-running integration here.

[lidavidm]

it passes!

[lidavidm]

Thanks @mateuszrzeszutek!

[conbench-apache-arrow]

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

There were 6 benchmark results indicating a performance regression:

• Commit Run on test-mac-arm at 2025-05-03 12:14:21Z
  • tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-10, scale_factor=1
  • tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-03, scale_factor=1
• and 4 more (see the report linked below)

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