You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "wjones127 (via GitHub)" <gi...@apache.org> on 2023/02/05 23:28:36 UTC
[GitHub] [arrow-adbc] wjones127 opened a new pull request, #417: format(api!): clarify const-ness of some pointers
wjones127 opened a new pull request, #417:
URL: https://github.com/apache/arrow-adbc/pull/417
While working on the Rust bindings I noticed there were some pointers in the ADBC functions parameters that seems like they should have been `const`. The current proposed Rust API includes these changes in the FFI, so wondering if some or all of these changes are welcome, or if I need to do a bit of redesign in the Rust implementation.
--
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] wjones127 commented on a diff in pull request #417: format(api!): clarify const-ness of some pointers
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#discussion_r1099032262
##########
adbc.h:
##########
@@ -624,18 +624,18 @@ struct ADBC_EXPORT AdbcDriver {
AdbcStatusCode (*DatabaseRelease)(struct AdbcDatabase*, struct AdbcError*);
AdbcStatusCode (*ConnectionCommit)(struct AdbcConnection*, struct AdbcError*);
- AdbcStatusCode (*ConnectionGetInfo)(struct AdbcConnection*, uint32_t*, size_t,
+ AdbcStatusCode (*ConnectionGetInfo)(struct AdbcConnection*, const uint32_t*, size_t,
struct ArrowArrayStream*, struct AdbcError*);
AdbcStatusCode (*ConnectionGetObjects)(struct AdbcConnection*, int, const char*,
- const char*, const char*, const char**,
+ const char*, const char*, char const* const*,
const char*, struct ArrowArrayStream*,
struct AdbcError*);
AdbcStatusCode (*ConnectionGetTableSchema)(struct AdbcConnection*, const char*,
const char*, const char*,
struct ArrowSchema*, struct AdbcError*);
AdbcStatusCode (*ConnectionGetTableTypes)(struct AdbcConnection*,
struct ArrowArrayStream*, struct AdbcError*);
- AdbcStatusCode (*ConnectionInit)(struct AdbcConnection*, struct AdbcDatabase*,
+ AdbcStatusCode (*ConnectionInit)(struct AdbcConnection*, struct AdbcDatabase const*,
struct AdbcError*);
AdbcStatusCode (*ConnectionNew)(struct AdbcConnection*, struct AdbcError*);
Review Comment:
Okay I've updated the doc string on each of those. LMK what you think.
--
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] github-actions[bot] commented on pull request #417: format(api!): clarify const-ness of some pointers
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#issuecomment-1418296136
:warning: Please follow the [Conventional Commits format in CONTRIBUTING.md](https://github.com/apache/arrow-adbc/blob/main/CONTRIBUTING.md) for PR titles.
--
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 #417: format(api!): clarify const-ness of some pointers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#issuecomment-1419311393
These changes are potentially source-breaking, if not necessarily ABI-breaking.
--
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 #417: fix(format): clarify const-ness of some pointers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm closed pull request #417: fix(format): clarify const-ness of some pointers
URL: https://github.com/apache/arrow-adbc/pull/417
--
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 #417: format(api!): clarify const-ness of some pointers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#discussion_r1097571502
##########
adbc.h:
##########
@@ -624,18 +624,18 @@ struct ADBC_EXPORT AdbcDriver {
AdbcStatusCode (*DatabaseRelease)(struct AdbcDatabase*, struct AdbcError*);
AdbcStatusCode (*ConnectionCommit)(struct AdbcConnection*, struct AdbcError*);
- AdbcStatusCode (*ConnectionGetInfo)(struct AdbcConnection*, uint32_t*, size_t,
+ AdbcStatusCode (*ConnectionGetInfo)(struct AdbcConnection*, const uint32_t*, size_t,
struct ArrowArrayStream*, struct AdbcError*);
AdbcStatusCode (*ConnectionGetObjects)(struct AdbcConnection*, int, const char*,
- const char*, const char*, const char**,
+ const char*, const char*, char const* const*,
const char*, struct ArrowArrayStream*,
struct AdbcError*);
AdbcStatusCode (*ConnectionGetTableSchema)(struct AdbcConnection*, const char*,
const char*, const char*,
struct ArrowSchema*, struct AdbcError*);
AdbcStatusCode (*ConnectionGetTableTypes)(struct AdbcConnection*,
struct ArrowArrayStream*, struct AdbcError*);
- AdbcStatusCode (*ConnectionInit)(struct AdbcConnection*, struct AdbcDatabase*,
+ AdbcStatusCode (*ConnectionInit)(struct AdbcConnection*, struct AdbcDatabase const*,
struct AdbcError*);
AdbcStatusCode (*ConnectionNew)(struct AdbcConnection*, struct AdbcError*);
Review Comment:
What I mean is, the pattern for an API that _allocates for you_ is `Foo**`. With `Foo*`, the callee has no way of providing the caller with the updated pointer.
--
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 #417: format(api!): clarify const-ness of some pointers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#discussion_r1097333013
##########
adbc.h:
##########
@@ -624,18 +624,18 @@ struct ADBC_EXPORT AdbcDriver {
AdbcStatusCode (*DatabaseRelease)(struct AdbcDatabase*, struct AdbcError*);
AdbcStatusCode (*ConnectionCommit)(struct AdbcConnection*, struct AdbcError*);
- AdbcStatusCode (*ConnectionGetInfo)(struct AdbcConnection*, uint32_t*, size_t,
+ AdbcStatusCode (*ConnectionGetInfo)(struct AdbcConnection*, const uint32_t*, size_t,
struct ArrowArrayStream*, struct AdbcError*);
AdbcStatusCode (*ConnectionGetObjects)(struct AdbcConnection*, int, const char*,
- const char*, const char*, const char**,
+ const char*, const char*, char const* const*,
const char*, struct ArrowArrayStream*,
struct AdbcError*);
AdbcStatusCode (*ConnectionGetTableSchema)(struct AdbcConnection*, const char*,
const char*, const char*,
struct ArrowSchema*, struct AdbcError*);
AdbcStatusCode (*ConnectionGetTableTypes)(struct AdbcConnection*,
struct ArrowArrayStream*, struct AdbcError*);
- AdbcStatusCode (*ConnectionInit)(struct AdbcConnection*, struct AdbcDatabase*,
+ AdbcStatusCode (*ConnectionInit)(struct AdbcConnection*, struct AdbcDatabase const*,
struct AdbcError*);
AdbcStatusCode (*ConnectionNew)(struct AdbcConnection*, struct AdbcError*);
Review Comment:
uninitialized in terms of API level, not in terms of memory.
--
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] paleolimbot commented on a diff in pull request #417: format(api!): clarify const-ness of some pointers
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#discussion_r1096868839
##########
adbc.h:
##########
@@ -729,7 +729,8 @@ AdbcStatusCode AdbcConnectionSetOption(struct AdbcConnection* connection, const
/// as well.
ADBC_EXPORT
AdbcStatusCode AdbcConnectionInit(struct AdbcConnection* connection,
- struct AdbcDatabase* database, struct AdbcError* error);
+ const struct AdbcDatabase* database,
Review Comment:
It seems like it should be able to modify the database if it needs to? (For example, the database private member might contain a counter to count open connections, and this could increment it by 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] wjones127 commented on a diff in pull request #417: format(api!): clarify const-ness of some pointers
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#discussion_r1097560216
##########
adbc.h:
##########
@@ -624,18 +624,18 @@ struct ADBC_EXPORT AdbcDriver {
AdbcStatusCode (*DatabaseRelease)(struct AdbcDatabase*, struct AdbcError*);
AdbcStatusCode (*ConnectionCommit)(struct AdbcConnection*, struct AdbcError*);
- AdbcStatusCode (*ConnectionGetInfo)(struct AdbcConnection*, uint32_t*, size_t,
+ AdbcStatusCode (*ConnectionGetInfo)(struct AdbcConnection*, const uint32_t*, size_t,
struct ArrowArrayStream*, struct AdbcError*);
AdbcStatusCode (*ConnectionGetObjects)(struct AdbcConnection*, int, const char*,
- const char*, const char*, const char**,
+ const char*, const char*, char const* const*,
const char*, struct ArrowArrayStream*,
struct AdbcError*);
AdbcStatusCode (*ConnectionGetTableSchema)(struct AdbcConnection*, const char*,
const char*, const char*,
struct ArrowSchema*, struct AdbcError*);
AdbcStatusCode (*ConnectionGetTableTypes)(struct AdbcConnection*,
struct ArrowArrayStream*, struct AdbcError*);
- AdbcStatusCode (*ConnectionInit)(struct AdbcConnection*, struct AdbcDatabase*,
+ AdbcStatusCode (*ConnectionInit)(struct AdbcConnection*, struct AdbcDatabase const*,
struct AdbcError*);
AdbcStatusCode (*ConnectionNew)(struct AdbcConnection*, struct AdbcError*);
Review Comment:
Oh and the "allocate" is probably referring to the private_data allocation.
--
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] wjones127 commented on a diff in pull request #417: format(api!): clarify const-ness of some pointers
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#discussion_r1096829035
##########
adbc.h:
##########
@@ -729,7 +729,8 @@ AdbcStatusCode AdbcConnectionSetOption(struct AdbcConnection* connection, const
/// as well.
ADBC_EXPORT
AdbcStatusCode AdbcConnectionInit(struct AdbcConnection* connection,
- struct AdbcDatabase* database, struct AdbcError* error);
+ const struct AdbcDatabase* database,
Review Comment:
I am the least confident about this one, admittedly.
--
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] wjones127 commented on a diff in pull request #417: format(api!): clarify const-ness of some pointers
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#discussion_r1099033660
##########
adbc.h:
##########
@@ -796,7 +797,7 @@ AdbcStatusCode AdbcConnectionRelease(struct AdbcConnection* connection,
/// \param[out] error Error details, if an error occurs.
ADBC_EXPORT
AdbcStatusCode AdbcConnectionGetInfo(struct AdbcConnection* connection,
- uint32_t* info_codes, size_t info_codes_length,
+ const uint32_t* info_codes, size_t info_codes_length,
Review Comment:
This is the only one left now. Maybe I should separate this out from the docs change, and we can consider this for the future?
--
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] wjones127 commented on a diff in pull request #417: format(api!): clarify const-ness of some pointers
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#discussion_r1096962941
##########
adbc.h:
##########
@@ -729,7 +729,8 @@ AdbcStatusCode AdbcConnectionSetOption(struct AdbcConnection* connection, const
/// as well.
ADBC_EXPORT
AdbcStatusCode AdbcConnectionInit(struct AdbcConnection* connection,
- struct AdbcDatabase* database, struct AdbcError* error);
+ const struct AdbcDatabase* database,
Review Comment:
Okay I think I can remove this 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: 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 #417: format(api!): clarify const-ness of some pointers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#discussion_r1097567090
##########
adbc.h:
##########
@@ -624,18 +624,18 @@ struct ADBC_EXPORT AdbcDriver {
AdbcStatusCode (*DatabaseRelease)(struct AdbcDatabase*, struct AdbcError*);
AdbcStatusCode (*ConnectionCommit)(struct AdbcConnection*, struct AdbcError*);
- AdbcStatusCode (*ConnectionGetInfo)(struct AdbcConnection*, uint32_t*, size_t,
+ AdbcStatusCode (*ConnectionGetInfo)(struct AdbcConnection*, const uint32_t*, size_t,
struct ArrowArrayStream*, struct AdbcError*);
AdbcStatusCode (*ConnectionGetObjects)(struct AdbcConnection*, int, const char*,
- const char*, const char*, const char**,
+ const char*, const char*, char const* const*,
const char*, struct ArrowArrayStream*,
struct AdbcError*);
AdbcStatusCode (*ConnectionGetTableSchema)(struct AdbcConnection*, const char*,
const char*, const char*,
struct ArrowSchema*, struct AdbcError*);
AdbcStatusCode (*ConnectionGetTableTypes)(struct AdbcConnection*,
struct ArrowArrayStream*, struct AdbcError*);
- AdbcStatusCode (*ConnectionInit)(struct AdbcConnection*, struct AdbcDatabase*,
+ AdbcStatusCode (*ConnectionInit)(struct AdbcConnection*, struct AdbcDatabase const*,
struct AdbcError*);
AdbcStatusCode (*ConnectionNew)(struct AdbcConnection*, struct AdbcError*);
Review Comment:
Though: you know it can't be a null pointer because then it should be Foo** not Foo*
--
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 #417: fix(format): clarify const-ness of some pointers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#issuecomment-1421309907
Cool. I think we'll merge changes like this to a branch for API 1.1.0 (rebasing on main every so often?) then fast-forward merge into main once we're happy with the new API changes?
--
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] wjones127 commented on a diff in pull request #417: format(api!): clarify const-ness of some pointers
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#discussion_r1099002368
##########
adbc.h:
##########
@@ -903,12 +904,10 @@ AdbcStatusCode AdbcConnectionGetInfo(struct AdbcConnection* connection,
/// \param[out] out The result set.
/// \param[out] error Error details, if an error occurs.
ADBC_EXPORT
-AdbcStatusCode AdbcConnectionGetObjects(struct AdbcConnection* connection, int depth,
- const char* catalog, const char* db_schema,
- const char* table_name, const char** table_type,
- const char* column_name,
- struct ArrowArrayStream* out,
- struct AdbcError* error);
+AdbcStatusCode AdbcConnectionGetObjects(
+ struct AdbcConnection* connection, int depth, const char* catalog,
+ const char* db_schema, const char* table_name, char const* const* table_type,
Review Comment:
This might be one where I leave as is in the header but keep this change in the Rust FFI definitions.
--
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 #417: format(api!): clarify const-ness of some pointers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#discussion_r1097580658
##########
adbc.h:
##########
@@ -796,7 +797,7 @@ AdbcStatusCode AdbcConnectionRelease(struct AdbcConnection* connection,
/// \param[out] error Error details, if an error occurs.
ADBC_EXPORT
AdbcStatusCode AdbcConnectionGetInfo(struct AdbcConnection* connection,
- uint32_t* info_codes, size_t info_codes_length,
+ const uint32_t* info_codes, size_t info_codes_length,
Review Comment:
I think we could probably accept this, even if it's technically a break
##########
adbc.h:
##########
@@ -903,12 +904,10 @@ AdbcStatusCode AdbcConnectionGetInfo(struct AdbcConnection* connection,
/// \param[out] out The result set.
/// \param[out] error Error details, if an error occurs.
ADBC_EXPORT
-AdbcStatusCode AdbcConnectionGetObjects(struct AdbcConnection* connection, int depth,
- const char* catalog, const char* db_schema,
- const char* table_name, const char** table_type,
- const char* column_name,
- struct ArrowArrayStream* out,
- struct AdbcError* error);
+AdbcStatusCode AdbcConnectionGetObjects(
+ struct AdbcConnection* connection, int depth, const char* catalog,
+ const char* db_schema, const char* table_name, char const* const* table_type,
Review Comment:
To be frank I'm not sure I often see the constness declared at this fine-grained of a level
##########
adbc.h:
##########
@@ -1139,7 +1139,7 @@ AdbcStatusCode AdbcStatementBindStream(struct AdbcStatement* statement,
///
/// \return ADBC_STATUS_NOT_IMPLEMENTED if the schema cannot be determined.
ADBC_EXPORT
-AdbcStatusCode AdbcStatementGetParameterSchema(struct AdbcStatement* statement,
+AdbcStatusCode AdbcStatementGetParameterSchema(const struct AdbcStatement* statement,
Review Comment:
I think it's simpler to let all 'methods' mutate their objects even if it seems like there's no reason to
--
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 #417: format(api!): clarify const-ness of some pointers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#discussion_r1097566487
##########
adbc.h:
##########
@@ -624,18 +624,18 @@ struct ADBC_EXPORT AdbcDriver {
AdbcStatusCode (*DatabaseRelease)(struct AdbcDatabase*, struct AdbcError*);
AdbcStatusCode (*ConnectionCommit)(struct AdbcConnection*, struct AdbcError*);
- AdbcStatusCode (*ConnectionGetInfo)(struct AdbcConnection*, uint32_t*, size_t,
+ AdbcStatusCode (*ConnectionGetInfo)(struct AdbcConnection*, const uint32_t*, size_t,
struct ArrowArrayStream*, struct AdbcError*);
AdbcStatusCode (*ConnectionGetObjects)(struct AdbcConnection*, int, const char*,
- const char*, const char*, const char**,
+ const char*, const char*, char const* const*,
const char*, struct ArrowArrayStream*,
struct AdbcError*);
AdbcStatusCode (*ConnectionGetTableSchema)(struct AdbcConnection*, const char*,
const char*, const char*,
struct ArrowSchema*, struct AdbcError*);
AdbcStatusCode (*ConnectionGetTableTypes)(struct AdbcConnection*,
struct ArrowArrayStream*, struct AdbcError*);
- AdbcStatusCode (*ConnectionInit)(struct AdbcConnection*, struct AdbcDatabase*,
+ AdbcStatusCode (*ConnectionInit)(struct AdbcConnection*, struct AdbcDatabase const*,
struct AdbcError*);
AdbcStatusCode (*ConnectionNew)(struct AdbcConnection*, struct AdbcError*);
Review Comment:
Yeah, allocate is misleading here
--
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 #417: format(api!): clarify const-ness of some pointers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#discussion_r1099043514
##########
adbc.h:
##########
@@ -796,7 +797,7 @@ AdbcStatusCode AdbcConnectionRelease(struct AdbcConnection* connection,
/// \param[out] error Error details, if an error occurs.
ADBC_EXPORT
AdbcStatusCode AdbcConnectionGetInfo(struct AdbcConnection* connection,
- uint32_t* info_codes, size_t info_codes_length,
+ const uint32_t* info_codes, size_t info_codes_length,
Review Comment:
That sounds good - this can be `docs(format): ...` and the other can be `fix(format): ...`
--
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 #417: fix(format): clarify const-ness of some pointers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#issuecomment-1675112464
Cherry-picked into #971. Thanks Will!
--
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] wjones127 commented on a diff in pull request #417: format(api!): clarify const-ness of some pointers
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#discussion_r1097569689
##########
adbc.h:
##########
@@ -624,18 +624,18 @@ struct ADBC_EXPORT AdbcDriver {
AdbcStatusCode (*DatabaseRelease)(struct AdbcDatabase*, struct AdbcError*);
AdbcStatusCode (*ConnectionCommit)(struct AdbcConnection*, struct AdbcError*);
- AdbcStatusCode (*ConnectionGetInfo)(struct AdbcConnection*, uint32_t*, size_t,
+ AdbcStatusCode (*ConnectionGetInfo)(struct AdbcConnection*, const uint32_t*, size_t,
struct ArrowArrayStream*, struct AdbcError*);
AdbcStatusCode (*ConnectionGetObjects)(struct AdbcConnection*, int, const char*,
- const char*, const char*, const char**,
+ const char*, const char*, char const* const*,
const char*, struct ArrowArrayStream*,
struct AdbcError*);
AdbcStatusCode (*ConnectionGetTableSchema)(struct AdbcConnection*, const char*,
const char*, const char*,
struct ArrowSchema*, struct AdbcError*);
AdbcStatusCode (*ConnectionGetTableTypes)(struct AdbcConnection*,
struct ArrowArrayStream*, struct AdbcError*);
- AdbcStatusCode (*ConnectionInit)(struct AdbcConnection*, struct AdbcDatabase*,
+ AdbcStatusCode (*ConnectionInit)(struct AdbcConnection*, struct AdbcDatabase const*,
struct AdbcError*);
AdbcStatusCode (*ConnectionNew)(struct AdbcConnection*, struct AdbcError*);
Review Comment:
Wait I thought any pointer could be null in C?
--
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 #417: format(api!): clarify const-ness of some pointers
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#discussion_r1097335088
##########
adbc.h:
##########
@@ -646,16 +646,16 @@ struct ADBC_EXPORT AdbcDriver {
AdbcStatusCode (*ConnectionRelease)(struct AdbcConnection*, struct AdbcError*);
AdbcStatusCode (*ConnectionRollback)(struct AdbcConnection*, struct AdbcError*);
- AdbcStatusCode (*StatementBind)(struct AdbcStatement*, struct ArrowArray*,
- struct ArrowSchema*, struct AdbcError*);
+ AdbcStatusCode (*StatementBind)(struct AdbcStatement*, struct ArrowArray const*,
+ struct ArrowSchema const*, struct AdbcError*);
Review Comment:
Binding should probably clear these structs since the statement is expected to take ownership
--
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] wjones127 commented on pull request #417: format(api!): clarify const-ness of some pointers
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#issuecomment-1419307253
> I'm not sure if we want to do this since this is a hard backwards compatibility break.
If this turns out to be ABI compatibility breaking, then I agree.
I'm hoping that the changes we settle on are clarifications on what was already intended, not meaningful changes in the API.
--
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] wjones127 commented on a diff in pull request #417: format(api!): clarify const-ness of some pointers
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #417:
URL: https://github.com/apache/arrow-adbc/pull/417#discussion_r1096830064
##########
adbc.h:
##########
@@ -624,18 +624,18 @@ struct ADBC_EXPORT AdbcDriver {
AdbcStatusCode (*DatabaseRelease)(struct AdbcDatabase*, struct AdbcError*);
AdbcStatusCode (*ConnectionCommit)(struct AdbcConnection*, struct AdbcError*);
- AdbcStatusCode (*ConnectionGetInfo)(struct AdbcConnection*, uint32_t*, size_t,
+ AdbcStatusCode (*ConnectionGetInfo)(struct AdbcConnection*, const uint32_t*, size_t,
struct ArrowArrayStream*, struct AdbcError*);
AdbcStatusCode (*ConnectionGetObjects)(struct AdbcConnection*, int, const char*,
- const char*, const char*, const char**,
+ const char*, const char*, char const* const*,
const char*, struct ArrowArrayStream*,
struct AdbcError*);
AdbcStatusCode (*ConnectionGetTableSchema)(struct AdbcConnection*, const char*,
const char*, const char*,
struct ArrowSchema*, struct AdbcError*);
AdbcStatusCode (*ConnectionGetTableTypes)(struct AdbcConnection*,
struct ArrowArrayStream*, struct AdbcError*);
- AdbcStatusCode (*ConnectionInit)(struct AdbcConnection*, struct AdbcDatabase*,
+ AdbcStatusCode (*ConnectionInit)(struct AdbcConnection*, struct AdbcDatabase const*,
struct AdbcError*);
AdbcStatusCode (*ConnectionNew)(struct AdbcConnection*, struct AdbcError*);
Review Comment:
Also, this is probably just a "Will not understanding C FFI" problem, but it was unclear to me at first that the `ConnectionNew`, `DatabaseNew` and `StatementNew` expected that the pointers passed are initialized and zero-ed memory, and not an uninitialized memory allocation or a null pointer. That might just be mixing up the notions of what "initialized" means--the memory itself or the connection/database/statement data. But
the docstring `Allocate a new (but uninitialized) database` almost reads as if the method itself was doing the memory allocation and initialization. Does that seem worth clarifying?
--
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