Skip to content

Conversation

@ccxxxyy
Copy link
Contributor

@ccxxxyy ccxxxyy commented Dec 31, 2025

for #31486 , for #31502
official document:

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

Copy link
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.

The direction of adding Doris ANALYZE/SHOW/CREATE grammar and tests is good—thanks for aligning the dialect support.

Blocking issues to fix before merge:

  • DorisAnalyzeTableStatement#getAttributes is inverted: with a table it returns empty attributes, and without a table it build TableSQLStatementAttribute/TableBroadcastRouteSQLStatementAttribute with a null table. This drops routing info for table-level ANALYZE and risks NPE/wrong broadcast for database-level ANALYZE. Please flip the logic to include table attributes when a table exists and return empty attributes otherwise.
  • The new DorisAnalyzeTableStatement isn’t bound in DALStatementBindEngine (only AnalyzeTableStatement is handled), so table binding/default schema completion won’t happen. Please wire it into the binder (reuse AnalyzeTableStatementBinder or a Doris variant) to ensure bound table segments for routing/rewriting.
  • SHOW DATA SKEW currently returns MySQLShowOtherStatement, losing table/partition context and effectively behaving as a generic SHOW. It should have a dedicated statement/segments that retain the table and partitions, with tests covering this.

If there are changes unrelated to this PR, please roll them back to keep the scope focused. The additions are valuable—once these issues are addressed, we can proceed.

@ccxxxyy
Copy link
Contributor Author

ccxxxyy commented Jan 1, 2026

@terrymanu Hi! I hope my revisions can meet the requirements. Hope you can review them when you have time.

@ccxxxyy ccxxxyy requested a review from terrymanu January 1, 2026 04:34
Copy link
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.

Good work extending Doris syntax with ANALYZE/SHOW DATA SKEW/CREATE WORKLOAD GROUP plus parser cases and assertions.

Blocking issues:

  • The SPI file for the Doris binder is broken. Path uses META-INF.services instead of META-INF/services, and the content is org.apache.shardingsphere.infra.binder.sqlserver.bind.DorisSQLBindEngine (wrong package/class). ServiceLoader won’t load the binder, so DorisAnalyzeTable binding never runs and default database/table context is missing. Please move the file to infra/binder/dialect/doris/src/main/resources/META-INF/services/org.apache.shardingsphere.infra.binder.engine.DialectSQLBindEngine and set the content to org.apache.shardingsphere.infra.binder.doris.bind.DorisSQLBindEngine.

Newly introduced problems:

  • Because the SPI isn’t picked up, the new binder logic is effectively dead code; routes/default database resolution can fail. Needs fixing before merge.

Suggestions:

  • After fixing the SPI, run a minimal parse+bind check to confirm DorisAnalyzeTable gets the expected table/database attributes.
  • Keep pushing once the binder is discoverable.

@ccxxxyy
Copy link
Contributor Author

ccxxxyy commented Jan 2, 2026

@terrymanu Hi! Thank you for your correction. I hope my revisions can meet the requirements. Hope you could take a look when you have a moment.

@ccxxxyy ccxxxyy requested a review from terrymanu January 2, 2026 03:44
Copy link
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.

Good direction overall—nice to see Doris SHOW DATA SKEW / CREATE WORKLOAD GROUP / ANALYZE support with AST/binder/test coverage added.

Blocking issues to fix:

  • COM_STMT_PREPARE regression: MySQLComStmtPrepareChecker still only whitelists AnalyzeTableStatement; Doris now produces DorisAnalyzeTableStatement, so prepared ANALYZE will be rejected. Please extend the whitelist to cover the new statement.
  • Attribute handling: DorisAnalyzeTableStatement#getAttributes returns empty attributes for ANALYZE DATABASE .... If we expect database-required or broadcast semantics there, please add the appropriate attributes; if not, a short rationale helps.

Please address the above and we’ll take another look.

@ccxxxyy
Copy link
Contributor Author

ccxxxyy commented Jan 4, 2026

@terrymanu Hi! I hope my revisions can meet the requirements. Hope you could take a look when you have a moment

@ccxxxyy ccxxxyy requested a review from terrymanu January 4, 2026 08:07
Copy link
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.

Great work on the Doris syntax parsing and assertions—looks solid.

One fix needed: you added infra/binder/dialect/doris but didn’t attach it to the proxy/jdbc dialect aggregator POMs, so the Doris binder SPI won’t ship. Please add the doris submodule to the proxy and jdbc dialect aggregator modules to ensure it’s included at runtime.

Looking forward to the update.

@ccxxxyy
Copy link
Contributor Author

ccxxxyy commented Jan 5, 2026

@terrymanu Hi! Hope you could review them when you have time. I hope my revisions can meet the requirements.

Copy link
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.

Good progress on covering Doris syntax (ANALYZE / SHOW DATA SKEW / CREATE WORKLOAD GROUP) and adding tests, and allowing DorisAnalyzeTableStatement in MySQL prepare. Thanks for pushing this.

One change request:

  • SHOW DATA SKEW currently has parsing/AST only; it skips a dialect binder. Since the doc allows omitting the database name, without SimpleTableSegmentBinder the default database and metadata/partition validation are missing, which can affect routing and error
    reporting. Please add a Doris-specific binder for DorisShowDataSkewStatement (similar to DorisAnalyzeTableStatementBinder) to bind the table and reuse metadata checks so TablesContext gets correct db/table info.

Please address the above and resubmit. Happy to take another look after the binder is added.

@ccxxxyy
Copy link
Contributor Author

ccxxxyy commented Jan 9, 2026

@terrymanu Hi! I hope my revisions can meet the requirements. Hope you could review them when you have a moment!

@ccxxxyy ccxxxyy requested a review from terrymanu January 10, 2026 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants