ARROW-15314: [C++][Java][FlightRPC] Add missing metadata on Arrow schemas returned by Flight SQL #11999

jcralmeida · 2021-12-20T17:08:52Z

This add an auxiliary class FlightSqlColumnMetadata (Java) and ColumnMetadata(CPP) meant to read and write known metadata for Arrow schema fields, such as:

CATALOG_NAME
SCHEMA_NAME
TABLE_NAME
PRECISION
SCALE
IS_AUTO_INCREMENT
IS_CASE_SENSITIVE
IS_READ_ONLY
IS_SEARCHABLE

github-actions · 2021-12-20T17:09:13Z

Thanks for opening a pull request!

If this is not a minor PR. 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 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:

jcralmeida · 2021-12-20T17:12:47Z

@lidavidm Here is the changes related to the column metadata that @jduo was talking about.

Could you take a look and share your thoughts, please?

lidavidm

Is the metadata here sufficient to cover at least JDBC and ODBC?

format/FlightSql.proto

java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlColumnMetadata.java

format/FlightSql.proto

jcralmeida · 2021-12-20T20:19:08Z

Is the metadata here sufficient to cover at least JDBC and ODBC?

Yes, we thought of this values thinking on both JDBC and ODBC.

lidavidm · 2021-12-21T13:40:41Z

Just a heads up, this needs rebasing now (sorry for the churn).

Given there are some format changes here, we can bundle this with the type info method vote.

jcralmeida · 2021-12-23T10:34:22Z

No problem. I've already rebased both PRs.

…of the map in column metadata

cpp/src/arrow/flight/sql/column_metadata.cc

cpp/src/arrow/flight/sql/column_metadata.h

format/FlightSql.proto

jcralmeida · 2022-01-11T21:59:29Z

Sure @lidavidm, let's combine both implementation and make one vote.

By the way we don't have the Jira Ticket for this, where can I create the ticket?

lidavidm · 2022-01-11T22:12:35Z

It can be created here: https://issues.apache.org/jira/secure/Dashboard.jspa

Then just update the PR title as described above: #11999 (comment)

github-actions · 2022-01-12T16:52:55Z

https://issues.apache.org/jira/browse/ARROW-15314

github-actions · 2022-01-12T16:52:56Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

lidavidm · 2022-01-12T16:57:38Z

Thanks for linking this up. BTW, are integration tests planned here too?

jcralmeida · 2022-01-12T17:19:20Z

@lidavidm Sorry, I've found an error here. I'm fixing it and I will let you know when I finish it

lidavidm

Thanks. Just one little thing I noticed on a last pass.

cpp/src/arrow/flight/sql/server_test.cc

jcralmeida · 2022-01-12T21:03:11Z

@lidavidm I've added the integration test for both PRs and fix the nits on both PRs

pitrou · 2022-02-07T09:33:56Z

format/FlightSql.proto

   * - return 4   (\b100)   => [SQL_SUBQUERIES_IN_INS];
   * - return 5   (\b101)   => [SQL_SUBQUERIES_IN_COMPARISONS, SQL_SUBQUERIES_IN_INS];
-   * - return 6   (\b110)   => [SQL_SUBQUERIES_IN_INS, SQL_SUBQUERIES_IN_EXISTS];
+   * - return 6   (\b110)   => [SQL_SUBQUERIES_IN_COMPARISONS, SQL_SUBQUERIES_IN_EXISTS];


Is this change valid? Judging by the above, SQL_SUBQUERIES_IN_COMPARISONS is 0b001 while SQL_SUBQUERIES_IN_INS is 0b100.

Yes, this seems wrong. Additionally the enum values are sequential (0, 1, 2, 3) not bitmasks (unless those values are intended to be bit indices in which case we should document them as such).

@jcralmeida any comment here? I forgot about this, let's fix these last few things and then kick off the vote

Oh sorry @lidavidm , I've changed back to how it was which was the correct one. I don't know how/why I've changed it

pitrou · 2022-02-07T09:37:10Z

format/FlightSql.proto

+ *  - ARROW:FLIGHT:SQL:DB_SCHEMA_NAME    - Database schema name
+ *  - ARROW:FLIGHT:SQL:TABLE_NAME        - Table name
+ *  - ARROW:FLIGHT:SQL:PRECISION         - Column precision/size
+ *  - ARROW:FLIGHT:SQL:SCALE             - Column scale/decimal digits


Why are precision and scale useful? Can they annotate non-decimal columns?

This is also requested by the JDBC and ODBC, to be extracted by them via ResultSetMetadata

Should we annotate this as (if applicable for the column type) or something?

It makes senses. Added

pitrou · 2022-02-07T09:40:28Z

format/FlightSql.proto


 /*
 * Represents a SQL query. Used in the command member of FlightDescriptor
 * for the following RPC calls:


Why does this have the same description as CommandGetTables? Do we have two different messages for doing the same thing?

The message itself is different, what is similar here is the response since the column metadata can be retrieved via both command

lidavidm · 2022-03-15T19:45:33Z

There was one doc lint hidden in a pipeline:

/arrow/cpp/src/arrow/flight/sql/column_metadata.h:139: error: argument 'IsAutoIncrement' of command @param is not found in the argument list of arrow::flight::sql::ColumnMetadata::ColumnMetadataBuilder::IsAutoIncrement(bool is_auto_increment) (warning treated as error, aborting now)

I think other failures are flakes/unrelated.

jcralmeida · 2022-03-16T12:09:04Z

Sorry @lidavidm. There were some errors related to the cases of the parameter. I've already fixed them

jcralmeida · 2022-03-16T18:02:24Z

Hi @lidavidm I've got some failures on some CI jobs but according to I'm seeing they are unrelated to code. Is it right?

lidavidm · 2022-03-16T18:54:04Z

I believe they're unrelated, I'll kick them to try again (the Java one is definitely unrelated)

pitrou · 2022-03-21T17:55:33Z

format/FlightSql.proto

+ *  - ARROW:FLIGHT:SQL:CATALOG_NAME      - Table's catalog name
+ *  - ARROW:FLIGHT:SQL:DB_SCHEMA_NAME    - Database schema name
+ *  - ARROW:FLIGHT:SQL:TABLE_NAME        - Table name


Is there a reason these three are duplicated from the fields above?

Just because they should unified in every place it appears

pitrou · 2022-03-21T17:56:05Z

format/FlightSql.proto

+ *  - ARROW:FLIGHT:SQL:IS_CASE_SENSITIVE - "1" indicates if the column is case sensitive, "0" otherwise.
+ *  - ARROW:FLIGHT:SQL:IS_READ_ONLY      - "1" indicates if the column is read only, "0" otherwise.
+ *  - ARROW:FLIGHT:SQL:IS_SEARCHABLE     - "1" indicates if the column is searchable via WHERE clause, "0" otherwise.
 * The returned data should be ordered by catalog_name, db_schema_name, table_name, then table_type, followed by table_schema if requested.


Ordering by table_schema is just a bytewise lexicographic ordering?

pitrou

Just some questions below.

ursabot · 2022-03-25T21:21:18Z

Benchmark runs are scheduled for baseline = acc6c2e and contender = c2fac05. c2fac05 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.67% ⬆️0.34%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.04% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

…emas returned by Flight SQL This add an auxiliary class FlightSqlColumnMetadata (Java) and ColumnMetadata(CPP) meant to read and write known metadata for Arrow schema fields, such as: - CATALOG_NAME - SCHEMA_NAME - TABLE_NAME - PRECISION - SCALE - IS_AUTO_INCREMENT - IS_CASE_SENSITIVE - IS_READ_ONLY - IS_SEARCHABLE Closes apache#11999 from jcralmeida/flight-sql-column-metadata Authored-by: Jose Almeida <[email protected]> Signed-off-by: David Li <[email protected]>

github-actions bot added the Component: Java label Dec 20, 2021

lidavidm reviewed Dec 20, 2021

View reviewed changes

lidavidm force-pushed the flight-sql branch from 9c2d8ed to 22bd624 Compare December 20, 2021 23:07

jcralmeida force-pushed the flight-sql-column-metadata branch from 70d11ea to 5ff5df7 Compare December 23, 2021 10:27

jcralmeida added 9 commits January 10, 2022 16:45

Fix typo and checkstyle on FlightSql.proto

7351c63

Implement logic of column metadata

7dba990

Revert change from SqlInfo commentary

53222af

Improve documentation of the column metadata

2e564be

Add namespace to column metadata and make changes to prevent mutable …

bdb3a24

…of the map in column metadata

Implement column metadata on flight sql cpp module

8664a31

Improve documentation of protobuf and add namespace to metadata names

d59a2c1

Move anonymous namespace inside the arrow:flight:sql namespaces

9534729

Rename const variables and set getter methods as const

b172c92

jcralmeida force-pushed the flight-sql-column-metadata branch from 5ff5df7 to b172c92 Compare January 10, 2022 19:52

jcralmeida changed the base branch from flight-sql to master January 10, 2022 19:53

github-actions bot added Component: FlightRPC Component: C++ labels Jan 10, 2022

jcralmeida changed the title ~~[JAVA] Add missing metadata on Arrow schemas returned by Flight SQL~~ Add missing metadata on Arrow schemas returned by Flight SQL Jan 10, 2022

jcralmeida mentioned this pull request Jan 10, 2022

[CPP] Add missing metadata on Arrow schemas returned by Flight SQL #11999 #12002

Closed

lidavidm reviewed Jan 10, 2022

View reviewed changes

cpp/src/arrow/flight/sql/column_metadata.cc Outdated Show resolved Hide resolved

cpp/src/arrow/flight/sql/column_metadata.h Outdated Show resolved Hide resolved

cpp/src/arrow/flight/sql/column_metadata.h Outdated Show resolved Hide resolved

format/FlightSql.proto Show resolved Hide resolved

jcralmeida added 5 commits January 11, 2022 11:32

Change string boolean values from YES/NO to 1/0

a65f49c

Inline return of the ColumnMetadataBuilder

656f0b6

Make getter of metadata_map as const and adjust code style

15541a0

Explicit declare variable as private and move it to the of file

69d7e6c

Refactor of metadata_map getter in the code

a8d796a

jcralmeida changed the title ~~Add missing metadata on Arrow schemas returned by Flight SQL~~ ARROW-15314: [Flight SQL] Add missing metadata on Arrow schemas returned by Flight SQL Jan 12, 2022

lidavidm changed the title ~~ARROW-15314: [Flight SQL] Add missing metadata on Arrow schemas returned by Flight SQL~~ ARROW-15314: [C++][Java][FlightRPC] Add missing metadata on Arrow schemas returned by Flight SQL Jan 12, 2022

jcralmeida added 2 commits January 12, 2022 16:48

Add column metadata to integration test on flight sql

fa3b1a3

Fix code style

19ce860

lidavidm approved these changes Jan 12, 2022

View reviewed changes

cpp/src/arrow/flight/sql/server_test.cc Show resolved Hide resolved

lidavidm approved these changes Jan 13, 2022

View reviewed changes

pitrou reviewed Feb 7, 2022

View reviewed changes

vfraga mentioned this pull request Mar 11, 2022

ARROW-15452: [FlightRPC][Java] JDBC driver for Flight SQL #12254

Closed

jcralmeida added 2 commits March 15, 2022 13:33

Fix documentation of SQL_SUPPORTED_SUBQUERIES enum in FlightSql.proto

d34c081

Improve documentation of ARROW:FLIGHT:SQL:SCALE in FlightSql.proto

e0948df

Fix cases from params to snake_case in column_metadata.h

297df6b

pitrou reviewed Mar 21, 2022

View reviewed changes

lidavidm closed this in c2fac05 Mar 25, 2022

ARROW-15314: [C++][Java][FlightRPC] Add missing metadata on Arrow schemas returned by Flight SQL #11999

ARROW-15314: [C++][Java][FlightRPC] Add missing metadata on Arrow schemas returned by Flight SQL #11999

Uh oh!

Conversation

jcralmeida commented Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 20, 2021

Uh oh!

jcralmeida commented Dec 20, 2021

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcralmeida commented Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lidavidm commented Dec 21, 2021

Uh oh!

jcralmeida commented Dec 23, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcralmeida commented Jan 11, 2022

Uh oh!

lidavidm commented Jan 11, 2022

Uh oh!

github-actions bot commented Jan 12, 2022

Uh oh!

github-actions bot commented Jan 12, 2022

Uh oh!

lidavidm commented Jan 12, 2022

Uh oh!

jcralmeida commented Jan 12, 2022

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcralmeida commented Jan 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Mar 15, 2022

Uh oh!

jcralmeida commented Mar 16, 2022

Uh oh!

jcralmeida commented Mar 16, 2022

Uh oh!

lidavidm commented Mar 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcralmeida commented Dec 20, 2021 •

edited

Loading

jcralmeida commented Dec 20, 2021 •

edited

Loading

pitrou Feb 7, 2022 •

edited

Loading

ursabot commented Mar 25, 2022 •

edited

Loading