morningman commented on code in PR #67471:
URL: https://github.com/apache/doris/pull/67471#discussion_r3923420031
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -7268,6 +7269,13 @@ public LogicalPlan
visitAlterCatalogComment(AlterCatalogCommentContext ctx) {
return new AlterCatalogCommentCommand(catalogName, comment);
}
+ @Override
+ public LogicalPlan
visitAlterStreamComment(DorisParser.AlterStreamCommentContext ctx) {
+ TableNameInfo streamName = new
TableNameInfo(visitMultipartIdentifier(ctx.name));
+ String comment = stripQuotes(ctx.comment.getText());
Review Comment:
Fixed in 8359bb5e7a3, with one half deliberately left out.
`visitAlterStreamComment` now uses `SqlLiteralUtils.parseStringLiteral`.
`visitCreateStream` turned out to have the mirror image of the same bug, so it
was changed too: it decoded backslash escapes through
`LogicalPlanBuilderAssistant.escapeBackSlash` but never collapsed doubled
quotes and ignored `NO_BACKSLASH_ESCAPES`. Before this commit the same literal
therefore produced two different comments depending on whether CREATE or ALTER
stored it. Both paths now decode identically and honour the sql mode the lexer
is already driven by (`NereidsParser` feeds
`SqlModeHelper.hasNoBackSlashEscapes()` into the lexer predicate).
Covered by the new
`AlterTableStreamCommentTest#testAlterStreamCommentStringLiteral`: doubled
single quotes, doubled double quotes, `\n` / `\t`, and `NO_BACKSLASH_ESCAPES`.
The regression suite additionally checks the doubled-quote case end to end
through `information_schema.table_streams` (stored value `a'b`, length 3).
Not doing the SHOW CREATE round-trip half here. That is
`Env.addTableComment`
(`fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java:7528`): it wraps
the value in single quotes but escapes only double quotes, via
`SqlUtils.escapeQuota`
(`fe/fe-common/src/main/java/org/apache/doris/common/util/SqlUtils.java:50`). A
comment containing `'` already makes plain `SHOW CREATE TABLE` emit
non-parsable DDL today. The function has 19 call sites covering every table
type, so this is pre-existing and not stream specific, and fixing it changes
`SHOW CREATE` output for all tables. I will file it separately rather than fold
it into this PR.
##########
regression-test/suites/table_stream_p0/test_table_stream_alter_comment.groovy:
##########
@@ -0,0 +1,94 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// ALTER STREAM ... SET/MODIFY COMMENT updates the comment of a table stream.
+suite("test_table_stream_alter_comment") {
+ if (isCloudMode()) {
+ logger.info("skip test_table_stream_alter_comment in cloud mode")
+ return
+ }
+
+ def baseTable = "test_stream_alter_comment_base"
Review Comment:
Fixed in 8359bb5e7a3. Both suites now hardcode their table and stream names,
the deterministic `information_schema.table_streams` results are generated
`order_qt_*` output
(`regression-test/data/table_stream_p0/test_table_stream_alter_comment.out`,
produced with `-genOut` against a local cluster and then verified by a second
run without it), and the trailing DROPs are gone, leaving only the pre-test
cleanup.
Two things kept as they were. The two `SHOW CREATE STREAM` checks stay
substring probes instead of qt output, because the full DDL carries a
PROPERTIES block that changes as the stream feature evolves. And
`test_binlog_property_alter_exception` has no deterministic query output to
capture, so it only received the naming and fixture-retention fixes.
Converting the suites surfaced a separate defect worth flagging: the skip
guard of `test_binlog_property_alter_exception` compared the config name
against `enable_feature_binlog`, but `SHOW FRONTEND CONFIG` reports that
EXPERIMENTAL config as `experimental_enable_feature_binlog`, so the suite was
skipping itself unconditionally, in CI as well. The coverage this PR claimed to
add was therefore dead. It now uses the framework helper
`getSyncer().checkEnableFeatureBinlog()`, and the run log shows all six
expected error messages actually firing.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
