You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "lidavidm (via GitHub)" <gi...@apache.org> on 2023/05/17 13:17:54 UTC
[GitHub] [arrow-adbc] lidavidm opened a new pull request, #686: feat(format): add AdbcStatementExecuteSchema
lidavidm opened a new pull request, #686:
URL: https://github.com/apache/arrow-adbc/pull/686
Fixes #318.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #686: feat(format): add AdbcStatementExecuteSchema
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #686:
URL: https://github.com/apache/arrow-adbc/pull/686#discussion_r1196764667
##########
go/adbc/adbc.go:
##########
@@ -536,3 +536,16 @@ type Statement interface {
// an error with a StatusNotImplemented code.
ExecutePartitions(context.Context) (*arrow.Schema, Partitions, int64, error)
}
+
+// Statement110 is an extension interface for methods added to Statement in
+// ADBC API revision 1.1.0.
+type Statement110 interface {
+ Statement
+
+ // ExecuteSchema returns the schema of the result set of a query without
+ // executing it.
+ //
+ // If the driver does not support this, this will return an error with a
+ // StatusNotImplemented code.
+ ExecuteSchema(context.Context) (*arrow.Schema, error)
+}
Review Comment:
well, I still want to avoid breaking changes unless we're bumping the version number. but if we bump the version number then you're correct that we don't need to worry about ABI compatibility and can just bump the version number of the package and add to the original interface
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #686: feat(format): add AdbcStatementExecuteSchema
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #686:
URL: https://github.com/apache/arrow-adbc/pull/686#discussion_r1196740281
##########
go/adbc/adbc.go:
##########
@@ -536,3 +536,16 @@ type Statement interface {
// an error with a StatusNotImplemented code.
ExecutePartitions(context.Context) (*arrow.Schema, Partitions, int64, error)
}
+
+// Statement110 is an extension interface for methods added to Statement in
+// ADBC API revision 1.1.0.
+type Statement110 interface {
+ Statement
+
+ // ExecuteSchema returns the schema of the result set of a query without
+ // executing it.
+ //
+ // If the driver does not support this, this will return an error with a
+ // StatusNotImplemented code.
+ ExecuteSchema(context.Context) (*arrow.Schema, error)
+}
Review Comment:
Well, we're going to have more methods - but I can remove the embedding and we can append more methods to the new interface while on the branch.
Or if we end up going for 2.0.0 we can just add them to the original interface. (Frankly since the compatibility concerns are different we could just go down that path anyways?)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #686: feat(format): add AdbcStatementExecuteSchema
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #686:
URL: https://github.com/apache/arrow-adbc/pull/686#discussion_r1196507165
##########
go/adbc/adbc.go:
##########
@@ -536,3 +536,16 @@ type Statement interface {
// an error with a StatusNotImplemented code.
ExecutePartitions(context.Context) (*arrow.Schema, Partitions, int64, error)
}
+
+// Statement110 is an extension interface for methods added to Statement in
+// ADBC API revision 1.1.0.
+type Statement110 interface {
+ Statement
+
+ // ExecuteSchema returns the schema of the result set of a query without
+ // executing it.
+ //
+ // If the driver does not support this, this will return an error with a
+ // StatusNotImplemented code.
+ ExecuteSchema(context.Context) (*arrow.Schema, error)
+}
Review Comment:
@zeroshade for this sort of thing, would it make more sense to actually add a new package so we could just name this `Statement` and re-export all the types?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #686: feat(format): add AdbcStatementExecuteSchema
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #686:
URL: https://github.com/apache/arrow-adbc/pull/686#discussion_r1196726165
##########
go/adbc/adbc.go:
##########
@@ -536,3 +536,16 @@ type Statement interface {
// an error with a StatusNotImplemented code.
ExecutePartitions(context.Context) (*arrow.Schema, Partitions, int64, error)
}
+
+// Statement110 is an extension interface for methods added to Statement in
+// ADBC API revision 1.1.0.
+type Statement110 interface {
+ Statement
+
+ // ExecuteSchema returns the schema of the result set of a query without
+ // executing it.
+ //
+ // If the driver does not support this, this will return an error with a
+ // StatusNotImplemented code.
+ ExecuteSchema(context.Context) (*arrow.Schema, error)
+}
Review Comment:
If we're looking to avoid the breaking change, there's a couple options:
1. We can introduce a single method interface that only exposes the `ExecuteSchema` method which a consumer can use a type inference test for (like we do for `PostInitOptions`)
2. Something like this (though I think I'd name it differently) that embeds the original interface with the new method.
3. adding a new package, if we go this route we'd make this a new major version and include the major version in the path (like we do for arrow itself) and just make the breaking change. Adding a new package that re-exports things would have to be using aliases if we don't want it to break anyone that is using the existing stuff (as non-aliases would be explicitly different types).
Of these options, I think I prefer the first option, adding a new, single-method interface like `StatementExecSchema` (for example, look at https://pkg.go.dev/database/sql/driver#ConnBeginTx)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm closed pull request #686: feat(format): add AdbcStatementExecuteSchema
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm closed pull request #686: feat(format): add AdbcStatementExecuteSchema
URL: https://github.com/apache/arrow-adbc/pull/686
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org