Skip to content
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.

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

sqlite3def: Add support for virtual table #310

Merged
merged 6 commits into from
Nov 18, 2022

Conversation

knaka
Copy link
Contributor

@knaka knaka commented Nov 18, 2022

I have added the support for virtual table but it is only to parse DDLs because the feature is not supported by modernc.org/sqlite package. This enables to export and add virtual tables and shadow ones to skip_tables list.

@knaka knaka marked this pull request as ready for review November 18, 2022 00:44
@knaka knaka marked this pull request as draft November 18, 2022 00:50
@knaka knaka marked this pull request as ready for review November 18, 2022 00:55
@k0kubun
Copy link
Collaborator

k0kubun commented Nov 18, 2022

👍

the feature is not supported by modernc.org/sqlite package

Is it the case on the latest version, v1.19.4 as well? What sqldef uses is old, so upgrading it might help.

@knaka
Copy link
Contributor Author

knaka commented Nov 18, 2022

As far as I see in the article, it has not been implemented. // Virtual table support/example (#26) · Issues · cznic / sqlite · GitLab

But I am trying modernc.org/sqlite v1.19.4 ...

Wow it is working. While fts is not supported in v1.19.4,

$ sqlite3def --config=config.yml --file=Schemafile db/test.sqlite3
-- Apply --
CREATE VIRTUAL TABLE fts_a USING fts4(
                                         body TEXT,
                                         tokenize=unicode61 "tokenchars=.=" "separators=X"
);
2022/11/18 10:08:40 SQL logic error: no such module: fts4 (1)

rtree is available.

$ sqlite3def --config=config.yml --file=Schemafile db/test.sqlite3
-- Apply --
CREATE VIRTUAL TABLE rtree_a USING rtree(
                                            id,            -- Integer primary key
                                            minX, maxX,    -- Minimum and maximum X coordinate
                                            minY, maxY,    -- Minimum and maximum Y coordinate
                                            +objname TEXT, -- name of the object
                                            +objtype TEXT, -- object type
                                            +boundary BLOB -- detailed boundary of object
);
CREATE TABLE items (
  name,
  misc,
  bar
);
$ sqlite3def --config=config.yml --file=Schemafile db/test.sqlite3
-- Apply --
DROP TABLE `rtree_a_rowid`;
DROP TABLE `rtree_a_node`;
DROP TABLE `rtree_a_parent`;

Virtual table feature itself seems to work while shadow tables must be added to skip_tables list.

Therefore, I would like to delete the following block.

if desired.table.virtual {
	return ddls, fmt.Errorf("virtual table feature is not supported by modernc.org/sqlite package: %#v", desired.table.name)
}

@knaka
Copy link
Contributor Author

knaka commented Nov 18, 2022

Nevertheless, I see no need to parse the details of virtual table DDLs, since alternation is not supported. // The Virtual Table Mechanism Of SQLite

One cannot run ALTER TABLE ... ADD COLUMN commands against a virtual table.

@k0kubun
Copy link
Collaborator

k0kubun commented Nov 18, 2022

Therefore, I would like to delete the following block.

Sure. Would you like to address that in this PR, or merge this first and then work on upgrading it?

@knaka
Copy link
Contributor Author

knaka commented Nov 18, 2022

I will push additional commits to this PR.

@knaka
Copy link
Contributor Author

knaka commented Nov 18, 2022

Oops...

@k0kubun
Copy link
Collaborator

k0kubun commented Nov 18, 2022

Is this i386-windows only? If so, I guess we could drop i386-windows support and just leave x86_64-windows for sqlite3def.

@knaka
Copy link
Contributor Author

knaka commented Nov 18, 2022

v1.19.1 dropped i386 Windows support https://gitlab.com/cznic/sqlite/-/tree/v1.19.1/lib https://gitlab.com/cznic/sqlite/-/commit/1b683acc8b500465bf058b2e8bf1e29096586563

I will drop “i386-windows” from test matrix.

@knaka knaka changed the title sqlite3def: Add support for virtual table (only to parse) sqlite3def: Add support for virtual table Nov 18, 2022
Copy link
Collaborator

@k0kubun k0kubun left a comment

Choose a reason for hiding this comment

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

Perfect, thank you so much!

@k0kubun k0kubun merged commit 99c6c2c into sqldef:master Nov 18, 2022
@k0kubun
Copy link
Collaborator

k0kubun commented Nov 18, 2022

So, I primarily maintain mysqldef and psqldef, ytakaya maintains mssqldef, and nobody really maintains sqlite3def. Given that your contributions have been so nice and it delivers a better result when somebody who really uses it maintains it, I'd like to make you the maintainer of sqlite3def if you wish. Would you be interested in maintaining sqlite3def, fixing and releasing it as you like?

@knaka
Copy link
Contributor Author

knaka commented Nov 18, 2022

The series of fixes so far has made sqlite3def practical for my use, thank you.

Should a maintainer supplement missing features and documentation, and to review PRs if needed? If so, I would like to be a maintainer.

I am not a native English speaker so I will make incorrect wording in code and document. If you feel some sentence incorrect I would ask you to correct it without hesitation.

@k0kubun
Copy link
Collaborator

k0kubun commented Nov 18, 2022

Should a maintainer supplement missing features and documentation, and to review PRs if needed? If so, I would like to be a maintainer.

Yes. Thank you! I invited you to be a collaborator in this repository, which should also allow you to release sqldef as well. I see that you already accepted it 🙂

I am not a native English speaker so I will make incorrect wording in code and document. If you feel some sentence incorrect I would ask you to correct it without hesitation.

No worries, I've had no problems communicating with you in English. Whether it's about English or not, I'll help conversations as needed.


My expectations for the maintainer of sqlite3def are:

  • You're expected to push commits that are mostly about SQLite3 to master without asking for my reviews.
    • No pull request is needed for such changes.
    • If you want to leave any description, please write that down in commit messages.
  • You're also expected to cut a release that is mostly about SQLite3.
    • If you merge a PR that impacts any behavior, you're expected to cut a release at least once on the same day.
    • It's as easy as updating CHANGELOG.md and running git tag v0.x.y && git push origin --tags && git push origin master at the latest master.
    • About versioning, please bump a minor version if you think it's a release with significant or breaking changes, or a teeny version if it's backward-compatible and insignificant. For now, please keep the major version (v0) as is.
  • If any issue that is specific to SQLite3 or caused by your change is filed, I would like you to fix and close it.

That's it. Please let me know if you have any questions.

@knaka
Copy link
Contributor Author

knaka commented Nov 18, 2022

Thank you for detailed instruction. There are still many parts of the code that I do not understand, so I will gradually work.

@knaka
Copy link
Contributor Author

knaka commented Nov 24, 2022

@k0kubun Could you permit me to push to master branch to update CHANGELOG.md?

@k0kubun
Copy link
Collaborator

k0kubun commented Nov 24, 2022

oh sorry, didn't notice you couldn't do that. I'll fix it today, but please use a pull request as a workaround until I fix it with my machine with YubiKey.

@k0kubun
Copy link
Collaborator

k0kubun commented Nov 24, 2022

OK, I dropped "Require status checks to pass before merging". You should now be able to push things to master directly.

@knaka
Copy link
Contributor Author

knaka commented Nov 25, 2022

Thank you!

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.

2 participants