GH-46087: [FlightSQL] Allow returning column remarks in FlightSQL's CommandGetTables by mateuszrzeszutek · Pull Request #46110 · apache/arrow

mateuszrzeszutek · 2025-04-11T09:02:09Z

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 · 2025-04-11T09:02:39Z

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

raulcd · 2025-04-11T10:10:49Z

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

mateuszrzeszutek · 2025-04-11T11:37:55Z

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 · 2025-04-22T12:41:50Z

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

lidavidm · 2025-04-22T23:51:54Z

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 · 2025-04-22T23:53:17Z

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

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

mateuszrzeszutek · 2025-04-23T06:50:06Z

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 · 2025-04-23T07:21:42Z

CC @zeroshade, I assume no objections :)

mateuszrzeszutek · 2025-04-24T06:44:00Z

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

mateuszrzeszutek · 2025-04-24T18:22:22Z

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

lidavidm · 2025-04-25T00:01:54Z

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?

lidavidm · 2025-04-25T00:02:34Z

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 · 2025-04-27T00:34:02Z

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 · 2025-04-27T08:23:15Z

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

…QL's CommandGetTables

This reverts commit cd3008b172d57ca562a4e0809b2d01cb12d36cdc.

lidavidm · 2025-04-27T23:08:57Z

Thanks - vote in progress

singh1203

LGTM here.
Thank you!

lidavidm · 2025-05-02T01:56:49Z

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 · 2025-05-02T08:49:45Z

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 🙏

…ommandGetTables (#361) ### Rationale for this change See apache/arrow#46110 ### What changes are included in this PR? A new column metadata value ### Are these changes tested? No; there seem to be no tests that'd verify that `ColumnMetadata` works; should I add unit tests specifically for the new methods? ### Are there any user-facing changes? Yes, two new methods

…mandGetTables (#727) Resolves#737 ## What's Changed This is an implementation of apache/arrow#46110 for Java

lidavidm · 2025-05-03T09:06:24Z

Go/Java merged. Re-running integration here.

lidavidm · 2025-05-03T09:29:51Z

it passes!

lidavidm · 2025-05-03T10:39:49Z

Thanks @mateuszrzeszutek!

conbench-apache-arrow · 2025-05-03T17:52:13Z

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.

…mandGetTables (#727) Resolves #737 ## What's Changed This is an implementation of apache/arrow#46110 for Java

…'s CommandGetTables (apache#727) Resolvesapache#737 ## What's Changed This is an implementation of apache/arrow#46110 for Java

mateuszrzeszutek requested a review from lidavidm as a code owner April 11, 2025 09:02

github-actionsBot added Component: C++ Component: FlightRPC awaiting review Awaiting review labels Apr 11, 2025

mateuszrzeszutek force-pushed the remarks branch 3 times, most recently from 81b10b6 to 4bd6cf8Compare April 17, 2025 11:38

lidavidm reviewed Apr 22, 2025
View reviewed changes

github-actionsBot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Apr 22, 2025

mateuszrzeszutek force-pushed the remarks branch from 4bd6cf8 to 94a85c8Compare April 23, 2025 06:48

github-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 23, 2025

mateuszrzeszutek mentioned this pull request Apr 24, 2025
GH-737: [FlightSQL] Allow returning column remarks in FlightSQL's CommandGetTables apache/arrow-java#727
Merged

mateuszrzeszutek mentioned this pull request Apr 24, 2025
GH-46087: [FlightSQL] Allow returning column remarks in FlightSQL's CommandGetTables apache/arrow-go#361
Merged

lidavidm reviewed Apr 25, 2025
View reviewed changes

github-actionsBot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Apr 25, 2025

zeroshade approved these changes Apr 25, 2025
View reviewed changes

github-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Apr 25, 2025

mateuszrzeszutek added 5 commits April 27, 2025 10:23

apacheGH-46087: [FlightSQL] Allow returning column remarks in FlightS…
1ce18e6
…QL's CommandGetTables

revert integration test changes
48d01a4

fix the typo in FlightSql.proto
27fe039

Add extra note about the recently added metadata field
9d95fc6

restore integration test changes
40fc2f7
This reverts commit cd3008b172d57ca562a4e0809b2d01cb12d36cdc.

mateuszrzeszutek force-pushed the remarks branch from 1188285 to 40fc2f7Compare April 27, 2025 08:23

singh1203 approved these changes Apr 28, 2025
View reviewed changes

Comment threadcpp/src/arrow/flight/sql/column_metadata.h Outdated

fix a typo
448bb5a

lidavidm pushed a commit to apache/arrow-java that referenced this pull request May 3, 2025
GH-737: [FlightSQL] Allow returning column remarks in FlightSQL's Com…
8314c19
…mandGetTables (#727) Resolves#737 ## What's Changed This is an implementation of apache/arrow#46110 for Java

lidavidm merged commit 2bb5fd5 into apache:mainMay 3, 2025

lidavidm removed the awaiting merge Awaiting merge label May 3, 2025

lidavidm mentioned this pull request May 3, 2025
[FlightSQL] Allow returning column remarks in FlightSQL's CommandGetTables#46087
Closed

kapoisu mentioned this pull request May 6, 2025
GH-46217: [C++][Parquet] Update the timestamp of parquet::encryption::TwoLevelCacheWithExpiration correctly #46283
Merged

Uh oh!

Conversation

mateuszrzeszutek commented Apr 11, 2025• edited by github-actions BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actionsBot commented Apr 11, 2025

Uh oh!

raulcd commented Apr 11, 2025

Uh oh!

mateuszrzeszutek commented Apr 11, 2025

Uh oh!

mateuszrzeszutek commented Apr 22, 2025

Uh oh!

lidavidm commented Apr 22, 2025

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

lidavidmApr 22, 2025

Choose a reason for hiding this comment

Uh oh!

mateuszrzeszutekApr 23, 2025

Choose a reason for hiding this comment

Uh oh!

mateuszrzeszutek commented Apr 23, 2025

Uh oh!

lidavidm commented Apr 23, 2025

Uh oh!

mateuszrzeszutek commented Apr 24, 2025

Uh oh!

mateuszrzeszutek commented Apr 24, 2025

Uh oh!

lidavidmApr 25, 2025

Choose a reason for hiding this comment

Uh oh!

mateuszrzeszutekApr 25, 2025

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Apr 25, 2025

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Apr 27, 2025

Uh oh!

mateuszrzeszutek commented Apr 27, 2025

Uh oh!

lidavidm commented Apr 27, 2025

Uh oh!

singh1203 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lidavidm commented May 2, 2025

Uh oh!

mateuszrzeszutek commented May 2, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lidavidm commented May 3, 2025

Uh oh!

lidavidm commented May 3, 2025

Uh oh!

lidavidm commented May 3, 2025

Uh oh!

conbench-apache-arrowBot commented May 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mateuszrzeszutek commented Apr 11, 2025•
edited by github-actions Bot
Loading

mateuszrzeszutek commented May 2, 2025•
edited
Loading