Cli act tests #1260

brennanjl · 2025-01-21T06:29:53Z

Still some work to do on this but putting it up for a discussion

config/config.go

brennanjl · 2025-01-21T07:38:06Z

Ok this is ready to go. There are still some tests we need with roundtripping all data types, kgw, etc., but for a beta this should be enough.

config/config.go

jchappelow

LGTM. @Yaiba probably should look though.

jchappelow · 2025-01-21T22:54:10Z

cmd/kwil-cli/cmds/exec-action.go

+	execActionLong = `TODO: fill me out`
+
+	execActionExample = `TODO: fill me out`


I was wondering how this compared to database execute etc.

So @Yaiba pointed out that our old version of database execute simply doesn't work with some of the new things supported within the engine. Namely, it requires all named parameters, rather than positional (which doesn't work for a lot of the things Gavin is doing with extension methods).

Furthermore, the old execute got pretty overloaded. Now that we have ad-hoc INSERT/UPDATE/DELETE, it had to also handle that (at least that's what I felt was most clear, since execute is definitely what I would expect to use to do that).

So I sort've took this as a chance to fix that. Now, it is clear what to use for actions and SQL. It can still use named params if you want, but positional is the default. The same command that handles executing an action once also handles it with CSV values.

I have another commit coming that adds docs for this, as well as a few other things. Since this is already reviewed, we can merge this and then Ill add the other later, since it has a lot of stuff (found a lot of bugs/edge cases)

jchappelow · 2025-01-21T23:01:18Z

core/types/results.go

+		if !wrote {
+			// if it didnt write a value, do not add the empty pointer to the slice


This means "leave the nil pointer in the slice" or, equivalently, "do not insert a non-nil pointer to the zero value for T"? I didn't know what an "empty pointer" was describing at first.

I think thats a bad comment, will fix.

It should say: "if convertScalar did not write to val, we should not add the new pointer to the slice"

Yaiba · 2025-01-21T23:14:48Z

LGTM. @Yaiba probably should look though.

will take a quick scan

Yaiba · 2025-01-21T23:33:14Z

cmd/kwil-cli/cmds/params.go

+
+	for _, arg := range args {
+		// trim off any leading '$'
+		arg = strings.TrimPrefix(arg, "$")


Just to point it out that this is unlikely to happen, if user pass $xx(without single quote) to the cli, it'll be expanded by SHELL.

They often times will need to wrap it in single quotes though. For example, if they are passing an array, they have to do:

'param:int[]=[1,2]' # or '$param:int[]=[1,2]' # regardless, they need single quotes

So IMO we should keep it

Yaiba · 2025-01-21T23:34:28Z

cmd/kwil-cli/cmds/params_test.go

+	tests := []testcase{
+		// scalars
+		{"username:text=satoshi", "username", "satoshi", false},
+		{`username:text="satoshi"`, "username", "satoshi", false},


I think we should only support single quote, to avoid the confusion if user want to insert a text like $xxx, because the SHELL will expand the value

This is still the case though. For example:

$ ab='hello!'; kwil-cli query 'select $a' -p a:text="$ab" | | +--------+ | hello! |

The double quotes will only get parsed if a user wraps the entire param in them:

ab='hello!'; kwil-cli query 'select $a' -p 'a:text="$ab"' | | +-----+ | $ab |

Yaiba · 2025-01-21T23:38:51Z

node/engine/interpreter/planner.go

@@ -75,6 +85,28 @@ func makeActionToExecutable(namespace string, act *Action) *executable {
 			for _, stmt := range stmtFns {
 				err := stmt(exec2, func(row *row) error {
 					row.columns = returnColNames
+
+					// we will ensure that the return values match the expected return types


Yaiba · 2025-01-21T23:56:54Z

lgtm, only issue I have is parsing 'text' type in cli I mentioned at #1260 (comment)

new commands for executing sql, actions

brennanjl · 2025-01-22T05:05:28Z

I ended up including some other changes in this PR. It's all in the last commit, so if you would like me to pop it off into a separate PR so that it is easier to review, I'm happy to.

It doesn't include much new stuff except tests, bug fixes, and docs for the new CLI commands.

many encoding, engine, and CLI fixes ran tidy fix lint

jchappelow

LGTM. Send it

brennanjl commented Jan 21, 2025

View reviewed changes

config/config.go Show resolved Hide resolved

brennanjl marked this pull request as ready for review January 21, 2025 07:37

charithabandi self-assigned this Jan 21, 2025

jchappelow reviewed Jan 21, 2025

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

brennanjl force-pushed the cli-act-tests branch from 2c04f9d to 15c8798 Compare January 21, 2025 17:38

brennanjl mentioned this pull request Jan 21, 2025

database call/execute doesnt work on extension methods #1269

Closed

jchappelow previously approved these changes Jan 21, 2025

View reviewed changes

jchappelow requested a review from Yaiba January 21, 2025 23:13

Yaiba reviewed Jan 21, 2025

View reviewed changes

brennanjl dismissed jchappelow’s stale review via e2baa35 January 22, 2025 05:01

brennanjl added 7 commits January 21, 2025 23:01

kwil-cli: new param passing

6f44bcc

new commands for executing sql, actions

allow ethereum addresses in genesis

04c0762

update hex bytes

e0d990f

fixed transfer test

f395f88

config: marshal ethereum addresses with 0x

b75fbf1

fix unit tests

f6c2a4b

fix lint

63284c1

brennanjl force-pushed the cli-act-tests branch from e2baa35 to 6c98867 Compare January 22, 2025 05:02

brennanjl force-pushed the cli-act-tests branch from 6c98867 to b8ca56a Compare January 22, 2025 05:06

debugging null bytea

32f9204

many encoding, engine, and CLI fixes ran tidy fix lint

brennanjl force-pushed the cli-act-tests branch from b8ca56a to 32f9204 Compare January 22, 2025 05:09

jchappelow approved these changes Jan 22, 2025

View reviewed changes

brennanjl merged commit 88353e6 into main Jan 22, 2025
2 checks passed

brennanjl deleted the cli-act-tests branch January 22, 2025 05:15

This was referenced Jan 22, 2025

fix types.QueryResult.Scan when client side use it #1267

Closed

Add round-trip testing for all data types #1264

Closed

This was referenced Jan 22, 2025

Bug: Invalid Signature Errors not returned over json rpc #1242

Closed

kwil-cli logs unexpected info #1238

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cli act tests #1260

Cli act tests #1260

brennanjl commented Jan 21, 2025

brennanjl commented Jan 21, 2025

jchappelow left a comment

jchappelow Jan 21, 2025

brennanjl Jan 21, 2025

brennanjl Jan 21, 2025

jchappelow Jan 21, 2025 •

edited

Loading

brennanjl Jan 21, 2025

Yaiba commented Jan 21, 2025

Yaiba Jan 21, 2025

brennanjl Jan 22, 2025 •

edited

Loading

Yaiba Jan 21, 2025

brennanjl Jan 22, 2025

Yaiba Jan 21, 2025

Yaiba commented Jan 21, 2025

brennanjl commented Jan 22, 2025

jchappelow left a comment

		execActionLong = `TODO: fill me out`

		execActionExample = `TODO: fill me out`

		if !wrote {
		// if it didnt write a value, do not add the empty pointer to the slice

Cli act tests #1260

Cli act tests #1260

Conversation

brennanjl commented Jan 21, 2025

brennanjl commented Jan 21, 2025

jchappelow 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

jchappelow Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yaiba commented Jan 21, 2025

Choose a reason for hiding this comment

brennanjl Jan 22, 2025 • edited Loading

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

Yaiba commented Jan 21, 2025

brennanjl commented Jan 22, 2025

jchappelow left a comment

Choose a reason for hiding this comment

jchappelow Jan 21, 2025 •

edited

Loading

brennanjl Jan 22, 2025 •

edited

Loading