Skip to content

Conversation

@lidavidm
Copy link
Member

Fixes #666.

@lidavidm
Copy link
Member Author

TODO: Use the new helper from #769

@lidavidm
Copy link
Member Author

Hmm, the new helper is a little too rigid for SQLite's purposes

@lidavidm lidavidm force-pushed the gh-666-primary-key branch from ed79dd5 to 5ba7dc7 Compare June 13, 2023 19:59
@lidavidm
Copy link
Member Author

Just needs a small bend to work!

@lidavidm lidavidm force-pushed the gh-666-primary-key branch 2 times, most recently from a07af36 to 60391c5 Compare June 13, 2023 20:14
@lidavidm
Copy link
Member Author

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...

@WillAyd
Copy link
Contributor

WillAyd commented Jun 13, 2023

I think the ASAN problem is with the destructor - I think can push up a fix soon

@WillAyd
Copy link
Contributor

WillAyd commented Jun 13, 2023

I think should be resolved by #785

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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair.

SQLite doesn't name its constraints so I wanted some way to just get any constraint.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah OK. In that case maybe you want to do something like this instead?

  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];
    ...
  }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternately could add a AdbcGetObjectsDataGetConstraintByPosition function

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nice. I'll update it tomorrow morning

@lidavidm lidavidm force-pushed the gh-666-primary-key branch from 60391c5 to 94cc45a Compare June 14, 2023 12:39
@lidavidm lidavidm merged commit 1717746 into apache:main Jun 14, 2023
@lidavidm lidavidm deleted the gh-666-primary-key branch June 14, 2023 13:14
@lidavidm lidavidm added this to the ADBC Libraries 0.5.0 milestone Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

c/driver/sqlite: GetObjects with PRIMARY KEY failed

2 participants