feat: add StarRocks as a compatibility-tested database#317
Open
driessamyn wants to merge 1 commit into
Open
Conversation
|
📝 WalkthroughWalkthroughAdds StarRocks support: expands CI compatibility matrix, adds StarRocks testcontainer and DB-specific table DDL, updates DB flavour detection to treat StarRocks as MySQL, and conditionally skips tests incompatible with StarRocks upsert/PRIMARY KEY semantics. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions (matrix job)
participant TC as Test Runner (Gradle/integrationTest)
participant C as Testcontainers Manager
participant SR as StarRocks Container
participant JDBC as JDBC Connection
GH->>TC: run ./gradlew integrationTest -Ddb=STARROCKS
TC->>C: start StarRocks GenericContainer
C->>SR: launch container (port 9030)
C->>SR: execute "SHOW BACKENDS" (poll)
SR-->>C: backend status (Alive)
C->>JDBC: create database `test`
TC->>JDBC: open JDBC connection to `test` db
TC->>JDBC: run integration tests (DDL, queries)
TC->>GH: upload artifact (STARROCKS-compat-test-results)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/src/integrationTest/kotlin/net/samyn/kapper/AbstractDbTests.kt (1)
62-62: Consider pinning the StarRocks container image version.Using
starrocks/allin1-ubuntu:latestmakes builds non-reproducible. A breaking change in a new StarRocks release could cause CI failures that are difficult to diagnose. Consider pinning to a specific version (e.g.,starrocks/allin1-ubuntu:3.2.4) similar to how other containers in this file use explicit versions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/integrationTest/kotlin/net/samyn/kapper/AbstractDbTests.kt` at line 62, Replace the floating StarRocks image tag used in the GenericContainer constructor to a pinned version to make integration tests reproducible; locate the GenericContainer("starrocks/allin1-ubuntu:latest") instantiation and change the image string to a specific release (for example "starrocks/allin1-ubuntu:3.2.4" or another tested version) so the container image is versioned consistently with other containers in this test suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/integrationTest/kotlin/net/samyn/kapper/AbstractDbTests.kt`:
- Around line 98-119: The polling loop in AbstractDbTests.kt leaks a ResultSet
from stmt.executeQuery("SHOW BACKENDS") inside the STARROCKS block; ensure the
ResultSet is closed by using its closeable semantics (e.g., call use on the
ResultSet returned by executeQuery or explicitly close it in a finally) within
the existing bootstrapConn.createStatement().use { ... } so the ResultSet is
closed each iteration after checking rs.next() and rs.getBoolean("Alive").
In `@README.md`:
- Line 25: Update the README badge markdown: remove the nonexistent logo
parameter from the StarRocks shields.io badge URL in the image line (the
markdown starting with
"")
by deleting "&logo=starrocks" so the URL becomes the badge without the logo
parameter (leaving "&logoColor=white" if desired).
---
Nitpick comments:
In `@core/src/integrationTest/kotlin/net/samyn/kapper/AbstractDbTests.kt`:
- Line 62: Replace the floating StarRocks image tag used in the GenericContainer
constructor to a pinned version to make integration tests reproducible; locate
the GenericContainer("starrocks/allin1-ubuntu:latest") instantiation and change
the image string to a specific release (for example
"starrocks/allin1-ubuntu:3.2.4" or another tested version) so the container
image is versioned consistently with other containers in this test suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d60ddcd-0945-40fb-8517-bdb71dd99375
📒 Files selected for processing (8)
.github/workflows/build-and-test.ymlREADME.mdcore/src/integrationTest/kotlin/net/samyn/kapper/AbstractDbTests.ktcore/src/integrationTest/kotlin/net/samyn/kapper/ExecuteDtoTests.ktcore/src/integrationTest/kotlin/net/samyn/kapper/ExecuteTests.ktcore/src/integrationTest/kotlin/net/samyn/kapper/TypesTest.ktcore/src/main/kotlin/net/samyn/kapper/internal/DbFlavourFunc.ktcore/src/test/kotlin/net/samyn/kapper/internal/DbFlavourTest.kt
driessamyn
force-pushed
the
feature/add-starrocks-compat
branch
from
b5bfb1b to
4d7ef5d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/integrationTest/kotlin/net/samyn/kapper/TypesTest.kt (1)
59-62: Consider extracting the StarRocks check to a reusable helper.The
System.getProperty("db", "").trim().uppercase() == "STARROCKS"pattern is duplicated across multiple test files. Extracting this to a shared utility (e.g.,isStarRocks()inAbstractDbTests) would improve maintainability and reduce repetition.♻️ Suggested helper in AbstractDbTests
protected fun isStarRocks(): Boolean = System.getProperty("db", "").trim().uppercase() == "STARROCKS"Then in tests:
-assumeFalse( - System.getProperty("db", "").trim().uppercase() == "STARROCKS", - "StarRocks does not support all SQL types tested here", -) +assumeFalse(isStarRocks(), "StarRocks does not support all SQL types tested here")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/integrationTest/kotlin/net/samyn/kapper/TypesTest.kt` around lines 59 - 62, Extract the repeated StarRocks check into a protected helper on AbstractDbTests and replace inline usages in tests (like the assumeFalse call in TypesTest) with that helper; add a method named isStarRocks() on AbstractDbTests that returns the boolean from System.getProperty("db", "").trim().uppercase() == "STARROCKS", then change the assumeFalse call in TypesTest (and other tests) to assumeFalse(isStarRocks(), "StarRocks does not support all SQL types tested here").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/src/integrationTest/kotlin/net/samyn/kapper/TypesTest.kt`:
- Around line 59-62: Extract the repeated StarRocks check into a protected
helper on AbstractDbTests and replace inline usages in tests (like the
assumeFalse call in TypesTest) with that helper; add a method named
isStarRocks() on AbstractDbTests that returns the boolean from
System.getProperty("db", "").trim().uppercase() == "STARROCKS", then change the
assumeFalse call in TypesTest (and other tests) to assumeFalse(isStarRocks(),
"StarRocks does not support all SQL types tested here").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94b6a54b-d363-4a08-8bcd-22f255d2dc16
📒 Files selected for processing (8)
.github/workflows/build-and-test.ymlREADME.mdcore/src/integrationTest/kotlin/net/samyn/kapper/AbstractDbTests.ktcore/src/integrationTest/kotlin/net/samyn/kapper/ExecuteDtoTests.ktcore/src/integrationTest/kotlin/net/samyn/kapper/ExecuteTests.ktcore/src/integrationTest/kotlin/net/samyn/kapper/TypesTest.ktcore/src/main/kotlin/net/samyn/kapper/internal/DbFlavourFunc.ktcore/src/test/kotlin/net/samyn/kapper/internal/DbFlavourTest.kt
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- core/src/main/kotlin/net/samyn/kapper/internal/DbFlavourFunc.kt
- core/src/test/kotlin/net/samyn/kapper/internal/DbFlavourTest.kt
- .github/workflows/build-and-test.yml
- core/src/integrationTest/kotlin/net/samyn/kapper/AbstractDbTests.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
starrocks/allin1-ubuntu:latest, polls for backend readinessTest plan
-Ddb=STARROCKSSummary by CodeRabbit
New Features
Tests
Documentation