[#3302][Sub-Task] StarRocks catalog Table/DB ops #7738

liunaijie · 2025-07-16T15:43:53Z

What changes were proposed in this pull request?

add StarRocks Catalog Implement

Why are the changes needed?

To support StarRocks Catalog.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By E2E test, the test is in another pr

#7792

liunaijie · 2025-07-21T09:33:25Z

Hi @yuqi1129 I finished the first version and has tested it in my local desktop. PTAL when you have time.
looking forward to your feedback.

yuqi1129 · 2025-07-22T02:02:50Z

Hi @yuqi1129 I finished the first version and has tested it in my local desktop. PTAL when you have time. looking forward to your feedback.

I see, and we will take the time to review it, Thanks.

yuqi1129 · 2025-07-22T12:20:40Z

@liunaijie
The PR for StarRocks catalog skeleton has been merged. Please update the code.

taylor12805 · 2025-07-23T10:58:55Z

...main/java/org/apache/gravitino/catalog/starrocks/operations/StarRocksDatabaseOperations.java

  }

  @Override
  protected boolean supportSchemaComment() {
-    return true;
+    return false;


Why not support schema comment since it can be added to PROPERTIES as in Doris

it can be added, but the SHOW CREATE DATABASE xxx can't return the properties. So I am set it to false

taylor12805 · 2025-07-23T11:28:13Z

...dbc-starrocks/src/main/java/org/apache/gravitino/catalog/starrocks/utils/StarRocksUtils.java

+
+  private static final Pattern DISTRIBUTION_INFO_PATTERN =
+      Pattern.compile(
+          "DISTRIBUTED BY\\s+(HASH|RANDOM)\\s*(\\(([^)]+)\\))?\\s*(BUCKETS\\s+(\\d+|AUTO))?");


StarRocks don't have auto keyword

+1, but it seems that he has modified it in StarRocksUtils#extractBucketNum.

StarRocks don't have auto keyword

Done, has deleted

yuqi1129 · 2025-07-22T12:33:18Z

...s/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/config/JdbcConfig.java

@@ -65,7 +66,7 @@ public class JdbcConfig extends Config {
          .doc("The password of the Jdbc connection")
          .version(ConfigConstants.VERSION_0_3_0)
          .stringConf()
-          .checkValue(StringUtils::isNotBlank, ConfigConstants.NOT_BLANK_ERROR_MSG)
+          .checkValue(Objects::nonNull, ConfigConstants.NOT_BLANK_ERROR_MSG)


Why do you change it?

This is because the StarRocks jdbc password is empty string. When I set the password to empty string in StarRocksContainer. This Config check is fail due to it's blank.

Due to this I am also update here [Config.java](https://github.com/apache/gravitino/pull/7738/files#diff-8918f298792aac4b08036dbde485791e97628dd67d399cb1d2dc0b7a6e7f4a55)

In here it will filter the empty config value. but retaining empty values may be necessary for some use cases.

When I set the password to empty string in StarRocksContainer. This Config check is fail due to it's blank.

Why do you set the password to an empty string? I believe an empty string is not common used in production environment.

By default the StarRocks root user's password is empty string.
I think this empty value check is not necessary, For example if I want use gravitino to connect my dev mysql. the password is empty string, Then it can't be connect.

yuqi1129 · 2025-07-23T12:36:04Z

...dbc-starrocks/src/main/java/org/apache/gravitino/catalog/starrocks/utils/StarRocksUtils.java

+
+  private static final Pattern DISTRIBUTION_INFO_PATTERN =
+      Pattern.compile(
+          "DISTRIBUTED BY\\s+(HASH|RANDOM)\\s*(\\(([^)]+)\\))?\\s*(BUCKETS\\s+(\\d+|AUTO))?");


+1, but it seems that he has modified it in StarRocksUtils#extractBucketNum.

liunaijie · 2025-07-23T12:58:18Z

.../src/test/java/org/apache/gravitino/catalog/doris/operation/TestDorisDatabaseOperations.java

@@ -33,7 +33,7 @@ public class TestDorisDatabaseOperations extends TestDoris {
  @Test
  public void testBaseOperationDatabase() {
    String databaseName = RandomNameUtils.genRandomName("it_db");
-    String comment = "comment";
+    String comment = "";


fault change, will revert

yuqi1129 · 2025-07-24T01:45:53Z

.../main/java/org/apache/gravitino/catalog/starrocks/converter/StarRocksExceptionConverter.java

+  static final int CODE_TABLE_EXISTS = 1050;
+  static final int CODE_DATABASE_NOT_EXISTS = 1008;
+  static final int CODE_UNKNOWN_DATABASE = 1049;
+  static final int CODE_UNKNOWN_DATABASE_2 = 5501;
+  static final int CODE_NO_SUCH_TABLE = 1051;
+  static final int CODE_NO_SUCH_TABLE_2 = 5502;
+  static final int CODE_UNAUTHORIZED = 1045;
+  static final int CODE_NO_SUCH_COLUMN = 1054;
+  static final int CODE_OTHER = 1105;
+  static final int CODE_DELETE_NON_EXISTING_PARTITION = 1507;
+  static final int CODE_PARTITION_ALREADY_EXISTS = 1517;


Please check the error code carefully; there are some differences.

For example, CODE_NO_SUCH_TABLE_2 does not exist in StarRocks

The CODE_UNKNOWN_DATABASE_2 , CODE_NO_SUCH_TABLE_2 is added when I run E2E test.
Because I find the error is not match with offical document.

yuqi1129 · 2025-07-24T01:48:45Z

.../main/java/org/apache/gravitino/catalog/starrocks/converter/StarRocksExceptionConverter.java

+  private static final Pattern DATABASE_NOT_EXISTS_PATTERN =
+      Pattern.compile(DATABASE_NOT_EXISTS_PATTERN_STRING);
+
+  private static final String UNKNOWN_DATABASE_PATTERN_STRING =


Please code check the error template.

Sorry, I didn't quite get that - could you rephrase?

I mean the error message may not be .*?detailMessage = Unknown database '.*?' if the database does not exists. I have briefly reviewed the message and noticed some minor differences between these two databases.

I see, will update

yuqi1129 · 2025-07-24T01:52:38Z

@liunaijie
Could you split this PR into several small ones, it hard to review such a big PR. You can split it into

table/data operation and UT
partition operations and UT
ITs

liunaijie · 2025-07-24T02:06:54Z

@liunaijie Could you split this PR into several small ones, it hard to review such a big PR. You can split it into

table/data operation and UT

partition operations and UT

ITs

sure

liunaijie · 2025-07-24T03:10:32Z

@liunaijie Could you split this PR into several small ones, it hard to review such a big PR. You can split it into

table/data operation and UT

partition operations and UT

ITs

Done. In this PR, I've only included code changes related to table/database operations. I'll submit separate PRs for the remaining commits.

yuqi1129 · 2025-07-24T03:12:11Z

@liunaijie Could you split this PR into several small ones, it hard to review such a big PR. You can split it into

table/data operation and UT

partition operations and UT

ITs

Done. In this PR, I've only included code changes related to table/database operations. I'll submit separate PRs for the remaining commits.

Great work!!!, I'm going to review it today.

yuqi1129 · 2025-07-24T08:28:59Z

...s/src/main/java/org/apache/gravitino/catalog/starrocks/converter/StarRocksTypeConverter.java

+  static final String ARRAY = "array";
+  static final String JSON = "json";
+  static final String MAP = "map";
+  static final String STRUCT = "struct";
+
+  static final String BITMAP = "bitmap";
+  static final String HLL = "hll";
+
+  static final String BIT = "BIT";


Are these types not yet supported?

Yes, those type are not supported for now.
Because JdbcTypeBean doesn't have element type

yuqi1129 · 2025-07-24T08:34:06Z

...rc/main/java/org/apache/gravitino/catalog/starrocks/operations/StarRocksTableOperations.java

+    List<TableChange.SetProperty> setProperties = new ArrayList<>();
+    for (int i = 0; i < changes.length; i++) {
+      TableChange change = changes[i];
+      if (change instanceof TableChange.AddColumn) {


Is SetProperty not supported for StarRocks?

It have limited support. I miss some code.

StarRocks only support alert those properties, and it recommand only update 1 properties by 1 time.
Do we need check by gravitino or just send to starrocks and let starrocks throw exception.

If we implement either a whitelist or property size validation here, the code will require corresponding updates with each StarRocks release

If we implement either a whitelist or property size validation here, the code will require corresponding updates with each StarRocks release

We can only support 3.3.x here and add some comments about limited support for other versions.

do we need check by gravitino or just send to starrocks and let starrocks throw exception

I believe this is reasonable, and that is the point where it differs from Doris.

yuqi1129 · 2025-07-25T06:25:55Z

It's generally LGTM, @taylor12805
Do you have any further comments?

taylor12805 · 2025-07-26T13:07:07Z

It's generally LGTM, @taylor12805 Do you have any further comments?

LGTM

yuqi1129 · 2025-07-28T01:46:42Z

@mchades
Would you like to take a look?

mchades · 2025-07-28T01:55:17Z

@mchades Would you like to take a look?

you guys can go ahead

yuqi1129 · 2025-07-28T11:33:22Z

@liunaijie
Thanks for you contribuiton. You are welcome to proceed. I believe the rest of the work would be

Partition operations
ITs
Related docs

It seems that there is a lack of PR regarding the documents.

### What changes were proposed in this pull request? add StarRocks Catalog Implement ### Why are the changes needed? To support StarRocks Catalog. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? By E2E test, the test is in another pr apache#7792

### What changes were proposed in this pull request? add StarRocks Catalog Implement ### Why are the changes needed? To support StarRocks Catalog. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? By E2E test, the test is in another pr #7792

liunaijie force-pushed the feature/catalog_sr branch from 1e348dc to 7dd5eff Compare July 21, 2025 09:28

liunaijie changed the title ~~[WIP] [3302] Feature: Supports StarRocks catalog~~ [#3302] Feature: Supports StarRocks catalog Jul 21, 2025

liunaijie marked this pull request as ready for review July 21, 2025 09:28

liunaijie force-pushed the feature/catalog_sr branch from 7dd5eff to 66179d7 Compare July 21, 2025 09:29

jerryshao mentioned this pull request Jul 22, 2025

[#7667] feat(starrocks-catalog): Add StarRocks catalog skeleton. #7671

Merged

liunaijie force-pushed the feature/catalog_sr branch from b0cc82f to ebceb59 Compare July 22, 2025 13:33

liunaijie mentioned this pull request Jul 23, 2025

[#7675] feat(startrocks-catalog): Add Implementation for StarRocks catalog #7783

Closed

taylor12805 reviewed Jul 23, 2025

View reviewed changes

yuqi1129 reviewed Jul 23, 2025

View reviewed changes

liunaijie commented Jul 23, 2025

View reviewed changes

yuqi1129 reviewed Jul 24, 2025

View reviewed changes

liunaijie force-pushed the feature/catalog_sr branch from 6ecd2c4 to a1846e0 Compare July 24, 2025 03:04

liunaijie changed the title ~~[#3302] Feature: Supports StarRocks catalog~~ [#3302][Sub-Task] StarRocks catalog Table/DB ops Jul 24, 2025

yuqi1129 reviewed Jul 24, 2025

View reviewed changes

[apache#3302] feat(Catalog): StarRocks Catalog Table ops

d9a2e72

liunaijie force-pushed the feature/catalog_sr branch from a1846e0 to d9a2e72 Compare July 25, 2025 02:36

yuqi1129 assigned liunaijie Jul 28, 2025

yuqi1129 approved these changes Jul 28, 2025

View reviewed changes

yuqi1129 merged commit 71936c8 into apache:main Jul 28, 2025
30 checks passed

yuqi1129 added the 1.0.0 Release v1.0.0 label Jul 28, 2025

[#3302][Sub-Task] StarRocks catalog Table/DB ops #7738

[#3302][Sub-Task] StarRocks catalog Table/DB ops #7738

Uh oh!

Conversation

liunaijie commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

liunaijie commented Jul 21, 2025

Uh oh!

yuqi1129 commented Jul 22, 2025

Uh oh!

yuqi1129 commented Jul 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuqi1129 commented Jul 24, 2025

Uh oh!

liunaijie commented Jul 24, 2025

Uh oh!

liunaijie commented Jul 24, 2025

Uh oh!

yuqi1129 commented Jul 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liunaijie Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuqi1129 commented Jul 25, 2025

Uh oh!

taylor12805 commented Jul 26, 2025

liunaijie commented Jul 16, 2025 •

edited

Loading

liunaijie Jul 24, 2025 •

edited

Loading