Copilot commented on code in PR #11239:
URL: https://github.com/apache/gravitino/pull/11239#discussion_r3304393587
##########
catalogs/catalog-jdbc-starrocks/src/main/java/org/apache/gravitino/catalog/starrocks/utils/StarRocksUtils.java:
##########
@@ -57,6 +57,14 @@ public class StarRocksUtils {
Pattern.compile(
"DISTRIBUTED
BY\\s+(HASH|RANDOM)\\s*(\\(([^)]+)\\))?\\s*(BUCKETS\\s+(\\d+))?");
+ // Match only top-level DISTRIBUTED BY clause boundaries to avoid false
positives
+ // from column comments. For strategy shape, RANDOM has no column list while
+ // others are expected to start with "strategy(".
+ private static final Pattern DISTRIBUTED_BY_CLAUSE_PATTERN =
+ Pattern.compile(
+ "(?:^|\\n|\\))\\s*DISTRIBUTED\\s+BY\\s+(?:RANDOM\\b|\\w+\\s*\\()",
Review Comment:
`DISTRIBUTED_BY_CLAUSE_PATTERN` is used to decide whether to fall back to
`Distributions.NONE` vs throwing. The current boundary `(?:^|\n|\))` can miss
real (but unparseable) `DISTRIBUTED BY` clauses when the SQL is formatted on a
single line or has tokens between `)` and `DISTRIBUTED` (e.g., `... )
ENGINE=OLAP DISTRIBUTED BY INVALID(...) ...`), causing an incorrect fallback to
`NONE` instead of the intended exception. Consider detecting `DISTRIBUTED BY`
outside of quoted strings/comments (e.g., strip/ignore quoted segments first)
or broaden the boundary handling (also handle `\r\n` line endings) so presence
detection is formatting-independent while still avoiding false positives from
column comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
