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

Create Dolt Database #2131

Merged
merged 15 commits into from
Sep 16, 2021
Merged

Create Dolt Database #2131

merged 15 commits into from
Sep 16, 2021

Conversation

andy-wm-arthur
Copy link
Contributor

No description provided.

@andy-wm-arthur andy-wm-arthur marked this pull request as ready for review September 13, 2021 19:08
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

I don't love it, but fine moving forward with this for now.

Particularly, the coupling of repo state and creation of in-memory databases is pretty nasty, should clean that up soon

pro := dsqle.NewDoltDatabaseProvider(dbs...)
cat := sql.NewCatalogWithDbProvider(pro)

infoDB := information_schema.NewInformationSchemaDatabase()
Copy link
Member

Choose a reason for hiding this comment

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

Past time to clean this up, no reason to make the integrator add this thing.

This should be available all the time based on databases provided to the engine.

Add a TODO?

ctx context.Context,
name, email string,
t time.Time,
def ref.BranchRef,
Copy link
Member

Choose a reason for hiding this comment

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

def?

"github.com/dolthub/dolt/go/libraries/doltcore/ref"
)

func NewMemoryDbData(ctx context.Context) (DbData, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this in the env package?

Copy link
Member

Choose a reason for hiding this comment

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

I mean all these functions, not just this one

return spec
}

func (m MemoryRepoState) UpdateStagedRoot(ctx context.Context, newRoot *doltdb.RootValue) error {
Copy link
Member

Choose a reason for hiding this comment

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

Mind adding a TODO to this and the other root update method? I'm in the middle of killing them in the interface, want to make sure to get them here too

}

func (m MemoryRepoState) UpdateBranch(name string, new BranchConfig) error {
return nil
Copy link
Member

Choose a reason for hiding this comment

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

Should throw an error

@@ -140,6 +153,41 @@ func NewDatabase(name string, dbData env.DbData, editOpts editor.Options) Databa
}
}

// GetInitialDBState returns the InitialDbState for |db|.
func GetInitialDBState(ctx context.Context, db Database) (dsess.InitialDbState, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Add a note here that this is only to support the creation of in-memory databases.

The method of using repo state to initialize these things is pretty janky

@@ -26,7 +26,7 @@ defmodule SmokeTest do
{:ok, result} = MyXQL.query(pid, "UPDATE test SET pk = pk where pk = 0")
myTestFunc(result.num_rows, 1)

{:ok, result} = MyXQL.query(pid, "INSERT INTO test VALUES (0, 0) ON DUPLICATE KEY UPDATE value = value")
{:ok, result} = MyXQL.query(pid, "INSERT INTO test VALUES (0, 0) ON DUPLICATE KEY UPDATE `value` = `value`")
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a bad regression. File an issue?

@VinaiRachakonda
Copy link
Contributor

This is pretty dope. Think the memory implementations will help new contributors a lot with understanding the fundamentals

@andy-wm-arthur andy-wm-arthur merged commit d6ba9e2 into master Sep 16, 2021
@andy-wm-arthur andy-wm-arthur deleted the andy/create-dolt-database branch September 16, 2021 16:40
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.

3 participants