Re: [PR] GH-1134: add COLUMN_DEF support to JDBC getColumnns() [arrow-java]
Verest commented on PR #1139: URL: https://github.com/apache/arrow-java/pull/1139#issuecomment-4399092007 > Have you thought if it would make sense in [CommandGetXdbcTypeInfo](https://github.com/apache/arrow/blob/2a89d03bbefd620b42126b8e00f8ae57e99cd638/format/FlightSql.proto#L1132)? Off-hand I would say a defaultValue is N/A for that class as it appears to be generic to SQL types rather than specific columns - A default value is only relevant for a specific column of a table. Maybe something along the lines of "supports_default_values" could be applicable there, but that would be out of scope for this PR. I am murky on the specifics of that class regardless. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-1134: add COLUMN_DEF support to JDBC getColumnns() [arrow-java]
Verest commented on code in PR #1139:
URL: https://github.com/apache/arrow-java/pull/1139#discussion_r3202882546
##
arrow-format/FlightSql.proto:
##
@@ -1213,6 +1213,7 @@ message CommandGetDbSchemas {
* - 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.
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-1134: add COLUMN_DEF support to JDBC getColumnns() [arrow-java]
jbonofre commented on PR #1139: URL: https://github.com/apache/arrow-java/pull/1139#issuecomment-4396796405 By the way, as this PR touches `.proto`, worth to at discuss on the mailing list. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-1134: add COLUMN_DEF support to JDBC getColumnns() [arrow-java]
jbonofre commented on code in PR #1139:
URL: https://github.com/apache/arrow-java/pull/1139#discussion_r3201133850
##
arrow-format/FlightSql.proto:
##
@@ -1213,6 +1213,7 @@ message CommandGetDbSchemas {
* - 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.
Review 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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] GH-1134: add COLUMN_DEF support to JDBC getColumnns() [arrow-java]
ennuite commented on PR #1139: URL: https://github.com/apache/arrow-java/pull/1139#issuecomment-4393294721 > Should this be discussed and voted like it was done for [is_update](https://lists.apache.org/thread/88msflnwkkw8t81czs2ndqhkn1fb1pxd)? Maybe it is overkill but I think at least [apache/arrow/FlightSql.proto](https://github.com/apache/arrow/blob/main/format/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 Yes, the proto files in this repo should mirror the main one. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-1134: add COLUMN_DEF support to JDBC getColumnns() [arrow-java]
xborder commented on PR #1139: URL: https://github.com/apache/arrow-java/pull/1139#issuecomment-4391943447 Have you thought if it would make sense in (CommandGetXdbcTypeInfo)[https://github.com/apache/arrow/blob/2a89d03bbefd620b42126b8e00f8ae57e99cd638/format/FlightSql.proto#L1132]? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-1134: add COLUMN_DEF support to JDBC getColumnns() [arrow-java]
xborder commented on PR #1139: URL: https://github.com/apache/arrow-java/pull/1139#issuecomment-4391894276 Should this be discussed and voted like it was done for [is_update](https://lists.apache.org/thread/88msflnwkkw8t81czs2ndqhkn1fb1pxd)? Maybe it is overkill but I think at least [apache/arrow/FlightSql.proto](https://github.com/apache/arrow/blob/main/format/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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-1134: add COLUMN_DEF support to JDBC getColumnns() [arrow-java]
Verest commented on PR #1139: URL: https://github.com/apache/arrow-java/pull/1139#issuecomment-4391480286 I am murky on the specific format for `COLUMN_DEF`, or where that responsibility lies. The Oracle [docs](https://docs.oracle.com/javase/8/docs/api/java/sql/DatabaseMetaData.html) mention wrapping the underling value in single quotes if it is a string, and a few other things. I suspect this format is not followed everywhere regardless 🤔. Since this is a generic JDBC driver, I am cautious to make assumptions about the incoming format of the COLUMN_DEF from the server. For now, I am tentatively expecting the server-side to handle the intended parsing of the COLUMN_DEF, leaving the arrow driver to read given strings as is. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-1134: add COLUMN_DEF support to JDBC getColumnns() [arrow-java]
github-actions[bot] commented on PR #1139: URL: https://github.com/apache/arrow-java/pull/1139#issuecomment-4391386433 Thank you for opening a pull request! Please label the PR with one or more of: - bug-fix - chore - dependencies - documentation - enhancement Also, add the 'breaking-change' label if appropriate. See [CONTRIBUTING.md](https://github.com/apache/arrow-java/blob/main/CONTRIBUTING.md) for details. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-1134: add COLUMN_DEF support to JDBC getColumnns() [arrow-java]
Verest closed pull request #1138: GH-1134: add COLUMN_DEF support to JDBC getColumnns() URL: https://github.com/apache/arrow-java/pull/1138 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-1134: add COLUMN_DEF support to JDBC getColumnns() [arrow-java]
Verest closed pull request #1138: GH-1134: add COLUMN_DEF support to JDBC getColumnns() URL: https://github.com/apache/arrow-java/pull/1138 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-1134: add COLUMN_DEF support to JDBC getColumnns() [arrow-java]
github-actions[bot] commented on PR #1138: URL: https://github.com/apache/arrow-java/pull/1138#issuecomment-4391216735 Thank you for opening a pull request! Please label the PR with one or more of: - bug-fix - chore - dependencies - documentation - enhancement Also, add the 'breaking-change' label if appropriate. See [CONTRIBUTING.md](https://github.com/apache/arrow-java/blob/main/CONTRIBUTING.md) for details. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
