Skip to content

Support for the ALTER RESOURCE statement#37392

Merged
terrymanu merged 3 commits into
apache:masterfrom
ccxxxyy:issueDoris
Dec 16, 2025
Merged

Support for the ALTER RESOURCE statement#37392
terrymanu merged 3 commits into
apache:masterfrom
ccxxxyy:issueDoris

Conversation

@ccxxxyy
Copy link
Copy Markdown
Contributor

@ccxxxyy ccxxxyy commented Dec 15, 2025

for #31509

Changes proposed in this pull request:


Before committing this PR, I'm sure that I have checked the following options:

  • My code follows the code of conduct of this project.
  • I have self-reviewed the commit code.
  • I have (or in comment I request) added corresponding labels for the pull request.
  • I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
  • I have made corresponding changes to the documentation.
  • I have added corresponding unit tests for my changes.
  • I have updated the Release Notes of the current development version. For more details, see Update Release Note

@terrymanu terrymanu requested a review from Copilot December 15, 2025 17:08
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Member

@terrymanu terrymanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Root Cause

  • DorisAlterResourceStatement redeclares databaseType, which already exists in the parent SQLStatement, creating duplicate state and potential divergence.
  • Assertions only check the resource name and non-null properties, never validating property key/value parsing, so visitor errors can slip through.
  • Grammar and visitor accept only string resource names and property keys/values; Doris syntax allows identifier-style keys and values beyond strings (official reference: https://doris.apache.org/docs/2.1/sql-manual/sql-statements/cluster-management/compute-management/ALTER-RESOURCE/).

Analysis

  • Redundant field: the parent already holds databaseType; keeping another copy in the subclass risks inconsistency on future changes/serialization.
  • Assertion gap: DorisAlterResourceStatementAssert doesn’t verify property key/values, so misparsed or missing properties still pass.
  • Syntax/visitor coverage: official syntax shows ALTER RESOURCE '<resource_name>' PROPERTIES(\key = value, ...), where the key is an identifier (backticked) and the value is not restricted to strings. The current rule using only string_ would reject valid statements with unquoted identifiers or non-string values.

Conclusion (Change Request)

  • Remove the duplicate databaseType field and assignment in DorisAlterResourceStatement; rely on the parent field. Keep constructor calling super(databaseType) and storing only resourceName and properties:
  public final class DorisAlterResourceStatement extends DALStatement {
      private final String resourceName;
      private final Properties properties;

      public DorisAlterResourceStatement(final DatabaseType databaseType, final String resourceName, final Properties properties) {
          super(databaseType);
          this.resourceName = resourceName;
          this.properties = properties;
      }
  }
  • Strengthen assertions: add expected property key/values in test cases (e.g., list or map) and validate size plus specific key/values in DorisAlterResourceStatementAssert to ensure visitor parsing is correct.
  • Align grammar and visitor with the official doc: allow resource names/property keys as identifiers or strings, and property values as literals (string/number/boolean). Update the visitor to handle IdentifierValue, StringLiteralValue, NumberLiteralValue, and booleans accordingly, and add SQL cases plus assertions covering these forms. After these adjustments, merging will be safer.

@ccxxxyy ccxxxyy requested a review from terrymanu December 16, 2025 03:54
@ccxxxyy
Copy link
Copy Markdown
Contributor Author

ccxxxyy commented Dec 16, 2025

@terrymanu Hi, I have made the revisions as required. Hope you can review them when you have some time.

Copy link
Copy Markdown
Member

@terrymanu terrymanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here’s what needs to be fixed before we can merge—thanks for the solid start, please keep going:

  • parser/sql/engine/dialect/doris/src/main/java/org/apache/shardingsphere/sql/parser/engine/doris/visitor/statement/type/DorisDALStatementVisitor.java:909: getPropertyValue only handles string/number literals; boolean/NULL/hex/etc. fall through to result.toString(), so
    those property values are parsed as object ids, not the literal values. Please decode all LiteralValue types (BooleanLiteralValue, NullLiteralValue, hex/bit, etc.) via getValue(), mirroring other dialect handlers.
  • Tests: test/it/parser/src/main/resources/case/dal/alter.xml and .../sql/supported/dal/alter.xml only cover string/number properties. Add cases for boolean/NULL (and other literal forms) to exercise the fixed paths.

Once these are addressed, the PR should be in good shape to re-review.

@ccxxxyy
Copy link
Copy Markdown
Contributor Author

ccxxxyy commented Dec 16, 2025

@terrymanu Hi, Hope you could take a look when you have a moment!

@ccxxxyy ccxxxyy requested a review from terrymanu December 16, 2025 05:27
Copy link
Copy Markdown
Member

@terrymanu terrymanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on the comprehensive coverage and tidy wiring—thanks for the thorough contribution!

@terrymanu terrymanu merged commit ffa1b59 into apache:master Dec 16, 2025
145 checks passed
@terrymanu terrymanu added this to the 5.5.3 milestone Dec 16, 2025
@ccxxxyy ccxxxyy deleted the issueDoris branch December 16, 2025 05:48
@ccxxxyy
Copy link
Copy Markdown
Contributor Author

ccxxxyy commented Dec 16, 2025

Nice work on the comprehensive coverage and tidy wiring—thanks for the thorough contribution!

It's my pleasure!

makssent pushed a commit to makssent/shardingsphere that referenced this pull request Apr 9, 2026
…ngsphere/master

* remotes/origin/master:
  Support for the ALTER RESOURCE statement (apache#37392)
  Update RELEASE-NOTES.md (apache#37400)
  Add CDCBackendHandlerTest (apache#37399)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants