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

logictest: add a kvtrace test type to logic tests. #45563

Merged
merged 1 commit into from
Mar 2, 2020

Conversation

jordanlewis
Copy link
Member

@jordanlewis jordanlewis commented Mar 1, 2020

It's common to want to test "kv trace" output in execbuilder tests, to
make sure that a particular SQL statement emits the expected set of key
reads, writes, deletes, and so on. It's very annoying to re-type out the
setup for kv tracing every time (it's multiple boiler plate-y lines), so
this commit introduces a shortcut for this common operation.

It looks like this:

query T kvtrace
SELECT * FROM t
----
<expected kv trace output>

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rohany
Copy link
Contributor

rohany commented Mar 1, 2020

Oh this is pretty slick -- is it possible to do something like

query T kvtrace(CPut, Get, InitPut)
...

To only see the requested operations?

Also, would it be too special cased to have the command accept a table id or an index id to filter on?

return err
}
query.colTypes = "T"
query.sql = `SELECT message FROM [SHOW KV TRACE FOR SESSION] WITH ORDINALITY
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need WITH ORDINALITY here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know, I just cargo culted it from somewhere else :) I'll get rid of it.

@rohany
Copy link
Contributor

rohany commented Mar 1, 2020

Might also be worth it to make an issue tracking transferring all our existing tests that do the boilerplate to use this

It's common to want to test "kv trace" output in execbuilder tests, to
make sure that a particular SQL statement emits the expected set of key
reads, writes, deletes, and so on. It's very annoying to re-type out the
setup for kv tracing every time (it's multiple boiler plate-y lines), so
this commit introduces a shortcut for this common operation.

It looks like this:

query T kvtrace
SELECT * FROM t
----
<expected kv trace output>

Release note: None
@jordanlewis
Copy link
Member Author

As far as enhancing the test type to include expected op types, I think it's a little too annoying at the moment given the way the logic test parser works. I could do it later maybe but for now I don't really want to bother, since it'll probably work for most use cases at the moment just fine.

@jordanlewis
Copy link
Member Author

I agree it's a good idea to transfer the existing tests to use this but yeah I definitely don't want to do this right now.

@rohany
Copy link
Contributor

rohany commented Mar 1, 2020

Sweet, both of those seem reasonable to me. code LGTM

@jordanlewis
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 2, 2020

Build succeeded

@craig craig bot merged commit 860c137 into cockroachdb:master Mar 2, 2020
@jordanlewis jordanlewis deleted the kvtrace-logictest branch March 2, 2020 00:55
craig bot pushed a commit that referenced this pull request Mar 3, 2020
45140: sql: use CommandResult for displaying ParameterStatus updates r=andreimatei a=otan

We were previously sending ParameterStatus using a listener. This
is refactored to talk using CommandResult instead, with results only
being buffered if "Close()" is called. To do this, a new
`AppendParamStatusUpdate` function to `RestrictedCommandResult`.

Since setting vars live in `sessionDataMutator`, we add a new
`paramStatusUpdater` interface which uses the `RestrictedCommandResult`.
This var is reset each time we execute with an openTxn with the
CommandResult used for it.

Also renamed "StatusParam" to "ParamStatus", which is closer to the name
"ParameterStatus".

Release note: None



45530: ui: network no connection r=dhartunian a=elkmaster

removed trigger when all the nodes are connected

Resolves: #45406

Release note (ui): none

45564: sql: inverted index fixes r=jordanlewis a=jordanlewis

This is based on #45563, and split out from #45157.

Closes #45154.
Closes #32468.

Miscellaneous fixes for inverted indexes.

1. Don't produce duplicate keys for inverted indexes.
2. Permit rows to not emit any keys for a particular index, which is required to allow not having any inverted index entries for NULL (or empty container).
3. Don't produce keys for NULL entries in inverted indexes

Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Vlad Los <[email protected]>
Co-authored-by: Jordan Lewis <[email protected]>
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