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

release-19.2: backport FD fixes #44604

Conversation

@RaduBerinde
Copy link
Member

RaduBerinde commented Jan 31, 2020

Backport:

  • 1/1 commits from "opt: remove lax constant functional dependencies" (#43532)
  • 1/1 commits from "opt: fix assertion failure due to lax empty key" (#43722)
  • 1/1 commits from "opt: improve functional dependency documentation" (#44360)
  • 1/1 commits from "opt: FD libary fixes and test infrastructure" (#44386)

Please see individual PRs for details.

/cc @cockroachdb/release

@RaduBerinde RaduBerinde requested a review from andy-kimball Jan 31, 2020
@RaduBerinde RaduBerinde requested a review from cockroachdb/sql-opt-prs as a code owner Jan 31, 2020
@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Jan 31, 2020

This change is Reviewable

Copy link
Contributor

andy-kimball left a comment

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball)

@rytaft
rytaft approved these changes Feb 1, 2020
Copy link
Contributor

rytaft left a comment

:lgtm:

Reviewed 14 of 14 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, 9 of 9 files at r4.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained

andy-kimball and others added 4 commits Dec 24, 2019
Before, the FuncDepSet allowed lax constant functional dependencies like:

  ()~~>(1,2)

These were columns that always had a constant value, or were NULL-valued.
However, this fails the definition for a lax dependency given in the
header comment in func_dep.go:

  (A(r1) = A(r2)) IS True ==> B(r1) NULL= B(r2)

While we know of know bugs caused by this discrepancy, it's safer to just
remove support for lax constants. It turns out that none of our test
scenarios require them; when removed, all transformations still work just
as they did.

Release note: None
PR #43532 removed the concept of lax constant functional dependencies.
There is a left-over case when we downgrade a key: if we had a strong
empty key, the result is a lax empty key which is no longer a concept.

This change fixes this by removing the key altogether in this case.

Fixes #43651.

Release note (bug fix): fixes "expected constant FD to be strict"
internal error.
Improve the descriptions of:

  1. Constant lax and strict functional dependencies.
  2. Lax keys.
  3. Empty keys.

Release note: None
This commits adds randomized testing for FuncDeps and fixes a set of
issues that were found.

The testing approach is described by this comment:
```
// TestFuncDepOpsRandom performs random FD operations and maintains a test
// relation in parallel, making sure that the FDs always hold w.r.t the test
// table. We start with a "full" table (all possible combinations of values in a
// certain range) and each operation filters out rows in conformance with the
// operation (e.g. if we add a key, we remove duplicate rows). We also test
// various FuncDepSet APIs at each stage.
```
When an error is found, the test presents the chain of operations
(which helps a lot in root causing the issue):
```
  initial numCols=3 valRange=3
   => MakeNotNull(2)
      FDs:
   => AddConstants(1,3) values {NULL,1}
      FDs: ()-->(1,3)
   => AddLaxKey(3)
      FDs: ()-->(1-3)

   < some error >
```

In addition, we also test joins (inner, left, full outer) by
generating two random chains and then joining them.

This change addresses a number of bugs found through this approach:

 - MakeNotNull was reading specific positions of the deps slice but it
   was also indirectly changing it at the same time.

 - various `Verify` assertion failures (e.g. due to not reducing a key
   after an operation).

 - it is incorrect to reduce the columns of a lax key: if a column is
   determined by other lax key columns, it could still be NULL and we
   could be removing the last NULL that allows for duplicates for a
   set of values. This was happening in many hard-to-find places,
   including in APIs like `ColsAreLaxKey` or `ProjectCols`. This is
   the root cause of #44296.

 - the outer join logic (which partially lived outside of the
   FuncDepSet code) had a bug. This logic is now fixed and moved
   inside a MakeFullOuter method which is covered by randomized
   testing.

Fixes #44296.

Release note (bug fix): fixed possibly incorrect query results in
various cornercases, especially when SELECT DISTINCT is used.
@RaduBerinde RaduBerinde force-pushed the RaduBerinde:backport19.2-43532-43722-44360-44386 branch from 490b509 to 7ade06e Feb 3, 2020
@RaduBerinde RaduBerinde merged commit 75ad022 into cockroachdb:release-19.2 Feb 3, 2020
2 checks passed
2 checks passed
GitHub CI (Cockroach) TeamCity build finished
Details
license/cla Contributor License Agreement is signed.
Details
@RaduBerinde RaduBerinde deleted the RaduBerinde:backport19.2-43532-43722-44360-44386 branch Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.