Skip to content

distsql: improve test harness for processors against operators#60648

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
yuzefovich:proc-op-cmp
Feb 17, 2021
Merged

distsql: improve test harness for processors against operators#60648
craig[bot] merged 1 commit intocockroachdb:masterfrom
yuzefovich:proc-op-cmp

Conversation

@yuzefovich
Copy link
Copy Markdown
Member

Previously, when comparing the output of processors and operators, if
any order is allowed, we would perform a quadratic search. This is
suboptimal, and this commit improves the situation by sorting both
outputs first and then using the ordered comparison.

Additionally, this commit fixes a rare flake when seemingly the same
outputs would result in a mismatch. I could somewhat reliably reproduce
it before the patch and cannot reproduce it after the patch. It is
likely to do something with the float comparison, but I didn't get to
the bottom of the flake, yet I don't think it's worth it given that this
commit improves the situation anyway.

Fixes: #60608.

Release note: None

@yuzefovich yuzefovich requested review from a team and asubiotto February 17, 2021 01:17
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Contributor

@asubiotto asubiotto left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

Previously, when comparing the output of processors and operators, if
any order is allowed, we would perform a quadratic search. This is
suboptimal, and this commit improves the situation by sorting both
outputs first and then using the ordered comparison.

Additionally, this commit fixes a rare flake when seemingly the same
outputs would result in a mismatch. I could somewhat reliably reproduce
it before the patch and cannot reproduce it after the patch. It is
likely to do something with the float comparison, but I didn't get to
the bottom of the flake, yet I don't think it's worth it given that this
commit improves the situation anyway.

Release note: None
@yuzefovich
Copy link
Copy Markdown
Member Author

I needed to make a slight adjustment so that we sort only on colIdxsToCheckForEquality since we're only checking equality on those columns.

TFTR!

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Feb 17, 2021

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Feb 17, 2021

Build succeeded:

@craig craig bot merged commit 5744288 into cockroachdb:master Feb 17, 2021
@yuzefovich yuzefovich deleted the proc-op-cmp branch February 17, 2021 19:04
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.

sql/distsql: TestAggregatorAgainstProcessor failed

3 participants