[feat](stream) Support ALTER STREAM ... SET/MODIFY COMMENT - #67471
Conversation
A table stream can be created with a comment (`CREATE STREAM ... COMMENT 'x'`), the comment is persisted, shown by `SHOW CREATE STREAM` and exposed as `information_schema.table_streams.STREAM_COMMENT`, but there was no way to change it: the grammar had no `ALTER STREAM` rule at all, so the statement failed with `no viable alternative at input 'ALTER STREAM'`, and `ALTER TABLE` rejects streams with `Do not support alter STREAM table[...]`. This adds `ALTER STREAM <name> SET COMMENT '...'`, accepting `MODIFY` as well so the syntax stays consistent with `ALTER TABLE ... MODIFY COMMENT`. The comment of a stream lives in the `Table` metadata only, so the new `Alter.processAlterStreamComment()` reuses `ModifyCommentOperationLog` and the existing generic replay path in `Alter.replayModifyComment()`. No new edit log operation and no meta version bump are needed. Cloud Meta Service stores only stream offsets and ids, so no extra RPC is needed either. Also adds a regression test for the immutable binlog table properties (`binlog.format`, `binlog.need_historical_value`, `binlog.enable`) on a ROW binlog table. That path had no coverage; it used to report a misleading light-schema-change error and was fixed as a side effect of apache#65810. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xx7TgjXJCiChnzLYa6hgtL
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: two issues are called out inline.
Goal and correctness: Plain SET/MODIFY COMMENT works, but valid escaped or doubled-quote literals are stored incorrectly and SHOW CREATE is not reliably round-trippable, so the feature is not correct for the full grammar.
Scope and parallel paths: Grammar, builder, PlanType, visitor, command dispatch, SET/MODIFY unification, and wrong-object handling are otherwise focused and connected. The separate binlog suite intentionally locks previously fixed behavior described in the PR.
Concurrency and lifecycle: No new threads or special lifecycle were added. Stream write locking plus dropped-object fencing preserves DROP ordering; no deadlock or lock-order issue was found.
Configuration, compatibility, persistence, and cloud: No new configuration was added, and the existing feature gate is enforced. Reusing OP_MODIFY_COMMENT is rolling-compatible; follower replay, inherited image serialization, cloud FE metadata ownership, and database-binlog handling are subtype-safe.
Tests: Basic positive, empty-comment, wrong-object, not-found, and immutable-binlog cases pass in current FE unit-test/P0 CI. Escaped literals, SQL modes, and SHOW round trips are missing, and the Groovy tests violate required generated-oracle and fixture-retention conventions. Replay and cloud are not directly exercised, though their production paths were verified.
Observability, transactions, protocol, and performance: This is metadata-only O(1) work with existing edit-log observability; it adds no data transaction, FE-BE protocol, or performance concern.
User focus: No additional focus was supplied.
Review completeness: Two full review rounds completed, and every Round 2 reviewer returned NO_NEW_VALUABLE_FINDINGS. Every candidate was accepted, duplicate-fenced, or dismissed with evidence. Reviewed exact head eabbe52.
| @Override | ||
| public LogicalPlan visitAlterStreamComment(DorisParser.AlterStreamCommentContext ctx) { | ||
| TableNameInfo streamName = new TableNameInfo(visitMultipartIdentifier(ctx.name)); | ||
| String comment = stripQuotes(ctx.comment.getText()); |
There was a problem hiding this comment.
Please decode the token as a SQL literal here. stripQuotes only removes delimiters, so a legal ALTER STREAM s SET COMMENT 'a''b' stores a''b instead of a'b; with default SQL mode, COMMENT '\n' stores a backslash plus n instead of a newline. information_schema.table_streams then exposes the wrong metadata, and SHOW CREATE STREAM can recreate a different value because the CREATE path unescapes it. Use the mode-aware SqlLiteralUtils.parseStringLiteral decoder, make the SHOW/CREATE quoting path round-trip the decoded value, and cover doubled quotes and backslash escapes including NO_BACKSLASH_ESCAPES.
There was a problem hiding this comment.
Fixed in 8359bb5, 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.
| return | ||
| } | ||
|
|
||
| def baseTable = "test_stream_alter_comment_base" |
There was a problem hiding this comment.
These new regression suites do not follow the repository's required fixture and oracle conventions: ordinary names are indirected through def, deterministic SHOW and metadata values use ad-hoc assertions instead of generated qt output, and both suites drop fixtures again at the end. Please hardcode the simple names, use generated ordered query-test output for deterministic results, and keep only the pre-test cleanup so failures leave state available for debugging.
There was a problem hiding this comment.
Fixed in 8359bb5. 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.
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: close apache#65388, related apache#65418 Related PR: apache#65810 Problem Summary: Review follow-up for this PR. Two problems were found in the first revision. 1. `visitAlterStreamComment` decoded the comment with `stripQuotes`, which only removes the outer delimiters. `ALTER STREAM s SET COMMENT 'a''b'` therefore stored `a''b` instead of `a'b`, and with the default sql mode `COMMENT '\n'` stored a backslash plus `n` instead of a newline. The value is exposed by `information_schema.table_streams.STREAM_COMMENT` and printed by `SHOW CREATE STREAM`, so the stored metadata was simply wrong. `visitCreateStream` had the other half of the problem: it decoded backslash escapes (`LogicalPlanBuilderAssistant.escapeBackSlash`) but did not collapse doubled quotes and ignored `NO_BACKSLASH_ESCAPES`, so the same literal produced different comments depending on whether it went through CREATE or ALTER. Both now use `SqlLiteralUtils.parseStringLiteral`, which collapses doubled quotes and honours the sql mode exactly like the lexer does (`NereidsParser` feeds `SqlModeHelper.hasNoBackSlashEscapes()` to the lexer). Not addressed here: `Env.addTableComment` wraps the comment in single quotes but only escapes double quotes (`SqlUtils.escapeQuota`), so a comment holding a single quote makes `SHOW CREATE` emit non-parsable DDL. That is pre-existing, shared by all 19 call sites of every table type, and is not stream specific. 2. The two new regression suites did not follow the repository test standards: names were indirected through `def`, deterministic results were asserted by hand instead of a generated `.out`, and both dropped their fixtures at the end. Fixed all three. While converting, the skip guard of `test_binlog_property_alter_exception` turned out to be dead: it compared the config name against `enable_feature_binlog`, but `SHOW FRONTEND CONFIG` reports the EXPERIMENTAL config as `experimental_enable_feature_binlog`, so the suite skipped itself unconditionally, in CI as well. It now uses the framework helper `getSyncer().checkEnableFeatureBinlog()`, and the suite actually runs. ### Release note None (the behaviour change of this PR is described in the first commit). ### Check List (For Author) - Test: - Unit Test: `AlterTableStreamCommentTest#testAlterStreamCommentStringLiteral` covers doubled single quotes, doubled double quotes, `\n`/`\t` and `NO_BACKSLASH_ESCAPES`. Both tests of the class pass locally (`Tests run: 2, Failures: 0, Errors: 0`). - Regression test: `test_table_stream_alter_comment` (the `.out` was generated with `-genOut` against a local cluster, then verified by a second run without it) and `test_binlog_property_alter_exception`, both pass locally and the six expected error messages were observed in the run log. - Behavior changed: Yes. A stream comment containing a doubled quote or a backslash escape is now stored decoded, and the sql mode is honoured. - Does this need documentation: No (the statement itself is documented by the first commit's doc PR). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xx7TgjXJCiChnzLYa6hgtL
|
run buildall |
Address review comment: drop the `forSetComment` static factory and make the constructor public, so the parser builds the command directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M2uex5qag4kC5G49J7Bm5t
|
run buildall |
TPC-H: Total hot run time: 16862 ms |
TPC-DS: Total hot run time: 82478 ms |
ClickBench: Total hot run time: 14.67 s |
FE Regression Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
4 participants
What problem does this PR solve?
Issue Number: close #65388, related #65418
Related PR: #65810
Problem Summary:
1.
ALTER STREAM ... SET COMMENTwas not supported (#65388)A table stream can be created with a comment and the comment is fully wired up everywhere except for changing it:
CREATE STREAM s ON TABLE t COMMENT 'x'InternalCatalog#createTableStream)Table#comment)SHOW CREATE STREAM sinformation_schema.table_streams.STREAM_COMMENTThere was no
ALTER STREAMrule inDorisParser.g4at all —STREAMonly appeared inCREATE STREAM,DROP STREAM,SHOW STREAMSandSHOW CREATE STREAM— so the statement failed at parser stage:ALTER TABLEis not an alternative either:Alter#processAlterTablerejects theSTREAMtable type withDo not support alter STREAM table[...].This PR adds:
MODIFYis accepted alongsideSETso the syntax stays consistent withALTER TABLE ... MODIFY COMMENT, which is the existing Doris spelling for the same operation on a table.Implementation notes:
Tablemetadata only, soAlter#processAlterStreamCommentreusesModifyCommentOperationLog.forTable(...)and the existing replay pathAlter#replayModifyComment, which already resolves a genericTable. No new edit log operation and no meta version bump.CloudInternalCatalog#beforeCreateTableStream/#afterCreateTableStream), so no extra RPC is needed and the behaviour is the same in cloud mode.AlterStreamCommandextendsAlterCommand, which already providesForwardWithSyncandStmtType.ALTER. It carries anAlterTypeenum so that otherALTER STREAMclauses can be added later without reshaping the command.ALTERon the stream, matchingALTER TABLE. Altering a non-stream table throughALTER STREAMreportsERR_WRONG_OBJECT, the same waySHOW CREATE STREAMdoes.Config.enable_table_streamgates the operation, consistent withCREATE STREAMandDROP STREAM.SqlLiteralUtils.parseStringLiteral, so a doubled quotecollapses to one quote and backslash escapes follow the session sql mode, matching the lexer
(
NereidsParserdrives the lexer withSqlModeHelper.hasNoBackSlashEscapes()).CREATE STREAM ... COMMENTwas decoding the same literal differently -- it unescapedbackslashes but never collapsed doubled quotes and ignored
NO_BACKSLASH_ESCAPES-- so it wasmoved onto the same decoder, otherwise the comment stored by CREATE and by ALTER would differ
for the same text. Not fixed here:
Env#addTableCommentquotes the value with single quoteswhile escaping only double quotes, so a comment holding a
'makesSHOW CREATEemitnon-parsable DDL. That is pre-existing, shared by all 19 call sites of every table type, and
will be filed separately.
2. Regression coverage for immutable binlog properties (#65383)
ALTER TABLE ... SET ("binlog.format" = ...)on a ROW binlog table used to fail with a misleading light-schema-change error, becauseAlterOperations#checkBinlogConfigChangedid not listbinlog.format/binlog.need_historical_valueand the statement was dispatched to the generic schema change path. That was fixed as a side effect of #65810 (f745ddf9e22), but no test locked the behaviour in. This PR addstest_binlog_property_alter_exception.groovycovering:binlog.format = ROWMOW table)SET ("binlog.format" = "STATEMENT_AND_SNAPSHOT")not support change binlog format from ROW to STATEMENT_AND_SNAPSHOTSET ("binlog.need_historical_value" = "false")not support change binlog.need_historical_value from true to falseSET ("binlog.enable" = "false")can't disable binlog when format is [Row]SET ("binlog.format" = "ROW")(same value)SET ("binlog.ttl_seconds" = "7200")SET ("binlog.format" = "ROW")on a table without binlognot support change binlog format from STATEMENT_AND_SNAPSHOT to ROWRelease note
Support
ALTER STREAM <name> SET|MODIFY COMMENT '<comment>'to change the comment of a table stream.Stream comments are now decoded as proper SQL string literals:
'a''b'storesa'b, andbackslash escapes follow the session sql mode (including
NO_BACKSLASH_ESCAPES). This applies toCREATE STREAM ... COMMENTas well, which previously stored a doubled quote verbatim.Check List (For Author)
Test
regression-test/suites/table_stream_p0/test_table_stream_alter_comment.groovyregression-test/suites/table_stream_p0/test_binlog_property_alter_exception.groovyfe/fe-core/src/test/java/org/apache/doris/catalog/AlterTableStreamCommentTest.java(testAlterStreamCommentand
testAlterStreamCommentStringLiteral, the latter covering doubled single quotes,doubled double quotes,
\n/\tandNO_BACKSLASH_ESCAPES)Behavior changed:
ALTER STREAM ... SET|MODIFY COMMENTis accepted. It was aparser error before, so no existing statement changes behaviour there.
CREATE STREAM ... COMMENTdoes change: a doubled quote in the comment now collapses to a single quote andNO_BACKSLASH_ESCAPESis honoured, i.e. the literal is decoded the way every other SQLstring literal is.
Does this need documentation?
Check List (For Reviewer who merge this PR)
🤖 Generated with Claude Code
https://claude.ai/code/session_01Xx7TgjXJCiChnzLYa6hgtL