sql: Avoid evaluating VALUES expressions during prepare.

[knz]

Spotted by @nvanbenschoten: prior to this patch VALUES would evaluate
its expressions during prepare, and thus fail (with either a crash or
an obscure error) if the expression contains a placeholder as argument
to a function.

Fixes #6225.

---

This change is

[knz]

This patch gets a failure on TestLogic for explain_debug:

--- FAIL: TestLogic (3.27s)
        logic_test.go:698: testdata/explain_debug:110: expected:
                "0"    "0"    "(1,"  "2,"   
                "3)"   "ROW"  "1"    "1"    
                "(4,"  "5,"   "6)"   "ROW"  
                but found:
                "0"    "0"    "(1,"  "2,"   
                "3)"   "ROW"  "1"    "1"    
                "(4,"  "5,"   "6)"   "ROW"  
                "2"    "2"    "(1,"  "2,"   
                "3)"   "ROW"  "3"    "3"    
                "(4,"  "5,"   "6)"   "ROW"  

Seems EXPLAIN(DEBUG) is now seeing the value rows two times. How come?

cc @RaduBerinde

[petermattis]

Review status: 0 of 3 files reviewed at latest revision, 6 unresolved discussions, some commit checks pending.

---

sql/values.go, line 33 [r1] (raw file):
You can move the initialization of v.rows into Start.

---

sql/values.go, line 50 [r1] (raw file):
s/one/once/g

---

sql/values.go, line 51 [r1] (raw file):
s/one/once/g

---

sql/values.go, line 77 [r1] (raw file):
Somewhat odd to define a method on a type before the type itself. Perhaps type valuesNode declaration to the top of this file to follow the pattern used for {delete,insert,update}Node.

---

sql/values.go, line 83 [r1] (raw file):
This comment is confusing to me. Which node is coming from a SQL query? The valuesNode? That seems obvious, but perhaps I'm missing something.

---

sql/values.go, line 94 [r1] (raw file):
s/aobut/about/g

---

Comments from Reviewable

[RaduBerinde]

^Looks like that statement is EXPLAIN (DEBUG) VALUES (1, 2, 3), (4, 5, 6). Definitely shouldn't be seeing those twice.. My guess is that Start is being called twice which results us appending the rows twice. We should check that n.rows is nil and panic if not in that function. We should probably add safeguards like that in more of the Start functions.

---

Review status: 0 of 3 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

sql/values_test.go, line 115 [r1] (raw file):
continue; instead of else/is. the non-error path should be non-indented as much as possible (see https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow)

---

Comments from Reviewable

[nvanbenschoten]

Thanks for fixing this @knz.

---

Review status: 0 of 3 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

sql/values.go, line 77 [r1] (raw file):
+1

---

sql/values.go, line 78 [r1] (raw file):
Is this check needed?

---

sql/values.go, line 93 [r1] (raw file):
Double check this for me, but I believe all of these methods could be called directly on the tuple without looping over it's sub-expressions.

---

Comments from Reviewable

[petermattis]

Review status: 0 of 3 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

sql/values.go, line 93 [r1] (raw file):
Yes, though we don't want to call Tuple.Eval as we've batched allocated the datums for the evaluated expressions.

---

Comments from Reviewable

[knz]

Review status: 0 of 8 files reviewed at latest revision, 9 unresolved discussions, some commit checks pending.

---

sql/values.go, line 33 [r1] (raw file):
Done.

---

sql/values.go, line 50 [r1] (raw file):
Done.

---

sql/values.go, line 51 [r1] (raw file):
Done.

---

sql/values.go, line 77 [r1] (raw file):
Done.

---

sql/values.go, line 78 [r1] (raw file):
Yes: sort.go and many others create valueNodes out of nothing just to store computed data.

---

sql/values.go, line 83 [r1] (raw file):
sort.go and many others create valueNodes out of nothing just to store computed data. No need to evaluate expressions for them.

---

sql/values.go, line 93 [r1] (raw file):
Indeed let's keep the batch alloc and call the individual evals.

---

sql/values.go, line 94 [r1] (raw file):
Done.

---

sql/values_test.go, line 115 [r1] (raw file):
Done.

---

Comments from Reviewable

[RaduBerinde]

Review status: 0 of 8 files reviewed at latest revision, 10 unresolved discussions, all commit checks successful.

---

sql/explain.go, line 218 [r2] (raw file):
Does this need to come after Start or can we move it before? I would imagine some nodes might want to know the explain mode during Start (especially if we move there some of the one-time initialization code from various Next implementations)
Either way we should document this for MarkDebug in planNode (whether it runs before or after Start)

---

Comments from Reviewable

[knz]

Review status: 0 of 8 files reviewed at latest revision, 10 unresolved discussions, all commit checks successful.

---

sql/explain.go, line 218 [r2] (raw file):
MarkDebug recurses into sub-nodes. However sub-nodes may not be determined before Start(). If we ant to make Start aware of the debug mode, we should add it as argument to Start().

---

Comments from Reviewable

[RaduBerinde]

Review status: 0 of 8 files reviewed at latest revision, 10 unresolved discussions, all commit checks successful.

---

sql/explain.go, line 218 [r2] (raw file):
Understood - can you add this bit of info to the planNode interface? Just something like "Plan subnodes may not be fully determined until Start is run" as part of the comment for Start, and "MarkDebug can only be called between Start and the first call to Next" for MarkDebug

---

Comments from Reviewable

[knz]

Review status: 0 of 8 files reviewed at latest revision, 10 unresolved discussions, all commit checks successful.

---

sql/explain.go, line 218 [r2] (raw file):
Done.

---

Comments from Reviewable

[RaduBerinde]

Reviewed 1 of 3 files at r1, 7 of 7 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

---

sql/plan.go, line 461 [r3] (raw file):
This is saying the wrong thing, is it missing a "before? E.g. but before the first call to Next()

---

sql/values.go, line 83 [r1] (raw file):
maybe just add "coming from a SQL query (as opposed to being internally created by another node)"

---

Comments from Reviewable

[knz]

Review status: 6 of 9 files reviewed at latest revision, 9 unresolved discussions, some commit checks pending.

---

sql/plan.go, line 461 [r3] (raw file):
Done.

---

sql/values.go, line 83 [r1] (raw file):
Done.

---

Comments from Reviewable

[RaduBerinde]

Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[knz]

LGTY?

[RaduBerinde]

---

Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

---

Comments from Reviewable