GH-1134: add COLUMN_DEF support to JDBC getColumnns()#1139
Conversation
This comment has been minimized.
This comment has been minimized.
Verest
requested review from
jbonofre,
laurentgo,
lidavidm and
wgtmac
as code owners
|
Should this be discussed and voted like it was done for is_update? Maybe it is overkill but I think at least apache/arrow/FlightSql.proto should be in sync. There are other places in FlightSql.proto that also mention metadata returned that probably would need to be updated |
|
Have you thought if it would make sense in CommandGetXdbcTypeInfo? |
Yes, the proto files in this repo should mirror the main one. |
| * - 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. | ||
| * - ARROW:FLIGHT:SQL:REMARKS - A comment describing the column. | ||
| * - ARROW:FLIGHT:SQL:COLUMN_DEF - The default value for the column. |
There was a problem hiding this comment.
Thanks to have update the javadoc here.
I think it's worth to document COLUMN_DEF in the javadoc of CommandStatementQuery, CommandStatementSubstraitPlan, CommandPreparedStatementQuery. As we have REMARKS in these messages, we should have COLUMN_DEF.
There was a problem hiding this comment.
Yes, I was uncertain about that - I can add them.
I did not add them originally as I do not think the COLUMN_DEF will be used in those code paths, but I don't think it hurts having it there either.
|
By the way, as this PR touches |
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
It seems reasonable to me. We should proceed with a vote on ML
5 participants
What's Changed
Updates the
FlightSqlColumnMetadatato contain the JDBCCOLUMN_DEFfield alongside getters/setters. Following exiting naming format.Updates
ArrowDatabaseMetadatato write to the pre-existingCOLUMN_DEFvector as part of thegetColumns()code paths.Updates test(s) in
ArrowDatabaseMetadataTestto test default value behavior.Closes #1134.