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/06/13 15:21:47 UTC
[GitHub] [arrow-adbc] lidavidm opened a new pull request, #777: fix(c/driver/sqlite): support PRIMARY KEY constraint in GetObjects
lidavidm opened a new pull request, #777:
URL: https://github.com/apache/arrow-adbc/pull/777
Fixes #666.
--
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 #777: fix(c/driver/sqlite): support PRIMARY KEY constraint in GetObjects
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #777:
URL: https://github.com/apache/arrow-adbc/pull/777#issuecomment-1589971732
Hmm, I just realized, why is this all in utils.c in the first place? Seems like it should go in adbc_validation_util.cc...
--
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] WillAyd commented on a diff in pull request #777: fix(c/driver/sqlite): support PRIMARY KEY constraint in GetObjects
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #777:
URL: https://github.com/apache/arrow-adbc/pull/777#discussion_r1228729970
##########
c/driver/common/utils.c:
##########
@@ -884,7 +884,7 @@ struct AdbcGetObjectsConstraint* AdbcGetObjectsDataGetConstraintByName(
for (int64_t i = 0; i < table->n_table_constraints; i++) {
struct AdbcGetObjectsConstraint* constraint = table->table_constraints[i];
struct ArrowStringView name = constraint->constraint_name;
- if (!strncmp(name.data, constraint_name, name.size_bytes)) {
+ if (!constraint_name || !strncmp(name.data, constraint_name, name.size_bytes)) {
Review Comment:
Ah OK. In that case maybe you want to do something like this instead?
```c
struct AdbcGetObjectsTable* table =
AdbcGetObjectsDataGetTableByName(*get_objects_data, quirks()->catalog().c_str(),
quirks()->db_schema().c_str(), "adbc_pkey_test");
if (table->n_table_constraints > 0) {
struct AdbcGetObjectsConstraint *constraint = table->table_constraints[0];
...
}
```
--
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 #777: fix(c/driver/sqlite): support PRIMARY KEY constraint in GetObjects
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #777:
URL: https://github.com/apache/arrow-adbc/pull/777#discussion_r1228726823
##########
c/driver/common/utils.c:
##########
@@ -884,7 +884,7 @@ struct AdbcGetObjectsConstraint* AdbcGetObjectsDataGetConstraintByName(
for (int64_t i = 0; i < table->n_table_constraints; i++) {
struct AdbcGetObjectsConstraint* constraint = table->table_constraints[i];
struct ArrowStringView name = constraint->constraint_name;
- if (!strncmp(name.data, constraint_name, name.size_bytes)) {
+ if (!constraint_name || !strncmp(name.data, constraint_name, name.size_bytes)) {
Review Comment:
Fair.
SQLite doesn't name its constraints so I wanted some way to just get any constraint.
--
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 #777: fix(c/driver/sqlite): support PRIMARY KEY constraint in GetObjects
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #777:
URL: https://github.com/apache/arrow-adbc/pull/777#discussion_r1228748513
##########
c/driver/common/utils.c:
##########
@@ -884,7 +884,7 @@ struct AdbcGetObjectsConstraint* AdbcGetObjectsDataGetConstraintByName(
for (int64_t i = 0; i < table->n_table_constraints; i++) {
struct AdbcGetObjectsConstraint* constraint = table->table_constraints[i];
struct ArrowStringView name = constraint->constraint_name;
- if (!strncmp(name.data, constraint_name, name.size_bytes)) {
+ if (!constraint_name || !strncmp(name.data, constraint_name, name.size_bytes)) {
Review Comment:
Ah, nice. I'll update it tomorrow morning
--
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] WillAyd commented on pull request #777: fix(c/driver/sqlite): support PRIMARY KEY constraint in GetObjects
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #777:
URL: https://github.com/apache/arrow-adbc/pull/777#issuecomment-1590045467
I think should be resolved by https://github.com/apache/arrow-adbc/pull/785
--
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] WillAyd commented on a diff in pull request #777: fix(c/driver/sqlite): support PRIMARY KEY constraint in GetObjects
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #777:
URL: https://github.com/apache/arrow-adbc/pull/777#discussion_r1228718834
##########
c/driver/common/utils.c:
##########
@@ -884,7 +884,7 @@ struct AdbcGetObjectsConstraint* AdbcGetObjectsDataGetConstraintByName(
for (int64_t i = 0; i < table->n_table_constraints; i++) {
struct AdbcGetObjectsConstraint* constraint = table->table_constraints[i];
struct ArrowStringView name = constraint->constraint_name;
- if (!strncmp(name.data, constraint_name, name.size_bytes)) {
+ if (!constraint_name || !strncmp(name.data, constraint_name, name.size_bytes)) {
Review Comment:
Current implementation does allow for UB if `constraint_name` is NULL so makes sense to return early if that happens, though I think that should be on function entry and also just return NULL there instead of grabbing the first constraint?
--
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 #777: fix(c/driver/sqlite): support PRIMARY KEY constraint in GetObjects
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #777:
URL: https://github.com/apache/arrow-adbc/pull/777#issuecomment-1589534508
TODO: Use the new helper from #769
--
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 #777: fix(c/driver/sqlite): support PRIMARY KEY constraint in GetObjects
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #777:
URL: https://github.com/apache/arrow-adbc/pull/777#issuecomment-1589903766
Hmm, the new helper is a little too rigid for SQLite's purposes
--
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 merged pull request #777: fix(c/driver/sqlite): support PRIMARY KEY constraint in GetObjects
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #777:
URL: https://github.com/apache/arrow-adbc/pull/777
--
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] WillAyd commented on pull request #777: fix(c/driver/sqlite): support PRIMARY KEY constraint in GetObjects
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #777:
URL: https://github.com/apache/arrow-adbc/pull/777#issuecomment-1590038023
I think the ASAN problem is with the destructor - I think can push up a fix soon
--
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] WillAyd commented on a diff in pull request #777: fix(c/driver/sqlite): support PRIMARY KEY constraint in GetObjects
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #777:
URL: https://github.com/apache/arrow-adbc/pull/777#discussion_r1228730405
##########
c/driver/common/utils.c:
##########
@@ -884,7 +884,7 @@ struct AdbcGetObjectsConstraint* AdbcGetObjectsDataGetConstraintByName(
for (int64_t i = 0; i < table->n_table_constraints; i++) {
struct AdbcGetObjectsConstraint* constraint = table->table_constraints[i];
struct ArrowStringView name = constraint->constraint_name;
- if (!strncmp(name.data, constraint_name, name.size_bytes)) {
+ if (!constraint_name || !strncmp(name.data, constraint_name, name.size_bytes)) {
Review Comment:
Alternately could add a `AdbcGetObjectsDataGetConstraintByPosition` function
--
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 #777: fix(c/driver/sqlite): support PRIMARY KEY constraint in GetObjects
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #777:
URL: https://github.com/apache/arrow-adbc/pull/777#issuecomment-1589939660
Just needs a small bend to work!
--
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] WillAyd commented on a diff in pull request #777: fix(c/driver/sqlite): support PRIMARY KEY constraint in GetObjects
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #777:
URL: https://github.com/apache/arrow-adbc/pull/777#discussion_r1228715340
##########
c/driver/common/utils.c:
##########
@@ -884,7 +884,7 @@ struct AdbcGetObjectsConstraint* AdbcGetObjectsDataGetConstraintByName(
for (int64_t i = 0; i < table->n_table_constraints; i++) {
struct AdbcGetObjectsConstraint* constraint = table->table_constraints[i];
struct ArrowStringView name = constraint->constraint_name;
- if (!strncmp(name.data, constraint_name, name.size_bytes)) {
+ if (!constraint_name || !strncmp(name.data, constraint_name, name.size_bytes)) {
Review Comment:
Hmm is this function receiving a nullptr? Seems a bit strange to call a `GetByName` function, pass it a NULL pointer for the name and expect a result back
--
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