Add a class for comparing versions of the record layer #3036

ScottDugas · 2025-01-13T19:35:52Z

This introduces a class for parsing and comparing versions of the Record Layer.
This can be combined with the work in #3031 to control what versions a yaml test will run against.

foundationdb-ci · 2025-01-13T20:32:56Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: db66999
Duration 0:56:52
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2025-01-14T21:18:56Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 74a17b8
Duration 0:54:25
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

ohadzeliger · 2025-01-14T20:31:42Z

yaml-tests/yaml-tests.gradle

@@ -55,6 +55,7 @@ dependencies {
    implementation(libs.asciitable)
    implementation(libs.jsr305)
    implementation(libs.junit.api)
+    implementation(libs.junit.params)


Ouch. I think this will conflict with a similar change I made.

ohadzeliger · 2025-01-14T20:35:33Z

...-tests/src/main/java/com/apple/foundationdb/relational/yamltests/server/SemanticVersion.java

+ *     Thus, this spec implements only slightly more than what the Record Layer needs, which is to say:
+ *     a sequence of positive numbers ({@code 0|[1-9]\\d*} separated by {@code .}s, and an optional {@code -SNAPSHOT}
+ *     at the end. Each version component is compared as integers, and versions with a different number of components
+ *     are not comparable. A version with {@code -SNAPSHOT} version added to the less than the same set of numbers


The last sentence is unclear.

...-tests/src/main/java/com/apple/foundationdb/relational/yamltests/server/SemanticVersion.java

ohadzeliger · 2025-01-14T21:09:10Z

...-tests/src/main/java/com/apple/foundationdb/relational/yamltests/server/SemanticVersion.java

+                    "(?:\\+(?<buildMetadata>" + dotSeparated(BUILD_PART) + "))?$");
+    private static final Pattern NUMERIC_PATTERN = Pattern.compile("\\d+");
+
+    private final List<Integer> versionNumbers;


Some explanation of the prerelease and metadata parts (with example) could be useful.

I added a comment about versionNumbers and ``prerelease`. Some of that was in the class javadoc.

I decided to remove buildMetadata. It's part of semver, but we don't support it.

ohadzeliger · 2025-01-14T21:17:45Z

...-tests/src/main/java/com/apple/foundationdb/relational/yamltests/server/SemanticVersion.java

+            return false;
+        }
+        final SemanticVersion that = (SemanticVersion)o;
+        return Objects.equals(versionNumbers, that.versionNumbers) && Objects.equals(prerelease, that.prerelease) && Objects.equals(buildMetadata, that.buildMetadata);


This is theoretical, but is there meaning to the order of prerelease strings?

Yes, I added some flavor text to the javadoc on prerelease that hopefully makes it clear that there is, but we don't really support it.

...-tests/src/main/java/com/apple/foundationdb/relational/yamltests/server/SemanticVersion.java

ohadzeliger · 2025-01-14T21:23:18Z

...-tests/src/main/java/com/apple/foundationdb/relational/yamltests/server/SemanticVersion.java

+                if (NUMERIC_PATTERN.matcher(otherPart).matches()) {
+                    int thisNumber = Integer.parseInt(thisPart);
+                    int otherNumber = Integer.parseInt(otherPart);
+                    if (thisNumber < otherNumber) {


It may add extra cost of boxing and unboxing, but maybe Objects.compare() here may be clearer?

int result = Objects.compare(thisNumber, otherNumber); if (result != 0) { result result; }

Since we only support SNAPSHOT or nothing, I removed this whole section, but I took your change and applied a variant of it to the version comparison code.

ohadzeliger · 2025-01-14T21:31:26Z

...ts/src/test/java/com/apple/foundationdb/relational/yamltests/server/SemanticVersionTest.java

+        final SemanticVersion versionB = SemanticVersion.parse(rawVersionB);
+        Assertions.assertAll(
+                () -> assertEquals(0, versionA.compareTo(versionB)),
+                () -> assertEquals(0, versionA.compareTo(versionB)));


Suggested change

() -> assertEquals(0, versionA.compareTo(versionB)));

() -> assertEquals(0, versionB.compareTo(versionA)));

ohadzeliger · 2025-01-14T21:37:21Z

...ts/src/test/java/com/apple/foundationdb/relational/yamltests/server/SemanticVersionTest.java

+    void alignsWithComparator(int versionA, int versionB) {
+        // Ensure that the signage aligns with the standard `compare` methods
+        // the value doesn't have to, but it does, and this is easier than asserting that they have the same sign
+        assertEquals(Integer.compare(versionA, versionB),


Can't we use signum()?

assertEquals( Integer.compare(a, b).signum(), SemanticVersion.parse(versionA + ".0.0").compareTo( SemanticVersion.parse(versionB + ".0.0")).signum();

ohadzeliger · 2025-01-14T21:38:26Z

...ts/src/test/java/com/apple/foundationdb/relational/yamltests/server/SemanticVersionTest.java

+                () -> assertThrows(IllegalArgumentException.class,
+                        () -> versionA.compareTo(versionB)),
+                () -> assertThrows(IllegalArgumentException.class,
+                        () -> versionA.compareTo(versionB)));


Suggested change

() -> versionA.compareTo(versionB)));

() -> versionB.compareTo(versionA)));

ohadzeliger · 2025-01-14T21:49:13Z

Maybe add some tests with parse(null) and compare to(null)?

@nonnull

This does: 1. expands / cleans up comments in `SemanticVersion` 2. Removes buildMetadata from the regex, and the field. 3. Removes comparison of prerelease with more than one component. We already don't support parsing multiple components. 4. Take advantage of `Integer.compare` and `Integer.signum` in code and tests 5. Fix the tests that were supposed to alternate the comparison order but I accidentally put the same order in twice. 6. Test for null arguments, and add @nonnull annotations

foundationdb-ci · 2025-01-15T18:40:48Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: d99ad0e
Duration 0:51:14
Result: ❌ FAILED
Error: Error while executing command: ./gradlew --no-daemon --console=plain -b ./build.gradle build destructiveTest -PcoreNotStrict -PreleaseBuild=false -PpublishBuild=false -PspotbugsEnableHtmlReport. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

@nonnull

This does: 1. expands / cleans up comments in `SemanticVersion` 2. Removes buildMetadata from the regex, and the field. 3. Removes comparison of prerelease with more than one component. We already don't support parsing multiple components. 4. Take advantage of `Integer.compare` and `Integer.signum` in code and tests 5. Fix the tests that were supposed to alternate the comparison order but I accidentally put the same order in twice. 6. Test for null arguments, and add @nonnull annotations

Conflicted on yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/server/package-info.java but it was just comments (years and javadoc)

foundationdb-ci · 2025-01-15T19:42:06Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: f291b97
Duration 0:53:27
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2025-01-15T20:08:18Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 8f79f46
Duration 0:53:35
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

...-tests/src/main/java/com/apple/foundationdb/relational/yamltests/server/SemanticVersion.java

ohadzeliger

Just one comment change

Co-authored-by: ohadzeliger <[email protected]>

foundationdb-ci · 2025-01-15T22:32:35Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 9d90c7c
Duration 0:50:52
Result: ❌ FAILED
Error: Error while executing command: ./gradlew --no-daemon --console=plain -b ./build.gradle build destructiveTest -PcoreNotStrict -PreleaseBuild=false -PpublishBuild=false -PspotbugsEnableHtmlReport. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

...-tests/src/main/java/com/apple/foundationdb/relational/yamltests/server/SemanticVersion.java

foundationdb-ci · 2025-01-16T03:03:08Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: fef2ded
Duration 0:54:00
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

ScottDugas requested a review from ohadzeliger January 13, 2025 19:35

Add a class for comparing versions of the record layer

74a17b8

ScottDugas force-pushed the semantic-versions branch from db66999 to 74a17b8 Compare January 14, 2025 20:24

ohadzeliger requested changes Jan 14, 2025

View reviewed changes

ScottDugas force-pushed the semantic-versions branch from d99ad0e to f291b97 Compare January 15, 2025 18:48

Merge branch 'main' into semantic-versions

8f79f46

Conflicted on yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/server/package-info.java but it was just comments (years and javadoc)

ohadzeliger reviewed Jan 15, 2025

View reviewed changes

...-tests/src/main/java/com/apple/foundationdb/relational/yamltests/server/SemanticVersion.java Outdated Show resolved Hide resolved

ohadzeliger requested changes Jan 15, 2025

View reviewed changes

Fix comment about comparing snapshot versions

9d90c7c

Co-authored-by: ohadzeliger <[email protected]>

ScottDugas mentioned this pull request Jan 15, 2025

yamsql: Add supported_version to blocks and queries, and support for actual versions #3049

Merged

ohadzeliger previously approved these changes Jan 15, 2025

View reviewed changes

ScottDugas commented Jan 16, 2025

View reviewed changes

...-tests/src/main/java/com/apple/foundationdb/relational/yamltests/server/SemanticVersion.java Outdated Show resolved Hide resolved

Add back missing } in SemanticVersion javadoc

fef2ded

ScottDugas dismissed ohadzeliger’s stale review via fef2ded January 16, 2025 02:09

ohadzeliger approved these changes Jan 16, 2025

View reviewed changes

ohadzeliger merged commit a7468b1 into FoundationDB:main Jan 16, 2025
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a class for comparing versions of the record layer #3036

Add a class for comparing versions of the record layer #3036

ScottDugas commented Jan 13, 2025

foundationdb-ci commented Jan 13, 2025

foundationdb-ci commented Jan 14, 2025

ohadzeliger Jan 14, 2025

ohadzeliger Jan 14, 2025

ohadzeliger Jan 14, 2025

ScottDugas Jan 15, 2025

ohadzeliger Jan 14, 2025

ScottDugas Jan 15, 2025

ohadzeliger Jan 14, 2025

ohadzeliger Jan 14, 2025

ScottDugas Jan 15, 2025

ohadzeliger Jan 14, 2025

ohadzeliger Jan 14, 2025

ohadzeliger Jan 14, 2025

ohadzeliger commented Jan 14, 2025

foundationdb-ci commented Jan 15, 2025

foundationdb-ci commented Jan 15, 2025

foundationdb-ci commented Jan 15, 2025

ohadzeliger left a comment

foundationdb-ci commented Jan 15, 2025

foundationdb-ci commented Jan 16, 2025

	() -> assertEquals(0, versionA.compareTo(versionB)));
	() -> assertEquals(0, versionB.compareTo(versionA)));

	() -> versionA.compareTo(versionB)));
	() -> versionB.compareTo(versionA)));

Add a class for comparing versions of the record layer #3036

Add a class for comparing versions of the record layer #3036

Conversation

ScottDugas commented Jan 13, 2025

foundationdb-ci commented Jan 13, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Jan 14, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohadzeliger commented Jan 14, 2025

foundationdb-ci commented Jan 15, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Jan 15, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Jan 15, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

ohadzeliger left a comment

Choose a reason for hiding this comment

foundationdb-ci commented Jan 15, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Jan 16, 2025

Result of fdb-record-layer-pr on Linux CentOS 7