Skip to content

Update sqlite3 wrapper for implement rust client #1849

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Mytherin merged 5 commits into from
Jun 13, 2021
Merged

Update sqlite3 wrapper for implement rust client #1849

Mytherin merged 5 commits into from
Jun 13, 2021

Conversation

@wangfenjin
Copy link
Contributor

@wangfenjin wangfenjin commented Jun 9, 2021

Add missing method, otherwise there might be link error when using sqlite3 wrapper:

= note: Undefined symbols for architecture x86_64:
          "_sqlite3_bind_zeroblob", referenced from:
              rusqlite::statement::Statement::bind_parameter::h0661d952bc6ba50b in rusqlite-f488f3eb8f4f694a.rusqlite.13sorw3o-cgu.11.rcgu.o
              ...
        ld: symbol(s) not found for architecture x86_64
        clang: error: linker command failed with exit code 1 (use -v to see invocation)

related #949

@wangfenjin
add impl method to fix sqlite3 link error
7e1dd71 
Change-Id: I25cf2b4a89001972ed56c756ba9d244e7e712780
@wangfenjin wangfenjin changed the title add impl method to fix sqlite3 link error WIP: add impl method to fix sqlite3 link error Jun 10, 2021
@wangfenjin
Copy link
Contributor Author

wangfenjin commented Jun 10, 2021

We can hold on this PR for now, I'm working on other changes

@Mytherin
Copy link
Collaborator

Mytherin commented Jun 10, 2021

Thanks for the PR! Could you run make format-fix to fix the formatting of the PR? Otherwise I think the PR looks fine.

@wangfenjin
fix some errors and format code
2afcbcf 
Change-Id: Iaccde5758ed3c0aabe5fdfd18836124d71eabe3b
@wangfenjin wangfenjin changed the title WIP: add impl method to fix sqlite3 link error Update sqlite3 wrapper for implement rust client Jun 11, 2021
@wangfenjin
Copy link
Contributor Author

wangfenjin commented Jun 11, 2021

Hi @Mytherin , please help review the code.

Please be noted that except for add some unsupported functions, I also update some of the logics.

@wangfenjin wangfenjin mentioned this pull request Jun 11, 2021
Closed
Mytherin
Mytherin reviewed Jun 11, 2021
View reviewed changes
Copy link
Collaborator

@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! Looks good. Some comments:

@wangfenjin
add tests
6353be8 
Change-Id: I2950f1d06a0ccff2ada659045333e97c1b7b2edd
@wangfenjin
Copy link
Contributor Author

wangfenjin commented Jun 11, 2021

Hi @Mytherin Please help review the code

Mytherin
Mytherin reviewed Jun 11, 2021
View reviewed changes
Copy link
Collaborator

@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! Some more comments:

tools/sqlite3_api_wrapper/sqlite3_api_wrapper.cpp Outdated
if (length < 0) {
length = 0;
}
return sqlite3_internal_bind_value(stmt, idx, Value::BLOB(string(length, '0')));
Copy link
Collaborator

@Mytherin Mytherin Jun 11, 2021

Choose a reason for hiding this comment

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

This generates the ASCII character 0, not the byte 0 (\0). I imagine zeroblob should generate null bytes, not the character 0, no? Perhaps you could verify this behavior in SQLite itself?

Copy link
Contributor Author

@wangfenjin wangfenjin Jun 11, 2021

Choose a reason for hiding this comment

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

From SQLite doc:

The zeroblob(N) function returns a BLOB consisting of N bytes of 0x00. SQLite manages these zeroblobs very efficiently. Zeroblobs can be used to reserve space for a BLOB that is later written using incremental BLOB I/O. This SQL function is implemented using the sqlite3_result_zeroblob() routine from the C/C++ interface.

Currently our BLOB value always convert to string, so I mark the api as unsupported for now.

@wangfenjin
add more tests; delete zeroblob
b2806ea 
Change-Id: Ib0ee080d30c0e5654d3e33c6ffc7107a2a25e86e
@wangfenjin
fix error test
2a68aa8 
Change-Id: I3c534c4f8056347756b8f1aeec0ad8849d9faa25
@Mytherin Mytherin merged commit 8bc050d into duckdb:master Jun 13, 2021
@Mytherin
Copy link
Collaborator

Mytherin commented Jun 13, 2021

Thanks! Looks good now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mytherin Mytherin Mytherin left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wangfenjin @Mytherin