set skip and limit for the recursive union cursor correctly. #3111
Conversation
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/Matchers.java
Show resolved
Hide resolved
| union all | ||
| select b.id, b.parent from c1 as a, t1 as b where a.id = b.parent) select id from c1 | ||
| - maxRows: 1 | ||
| - result: [{ID: 1}] |
There was a problem hiding this comment.
there is an outstanding PR that does this stuff automatically for up/downlevel testing. So you may not need these tests in a week from now.
There was a problem hiding this comment.
Got it, I would leave it for now not to lose test coverage, and we can clean up later when we have a way to automatically test continuations in YAML.
| } | ||
| if (!expectedAsMultiSet.isEmpty()) { | ||
| return ImmutablePair.of(ResultSetMatchResult.fail("result does not contain all expected rows"), null); | ||
| return ImmutablePair.of(ResultSetMatchResult.fail(String.format("result does not contain all expected rows, expected %d row(s), got %d row(s) instead.", expectedRowCount, actualRowsCounter - 1)), null); |
There was a problem hiding this comment.
I understand that this is tests, but I think we want to refrain from String.format (especially without a locale) if we can avoid it. That does seem to be a larger problem in this file, though
There was a problem hiding this comment.
I made all the String.format calls here use Locale.ROOT. I will propose a separate PR that extends this fix to the entire codebase.
| select id, parent from t1 where parent = -1 | ||
| union all | ||
| select b.id, b.parent from c1 as a, t1 as b where a.id = b.parent) select id from c1 | ||
| - supported_version: !current_version |
There was a problem hiding this comment.
It looks like without this, some JDBC tests were failing as they were incorrectly trying to run continuations against 4.0.575.0. If I understand what this configuration means correctly, the query will be tested against current commit onwards only, which is the correct thing to do, considering that recursive CTE support of continuations was misbehaving before this PR. @ScottDugas can you please have a look?
There was a problem hiding this comment.
LGTM. I'm not sure we have great documentation of the stability of this feature, which is for all intents and purposes experimental. We should probably still call out that limit and skip behavior have changed in the release notes, potentially calling it out as a breaking change. We'll be updating to 4.1 fairly soon (see #3082), and I don't think we'd usually update the minor version on an experimental change like this, but it wouldn't be a bad idea to piggy back on that minor version bump in this case (which basically just means merging but not releasing until the other PR goes in)
|
Given the state of the PRs, we could even update the release notes on #3082 to avoid merge conflicts |
* Update relase workflow to accept artifact version This updates the release workflow so that it runs when someone manually triggers it rather than on every commit. It also takes an argument for the version, which hopefully will fix some tests that are failing during the current build. It still doesn't update the build to do any of the other things we associate with releases (e.g., updating any of the values storing the current version), but it may give us insight into the release build itself. * Add script to increment version automatically and hook it into the release * increment version during release * update patch release build to merge into main at the end * remove artifact version task * update workflows based on testing * Add configuration to publish to GitHub packages and to Maven Central This adds the gradle configuration necessary to publish to maven central and GitHub packages if requested. It also then calls those methods during the release step of our GitHub actions. * separate out FDB and env setup to avoid running setup twice * verison bump * some cleanup * update to 4.1 ; add documentation about maven central * update release notes for #3111 * remove compiler.xml which is no longer necessary (as stated by the .gitignore) but will now update with every release * update versionutils.py to make more internally consistent
This sets the
limitandskip(if any) on the recursive union cursor correctly. It also includes a number of small fixes to the YAML test framework error reporting in case of result mismatches, and a small optimization of handling unordered result matching.This fixes #3100.