You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "fredrikhgrelland (via GitHub)" <gi...@apache.org> on 2023/09/27 18:51:09 UTC
[GitHub] [arrow-adbc] fredrikhgrelland opened a new pull request, #1118: fix(go/adbc): missing adbc.ingest.temporary
fredrikhgrelland opened a new pull request, #1118:
URL: https://github.com/apache/arrow-adbc/pull/1118
(no comment)
--
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 #1118: fix(go/adbc): missing adbc.ingest.temporary
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm closed pull request #1118: fix(go/adbc): missing adbc.ingest.temporary
URL: https://github.com/apache/arrow-adbc/pull/1118
--
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 #1118: fix(go/adbc): missing adbc.ingest.temporary
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1118:
URL: https://github.com/apache/arrow-adbc/pull/1118#discussion_r1339182193
##########
go/adbc/adbc.go:
##########
@@ -237,6 +237,7 @@ const (
OptionKeyProgress = "adbc.statement.exec.progress"
OptionKeyMaxProgress = "adbc.statement.exec.max_progress"
OptionKeyIngestTargetTable = "adbc.ingest.target_table"
+ OptionKeyIngestTemporary = "adbc.ingest.temporary"
Review Comment:
you can use godoc to put a comment in there
```go
// This option is experimental
OptionKeyIngestTemporary = "adbc.ingest.temporary"
```
If you want it to get bolded or otherwise you can use `// # This option is experimental` and it'll show up as a header in the docs.
--
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 #1118: fix(go/adbc): missing adbc.ingest.temporary
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #1118:
URL: https://github.com/apache/arrow-adbc/pull/1118#discussion_r1339067020
##########
go/adbc/driver/snowflake/statement.go:
##########
@@ -108,6 +108,11 @@ func (st *statement) SetOption(key string, val string) error {
case adbc.OptionKeyIngestTargetTable:
st.query = ""
st.targetTable = val
+ case adbc.OptionKeyIngestTemporary:
+ return adbc.Error{
+ Msg: fmt.Sprintf("[Snowflake] Unknown statement option '%s'", key),
+ Code: adbc.StatusNotImplemented,
+ }
Review Comment:
I think instead of this, the 'default' at the bottom needs to use the NotSupported code instead of InvalidArgument
--
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 #1118: fix(go/adbc): missing adbc.ingest.temporary
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #1118:
URL: https://github.com/apache/arrow-adbc/pull/1118#discussion_r1339065139
##########
go/adbc/adbc.go:
##########
@@ -237,6 +237,7 @@ const (
OptionKeyProgress = "adbc.statement.exec.progress"
OptionKeyMaxProgress = "adbc.statement.exec.max_progress"
OptionKeyIngestTargetTable = "adbc.ingest.target_table"
+ OptionKeyIngestTemporary = "adbc.ingest.temporary"
Review Comment:
We haven't technically added these options to the 'specification' interface
##########
go/adbc/driver/snowflake/statement.go:
##########
@@ -108,6 +108,11 @@ func (st *statement) SetOption(key string, val string) error {
case adbc.OptionKeyIngestTargetTable:
st.query = ""
st.targetTable = val
+ case adbc.OptionKeyIngestTemporary:
+ return adbc.Error{
+ Msg: fmt.Sprintf("[Snowflake] Unknown statement option '%s'", key),
+ Code: adbc.StatusNotImplemented,
+ }
Review Comment:
Is this really necessary? The default will do this already
##########
go/adbc/adbc.go:
##########
@@ -237,6 +237,7 @@ const (
OptionKeyProgress = "adbc.statement.exec.progress"
OptionKeyMaxProgress = "adbc.statement.exec.max_progress"
OptionKeyIngestTargetTable = "adbc.ingest.target_table"
+ OptionKeyIngestTemporary = "adbc.ingest.temporary"
Review Comment:
@zeroshade is there any way to mark things 'experimental'?
--
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 #1118: fix(go/adbc): missing adbc.ingest.temporary
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #1118:
URL: https://github.com/apache/arrow-adbc/pull/1118#discussion_r1339066426
##########
go/adbc/driver/snowflake/statement.go:
##########
@@ -108,6 +108,11 @@ func (st *statement) SetOption(key string, val string) error {
case adbc.OptionKeyIngestTargetTable:
st.query = ""
st.targetTable = val
+ case adbc.OptionKeyIngestTemporary:
+ return adbc.Error{
+ Msg: fmt.Sprintf("[Snowflake] Unknown statement option '%s'", key),
+ Code: adbc.StatusNotImplemented,
+ }
Review Comment:
Ah, I see
--
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] fredrikhgrelland commented on a diff in pull request #1118: fix(go/adbc): missing adbc.ingest.temporary
Posted by "fredrikhgrelland (via GitHub)" <gi...@apache.org>.
fredrikhgrelland commented on code in PR #1118:
URL: https://github.com/apache/arrow-adbc/pull/1118#discussion_r1339080964
##########
go/adbc/driver/snowflake/statement.go:
##########
@@ -108,6 +108,11 @@ func (st *statement) SetOption(key string, val string) error {
case adbc.OptionKeyIngestTargetTable:
st.query = ""
st.targetTable = val
+ case adbc.OptionKeyIngestTemporary:
+ return adbc.Error{
+ Msg: fmt.Sprintf("[Snowflake] Unknown statement option '%s'", key),
+ Code: adbc.StatusNotImplemented,
+ }
Review Comment:
The default https://github.com/apache/arrow-adbc/blob/main/go/adbc/driver/snowflake/statement.go#L145-L149 will throw `StatusInvalidArgument` while https://github.com/apache/arrow-adbc/blob/main/python/adbc_driver_manager/adbc_driver_manager/dbapi.py#L876-L879 checks for `StatusNotImplemented`
If you do not want to change the go implementation, the python manager could pass on ProgrammingError as well,
```
try:
self._stmt.set_options(**options)
except (NotSupportedError, ProgrammingError):
pass
```
--
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 pull request #1118: fix(go/adbc): missing adbc.ingest.temporary
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1118:
URL: https://github.com/apache/arrow-adbc/pull/1118#issuecomment-1739107135
I merged #1120. Thanks for digging into this!
--
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 #1118: fix(go/adbc): missing adbc.ingest.temporary
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #1118:
URL: https://github.com/apache/arrow-adbc/pull/1118#discussion_r1339095054
##########
go/adbc/driver/snowflake/statement.go:
##########
@@ -108,6 +108,11 @@ func (st *statement) SetOption(key string, val string) error {
case adbc.OptionKeyIngestTargetTable:
st.query = ""
st.targetTable = val
+ case adbc.OptionKeyIngestTemporary:
+ return adbc.Error{
+ Msg: fmt.Sprintf("[Snowflake] Unknown statement option '%s'", key),
+ Code: adbc.StatusNotImplemented,
+ }
Review Comment:
Right, I think the default used here is wrong. The C header specifies NOT_IMPLEMENTED for unrecognized options and Go usually follows this: https://github.com/apache/arrow-adbc/blob/0656c69da28e01f947ba3477fdcd338faf554a85/adbc.h#L1220
--
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] fredrikhgrelland commented on a diff in pull request #1118: fix(go/adbc): missing adbc.ingest.temporary
Posted by "fredrikhgrelland (via GitHub)" <gi...@apache.org>.
fredrikhgrelland commented on code in PR #1118:
URL: https://github.com/apache/arrow-adbc/pull/1118#discussion_r1339109997
##########
go/adbc/driver/snowflake/statement.go:
##########
@@ -108,6 +108,11 @@ func (st *statement) SetOption(key string, val string) error {
case adbc.OptionKeyIngestTargetTable:
st.query = ""
st.targetTable = val
+ case adbc.OptionKeyIngestTemporary:
+ return adbc.Error{
+ Msg: fmt.Sprintf("[Snowflake] Unknown statement option '%s'", key),
+ Code: adbc.StatusNotImplemented,
+ }
Review Comment:
alternative https://github.com/apache/arrow-adbc/pull/1120 still fixes https://github.com/apache/arrow-adbc/issues/1119 you may close this PR
--
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