sql, tests: change empty strings rendering in sqllogictest suite

[rampage644]

Right now empty string values are not rendered as documentation suggests:

In the results section, integer values are rendered as if by printf("%d"). Floating point values are rendered as if by printf("%.3f"). NULL values are rendered as "NULL". Empty strings are rendered as "(empty)". Within non-empty strings, all control characters and unprintable characters are rendered as "@".

Some problems arise with empty strings. Sometimes empty string is not added to the row (while parsing test file and query result), and sorting sorts "interleaved" rows instead (because it expects 5 columns and get only 4 thus taking 1 element from neighbor row). Simple fix solves that but requires to rewrite logictest files.

This PR renders empty strings with "(empty)" and modifies all logic test files to make tests pass.

Closes #17134.

[cockroach-teamcity]

This change is 

[jordanlewis]

Cool. Can you please check your implementation against the test files that get run by the -bigtest switch? Last time I tried something like this I ran into issues there. Those testfiles are found in http://github.com/cockroachdb/sqllogictest.

[rampage644]

Yes, I'll have a look.

[RaduBerinde]

I sure hope you knew about the -rewrite-results-in-testfiles flag when you generated all these diffs :)
+1 on checking what happens with the bigtest suite.

---

Review status: 0 of 40 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[knz]

Thanks for the hard work.

I am not completely fond of the approach taken. Regarding the bigtest I would be surprised if it passes.
There are other possibly less-intrusive solution to the problem outlined in #17134:

• make it part of the test specs that the reference output is columnar (with a fixed number of characters per columns) and not strip/reduce the whitespace when sorting rows.
• require an explicit "empty string" marker in reference output only for those tests marked "rowsort"

Should you go forward with your current approach I have two requests however:

• please clean up the trailing whitespaces. Radu's suggestion to run with -rewrite-results-in-testfiles will do that, and so will the command M-x whitespace-cleanup in emacs.

• visually I find the string "(empty)" too distracting. There is wisdom to be found in having empty strings also quiet visually. May I suggest you instead use some non-ascii punctuation character such as the middle dot ("·") instead.

---

Reviewed 1 of 1 files at r1.
Review status: 1 of 40 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[rampage644]

@RaduBerinde I wasn't smart enough to read the code, so I have to reinvent the wheel ;) Thankfully, it didn't take much time.

Regarding bigtest switch: it passes:

# ramp @ thinkpad430s in ~/soft/go/src/github.com/cockroachdb/cockroach on git:issue17134 x [18:56:37] 
$ make testlogic 
GOPATH set to /home/ramp/soft/go
Running make with -j4
Running make with -j4
cd pkg/sql/logictest && logictest.test -test.run "TestLogic" -test.v  -bigtest
I170724 18:58:08.244535 1 rand.go:76  Random seed: 4101253071964275013
... (omitted)
# ramp @ thinkpad430s in ~/soft/go/src/github.com/cockroachdb/cockroach on git:issue17134 x [19:40:29] 
$ echo $?
0

However, since you were suspecting them to fail, I think now that probably I wasn't running it correctly. I've changed Makefile to always include -bigtest and removed timeout switch. Can please anyone check the result?

Next, my intention was to try to comply with original ref spec. However, I'm completely flexible with a solution. Right now the problem is with empty string value being either at the first or the last position. I can come with different approach if other criterion is better than sqlite spec compliance.

[RaduBerinde]

This is the way to run bigtest (lifted from build/teamcity-sqllogictest.sh):

make test TESTFLAGS="-v -bigtest -config default" TESTTIMEOUT='24h' PKG='./pkg/sql/logictest' TESTS='^TestLogic$$'

and

make test TESTFLAGS="-v -bigtest -config distsql" TESTTIMEOUT='24h' PKG='./pkg/sql/logictest' TESTS='^TestLogic$$'

It should take a while to run.

It's possible that there are no empty cases in the bigtest.

---

Review status: 1 of 40 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/sql/logictest/testdata/logic_test/aggregate, line 256 at r2 (raw file):

EXPLAIN (VERBOSE) SELECT COUNT(*), k+v FROM kv GROUP BY k+v
----
0  group    (empty)      (empty)                ("count(*)", "k + v")           (empty)  

All these lines have extra whitespaces at the end, which can be annoying when editing, and more importantly this will generate a lot of diffs next time someone uses -rewrite-results-in-testfiles. Can you rerun with that flag so it fixes all these?

---

Comments from Reviewable

[rampage644]

It's possible that there are no empty cases in the bigtest.

---

Review status: 1 of 40 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/sql/logictest/testdata/logic_test/aggregate, line 256 at r2 (raw file):

Previously, RaduBerinde wrote…

All these lines have extra whitespaces at the end, which can be annoying when editing, and more importantly this will generate a lot of diffs next time someone uses -rewrite-results-in-testfiles. Can you rerun with that flag so it fixes all these?

Sure, it's already on my TODO list. I planned to wait for more feedback and probably revert previous commit first so it be easier to review.

---

Comments from Reviewable

[rampage644]

Different invocation options didn't change the result I'm getting:

# ramp @ thinkpad430s in ~/soft/go/src/github.com/cockroachdb/cockroach on git:issue17134 x [12:34:15] 
$ make test TESTFLAGS="-v -bigtest -config default" TESTTIMEOUT='24h' PKG='./pkg/sql/logictest' TESTS='^TestLogic$$'
... (omitting most output)
PASS
ok  	github.com/cockroachdb/cockroach/pkg/sql/logictest	2548.112s

I've also rebased the branch (to resolve the conflict) and included only the commit with generated files by -rewrite-results-in-testfiles.

For now I didn't changed the solution. Waiting for more feedback on it.

[RaduBerinde]

I'll let @knz have the final word on this. The only thing that's not great is that the output of EXPLAIN statements is now harder to look at (most of those statement are there to help one understand the testcases).

[knz]

So overall I'm happy if the bigtest passes, but then I wouldn't be surprised if that indeed didn't use any empty strings, so we might just be lucky. Should this be the case, we could later add a configuration flag that gates the change made here to keep the compatibility, so I am not worried.

Regarding the remaining I am still concerned about readability, and my previous request stands: use another string than "(empty)", preferably something that's visually as short as possible. I made a concrete suggestion above ("·") but feel free to propose an alternative.

---

Reviewed 1 of 39 files at r4.
Review status: 2 of 40 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable

[petermattis]

How about "∅" (empty set) or "␀" (nul symbol)? I'd prefer the former as the latter is a bit hard to read.

---

Review status: 2 of 40 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable

[knz]

This looks good, but please squash the two commits together before we can merge this PR.

---

Review status: 0 of 41 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable

[knz]

Excellent! Thanks for your contribution!

[rampage644]

Thanks for accepting!