Fixes #3139: Plan Hash Instability in recursive union query #3142
Conversation
This updates the plan hashes for some of the plan operators associated with recursive CTEs. They previously hashed the quantifier directly, which could include things like enum hashes, which are not stable across JVMs. This resulted in multi-server test failures. This updates the plan hash calculations to be based off of the child _plans_ instead of the child _quantifiers_, which have stable plan hashes. To test this, I published this change to my maven local repo. I then configured the yaml tester to use that newly published library against my embedded driver, and I validated that the failing tests in `recursive-cte.yamsql` now passed. In order to make this actually mergeable, though, I needed to update the `supported_version` on those two tests to `!current_version`, which means we won't actually get coverage of this until the next release. We could consider adding additional configurations that compare embedded versus external on the same version, which would help cover things like this, but I didn't add that in this PR. I did update `recursive-cte.yamsql` with query plans, which made it clearer what plans to look at and felt like good things to have. I've checked in the now necessary metrics files as well for those plans. This fixes FoundationDB#3139.
| @@ -184,7 +184,7 @@ public int hashCodeWithoutChildren() { | |||
|
|
|||
| @Override | |||
| public int planHash(@Nonnull final PlanHashMode mode) { | |||
| return PlanHashable.objectsPlanHash(mode, BASE_HASH, getTempTableReferenceValue()); | |||
| return PlanHashable.objectsPlanHash(mode, BASE_HASH, getChild()); | |||
There was a problem hiding this comment.
This should actually use BOTH the reference value and the child. The reference value is safe (to be used for plan hashes; only quantifiers are not).
There was a problem hiding this comment.
I've added back the getTempTableReferenceValue() (and the Boolean isOwningTemplate, which I think was an oversight). I'd thought that that was also unstable, but further local tests seem to suggest I was wrong, so either I was comparing the wrong thing or I was comparing this code against hashing the quantifier by mistake. I've re-done the final test (pushing a build to maven local and running the multi-server tests with two JVMs on the latest code), and the plan hashes were stable.
|
This updates the plan hashes for some of the plan operators associated with recursive CTEs. They previously hashed the quantifier directly, which could include things like enum hashes, which are not stable across JVMs. This resulted in multi-server test failures. This updates the plan hash calculations to be based off of the child plans instead of the child quantifiers, which have stable plan hashes.
To test this, I published this change to my maven local repo. I then configured the yaml tester to use that newly published library against my embedded driver, and I validated that the failing tests in
recursive-cte.yamsqlnow passed. In order to make this actually mergeable, though, I needed to update thesupported_versionon those two tests to!current_version, which means we won't actually get coverage of this until the next release. We could consider adding additional configurations that compare embedded versus external on the same version, which would help cover things like this, but I didn't add that in this PR.I did update
recursive-cte.yamsqlwith query plans, which made it clearer what plans to look at and felt like good things to have. I've checked in the now necessary metrics files as well for those plans.This fixes #3139.