Create dolt_help system table #8739

milogreg · 2025-01-13T22:32:56Z

Created the dolt_help system table. This table is meant to store documentation for system tables, procedures, functions, and variables. Currently dolt_help is only populated with documentation for procedures, and only procedures that have equivalent CLI commands.

Part of #7984

zachmu

This is the right idea, but it's missing a few things. Check out the difference between the command line and sql output here:

select arguments from dolt_help where target = 'dolt_add';
+-------------------------------------------------------------------------------------------------------------------------------------+
| arguments                                                                                                                           |
+-------------------------------------------------------------------------------------------------------------------------------------+
| {"table": "Working table(s) to add to the list tables staged to be committed. The abbreviation '.' can be used to add all tables."} |
+-------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

v. command line:

% dolt add --help
NAME
        dolt add - Add table contents to the list of staged tables

SYNOPSIS
        dolt add [<table>...]

DESCRIPTION

        This command updates the list of tables using the current content found in the working root, to prepare the content staged for the next commit. It adds the current content of existing
        tables as a whole or remove tables that do not exist in the working root anymore.

        This command can be performed multiple times before a commit. It only adds the content of the specified table(s) at the time the add command is run; if you want subsequent changes
        included in the next commit, then you must run dolt add again to add the new content to the index.

        The dolt status command can be used to obtain a summary of which tables have changes that are staged for the next commit.

OPTIONS
        <table>
          Working table(s) to add to the list tables staged to be committed. The abbreviation '.' can be used to add all tables.

        -A, --all
          Stages any and all changes (adds, deletes, and modifications) except for ignored tables.

        -f, --force
          Allow adding otherwise ignored tables.

        -p, --patch
          Interactively select changes to add to the staged set.

So relative to the CLI, this is missing:

options (flags)
synopsis
<table> formatting for arguments

zachmu · 2025-01-14T00:42:11Z

go/libraries/doltcore/sqle/dtables/help_table.go

+	return NewHelpRowIter(), nil
+}
+
+type HelpRowIter struct {


Best practice here is to use a pointer for the receiver type and not the fields

zachmu · 2025-01-14T01:15:50Z

go/libraries/doltcore/sqle/dtables/help_table.go

+
+			if hasProcedure && docs != nil {
+				argsMap := map[string]string{}
+				for _, argHelp := range curr.Docs().ArgParser.ArgListHelp {


This is missing all the options (flags)

zachmu · 2025-01-14T01:19:18Z

go/libraries/doltcore/sqle/dtables/help_table.go

+func (ht *HelpTable) Schema() sql.Schema {
+	return []*sql.Column{
+		{
+			Name:           "target",


Would call this name instead

go/libraries/doltcore/sqle/dtables/help_table.go

zachmu

This looks great with the exception of a couple weird bugs I found during testing.

The other high level comment is you should add a couple spot check tests in the enginetest package. Add a new method in dolt_engine_test.go that verifies the output of a couple commands (and verifies the synopsis behavior bug below isn't there anymore).

Fix those things and you're good to merge this.

zachmu · 2025-01-16T01:42:31Z

go/libraries/doltcore/sqle/dtables/help_table.go

+					argsMap[usage[0]] = usage[1]
+				}
+
+				argsJson, err := json.Marshal(argsMap)


I'm not sure why, but something about this seems to be producing invalid json in a subtle and weird way. Check this out:

db1/main> select json_pretty(arguments) from dolt_help where name = 'dolt_revert'; invalid data type for JSON data in argument 1 to function json_pretty; a JSON string or JSON type is required db1/main> create table json_test (j json); db1/main*> insert into json_test (select arguments from dolt_help where name = 'dolt_revert'); db1/main*> select json_pretty(j) from json_test; +-----------------------------------------------------------------------------------------------------------------------------------+ | json_pretty(j) | +-----------------------------------------------------------------------------------------------------------------------------------+ | { | | "--author=\u003cauthor\u003e": "Specify an explicit author using the standard A U Thor \[email protected]\u003e format.", | | "\u003crevision\u003e": "The commit revisions. If multiple revisions are given, they're applied in the order given." | | } | +-----------------------------------------------------------------------------------------------------------------------------------+ 1 row in set (0.00 sec)

So whatever it is, it's getting handled by our round-trip into storage, but can't get processed by as returned by this table. This seems to affect all our json functions, so it needs to be fixed. Another example:

db1/main*> select arguments->>"$.<revision>" from dolt_help where name = 'dolt_revert'; invalid data type for JSON data in argument 1 to function json_extract; a JSON string or JSON type is required db1/main*> select j->>"$.<revision>" from json_test; +--------------------------------------------------------------------------------------------+ | j->>"$.<revision>" | +--------------------------------------------------------------------------------------------+ | The commit revisions. If multiple revisions are given, they're applied in the order given. | +--------------------------------------------------------------------------------------------+ 1 row in set (0.00 sec)

I think it has something to do with the < and > characters. I would expect json.Marshall to do something reasonable there but it apparently is not.

Above examples work great now, but there's a related problem where ascii command characters (used for bolding on the terminal) are making it into the SQL output like this:

"-f, --force": "Reset \u003cbranchname\u003e to \u003cstartpoint\u003e, even if \u003cbranchname\u003e exists already

I think what we need here is the OptionsUsageList() method to accept a Formatter object (needs to be defined) that we can use to change out the formatting behavior. For the SQL use case, we want to pass a formatter that just deletes the template options, rather than replacing them with command chars like the CLI implementation does.

go/libraries/doltcore/sqle/dtables/help_table.go

…elections of synopsis

zachmu

This is looking pretty good. One major comment about this, which is that we need configurable format options for the OptionUsageList method.

Fix that and we're good to merge.

zachmu · 2025-01-17T23:22:38Z

go/cmd/dolt/doltcmd/doltcmd.go

+	LongDesc:  `Dolt comprises of multiple subcommands that allow users to import, export, update, and manipulate data with SQL.`,
+
+	Synopsis: []string{
+		"<--data-dir=<path>> subcommand <subcommand arguments>",


Suggested change

"<--data-dir=<path>> subcommand <subcommand arguments>",

"[global flags] subcommand [subcommand arguments]",

zachmu · 2025-01-17T23:44:13Z

go/libraries/doltcore/sqle/dtables/help_table.go

+	return NewHelpRowIter(), nil
+}
+
+type HelpRowIter []sql.Row


Prefer to define an actual struct here, with []sql.Row field and a counter int to keep track of the place in it.

zachmu · 2025-01-17T23:46:40Z

go/libraries/doltcore/sqle/dtables/help_table.go

+			procedureName := strings.ReplaceAll(fullName, "-", "_")
+
+			hasProcedure := false
+			for _, procedure := range dprocedures.DoltProcedures {


Would pull out a boolean procedureExists method for this, and check for this condition before beginning the loop

zachmu · 2025-01-17T23:47:54Z

go/libraries/doltcore/sqle/dtables/help_table.go

+			continue
+		}
+
+		if subCmdHandler, ok := curr.(cli.SubCommandHandler); ok {


This loop can be simplified, because there are no procedures that correspond to sub commands. E.g. there is no dolt_table_ls command.

zachmu · 2025-01-17T23:56:47Z

go/libraries/doltcore/sqle/dtables/help_table.go

+					argsMap[usage[0]] = usage[1]
+				}
+
+				argsJson, err := json.Marshal(argsMap)


Above examples work great now, but there's a related problem where ascii command characters (used for bolding on the terminal) are making it into the SQL output like this:

"-f, --force": "Reset \u003cbranchname\u003e to \u003cstartpoint\u003e, even if \u003cbranchname\u003e exists already

I think what we need here is the OptionsUsageList() method to accept a Formatter object (needs to be defined) that we can use to change out the formatting behavior. For the SQL use case, we want to pass a formatter that just deletes the template options, rather than replacing them with command chars like the CLI implementation does.

zachmu · 2025-01-18T00:00:54Z

go/libraries/doltcore/sqle/enginetest/dolt_queries_help.go

+		SetUpScript: []string{},
+		Assertions: []queries.ScriptTestAssertion{
+			{
+				Query: "select long_description from dolt_help where name='dolt_add'",


This test is probably over specified -- we tweak these pretty often, don't want to check the whole thing.

I would probably do something like select INSTR(long_description, "add") > 0, the idea is just a spot test to be sure this returns the correct contents, not to check the contents verbatim.

zachmu · 2025-01-18T00:01:42Z

go/libraries/doltcore/sqle/enginetest/dolt_queries_help.go

+	},
+
+	{
+		Name:        "dolt_help arguments are correct",


Same comment here, loosen up these test to make them less likely to break when we change things in the future.

What you have in the bats tests is much closer to what we want here.

zachmu · 2025-01-18T00:05:24Z

Also fix the copyright header on your new file, it needs to match other files exactly.

milogreg added 2 commits January 13, 2025 13:52

Create dolt_help system table

13c546b

Update system-tables.bats to include tests for dolt_help table

f86a441

coffeegoddd added the contribution label Jan 13, 2025

milogreg added 3 commits January 13, 2025 14:39

Fix unnecessary recomputation

d0fca91

Fix procedure name formatting

485cbb3

Fix failing go test

2009c87

zachmu reviewed Jan 14, 2025

View reviewed changes

milogreg added 6 commits January 14, 2025 11:35

Fix failing bats test

b17e52d

Update schema

0495fe3

Add flags in addition to arguments to dolt_help table

8373c72

Populate synopsis in dolt_help table

7d042f6

Clean up HelpRowIter

4355fae

Update bats tests

4c0ab89

milogreg requested a review from zachmu January 14, 2025 21:19

zachmu reviewed Jan 16, 2025

View reviewed changes

milogreg added 4 commits January 16, 2025 12:45

Fix invalid JSON bug

6e87334

Fix synopsis bug

920e7f0

Add enginetests for dolt_help argument JSON validity and successive s…

f88f7b9

…elections of synopsis

Add enginetests to validate results of selects on dolt_help

dbee26c

milogreg requested a review from zachmu January 16, 2025 23:25

zachmu reviewed Jan 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create dolt_help system table #8739

Create dolt_help system table #8739

milogreg commented Jan 13, 2025

zachmu left a comment

zachmu Jan 14, 2025

zachmu Jan 14, 2025

zachmu Jan 14, 2025

zachmu left a comment

zachmu Jan 16, 2025

zachmu Jan 16, 2025

zachmu Jan 17, 2025

zachmu left a comment

zachmu Jan 17, 2025

zachmu Jan 17, 2025

zachmu Jan 17, 2025

zachmu Jan 17, 2025

zachmu Jan 17, 2025

zachmu Jan 18, 2025

zachmu Jan 18, 2025

zachmu Jan 18, 2025

zachmu commented Jan 18, 2025

	"<--data-dir=<path>> subcommand <subcommand arguments>",
	"[global flags] subcommand [subcommand arguments]",

Create dolt_help system table #8739

Are you sure you want to change the base?

Create dolt_help system table #8739

Conversation

milogreg commented Jan 13, 2025

zachmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmu commented Jan 18, 2025