Skip to content

Conversation

mallman
Copy link
Collaborator

@mallman mallman commented Oct 20, 2020

This PR adds support for generated columns to GRDB, including enhancements to the column definition API for defining generated columns. It arises from #854.

User documentation has not been updated (yet).

Thorough unit testing will require building against a version of SQLite that actually supports generated columns. Not sure how to handle that here...

Pull Request Checklist

  • This pull request is submitted against the development branch.
  • Inline documentation has been updated.
  • README.md or another dedicated guide has been updated.
  • Changes are tested.

Copy link
Owner

@groue groue left a comment

Choose a reason for hiding this comment

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

@mallman, this is quality pull request.

Will you please perform the requested changes, so that we do not drop support for the SQLite versions that ships with iOS, macOS, etc?

After that:

  • Blocking: The https://github.com/swiftlyfalling/SQLiteLib submodule must be updated to SQLite 3.33.0

    I take this on me. Please be patient: @swiftlyfalling is sometimes slow to respond. I was granted push access to his repository, so we are not blocked. Yet I'm too courteous for pushing right away without asking first.

  • Blocking: We need support for table alteration as well. According to the doc, it is possible to add virtual generated columns.

    I always attempt at maximally cover features, because nothing is more frustrating than obvious holes in an api.

    Do you feel like adding this?

  • Blocking: We need tests.

    Do you feel like writing tests?

  • Non-blocking: We need to add documentation.

    At first sight, the documentation could be written as:

    If you feel like writing this doc, go ahead, this is a funny exercise! Otherwise, I'll happily do it.

@groue
Copy link
Owner

groue commented Oct 21, 2020

SQLite 3.33.0: swiftlyfalling/SQLiteLib#48

@mallman
Copy link
Collaborator Author

mallman commented Oct 22, 2020

@groue I'll work on making more progress on the areas you outlined within the next few days. Cheers.

@mallman
Copy link
Collaborator Author

mallman commented Oct 25, 2020

I've pushed some minor revisions to this PR, including some rudimentary tests for the new generated column table definition API. We will want/need some additional test coverage to validate that generated columns are actually working in DML queries. I imagine those should go in an entirely new test file. I'll look for inspiration from a similar class of tests in adding this.

Copy link
Owner

@groue groue left a comment

Choose a reason for hiding this comment

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

Thank you, @mallman.

@groue
Copy link
Owner

groue commented Oct 31, 2020

We will want/need some additional test coverage to validate that generated columns are actually working in DML queries.

We don't want to test SQLite. But we do want to test the changes to Database.columns(in:), yes.

@groue
Copy link
Owner

groue commented Oct 31, 2020

OK, I added the integration tests that make me comfortable. Basically, I asked Xcode for the call hierarchy of Database.columns(in:), and made sure generated columns are well handled. I also made sure it is an error to use record types to insert or update generated columns. In my opinion, the testing requirement is completed.

@groue
Copy link
Owner

groue commented Oct 31, 2020

I now look at the documentation.

I still think that we need to provide good practices, but this will be done later. There are several kinds of records that need advice. On our specific topic, records on generated columns are similar to records on columns with default values, and records on views, and they should be addressed together. Other kinds of records need advice as well, such as singletons.

I'll thus just add doc on the table creation DSL.

@groue
Copy link
Owner

groue commented Oct 31, 2020

... and support for query interface expressions has been added as well.

OK now let's wait for CI, and we'll be good.

@mallman
Copy link
Collaborator Author

mallman commented Oct 31, 2020

Thank you for your help getting this across the finish line. I've been working on my primary project this past week.

@mallman
Copy link
Collaborator Author

mallman commented Oct 31, 2020

What are your thoughts about what version of GRDB you'll introduce these changes in?

@groue
Copy link
Owner

groue commented Oct 31, 2020

What are your thoughts about what version of GRDB you'll introduce these changes in?

The next one, 5.1, very soon. I just have a little other topic to look at.

May I ask a question? Two, actually.

How do you integrate your custom SQLite build with GRDB? It looks like you do not depend on https://github.com/swiftlyfalling/SQLiteLib.

And... Would you accept an invitation that grants you a push access to GRDB?

@mallman
Copy link
Collaborator Author

mallman commented Oct 31, 2020

How do you integrate your custom SQLite build with GRDB? It looks like you do not depend on https://github.com/swiftlyfalling/SQLiteLib.

I modify my local checkout of SQLiteLib with whatever version of SQLite I want to use. Of course, now that SQLiteLib uses 3.33.0, this will no longer be necessary.

And... Would you accept an invitation that grants you a push access to GRDB?

Yes, but please specify some guidelines for my usage of that privilege. I aim to be courteous and conservative when pushing to someone else's repo, but specific communication will help.

@groue
Copy link
Owner

groue commented Oct 31, 2020

How do you integrate your custom SQLite build with GRDB? It looks like you do not depend on https://github.com/swiftlyfalling/SQLiteLib.

I modify my local checkout of SQLiteLib with whatever version of SQLite I want to use. Of course, now that SQLiteLib uses 3.33.0, this will no longer be necessary.

All right :-)

And... Would you accept an invitation that grants you a push access to GRDB?

Yes, but please specify some guidelines for my usage of that privilege. I aim to be courteous and conservative when pushing to someone else's repo, but specific communication will help.

With pleasure, and thanks for asking.

Our interaction here has been perfect, as well as previous ones, and I'm very pleased to welcome you. This is how I expect future contributions to happen: we engage in a quality conversation that ends with a solution that benefits all GRDB users.

I do intend to keep the lead of the project, because I know it well, I am able to provide guidance, and I did not write the informal "rules" that drive GRDB. You can see in #855 (review) some examples of such rules: clear requirements, tests, documentation, respect for SQLite, and respect for users, so that they can build simple mental models and find the apis that support them. This is the "soul" of GRDB I'd like to foster, and so far I have nothing but conversations in order to engage contributors with this "soul".

Yet I may be unable to answer some day. It is good for everyone that people of various levels of Swift and SQLite expertise can act when it becomes necessary. I propose the push access to all authors of merged pull requests, as long as I see compatibility with the goals of the library.

EDIT: maintaining a library is time consuming, so I do not ask for a stronger involvement. But I'm super happy to support ideas I coud not come up with myself, and I'd be pleased to share more as a relationship evolves.

Is it ok for you? Any other question?

@groue
Copy link
Owner

groue commented Oct 31, 2020

OK now let's wait for CI, and we'll be good.

All right, Travis only exhibits its regular timeouts on a couple of setups (🙄). We can merge 💯

@groue groue merged commit 2389ec6 into groue:development Oct 31, 2020
@mallman
Copy link
Collaborator Author

mallman commented Oct 31, 2020

Is it ok for you? Any other question?

Sounds good to me. Thank you.

@mallman mallman deleted the generated_columns branch October 31, 2020 18:01
@groue groue changed the title Initial commit for generated column support Support for generated columns with custom SQLite build Oct 31, 2020
@groue
Copy link
Owner

groue commented Nov 1, 2020

Shipped in v5.1.0!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants