Repository navigation
[fix](generated column) Preserve generated column sessions in CREATE TABLE LIKE - #67809
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
### What problem does this PR solve?
Problem Summary: CREATE TABLE LIKE reconstructs generated columns from SQL
under the caller's session settings, losing the source columns' expression
settings. A decimal multiplication created with enable_decimal256=true can
therefore produce 1.26212530000 in a clone made with the option disabled,
instead of the source result 1.26212529751. Snapshot each generated column's
settings with its schema, use them when parsing and analyzing the cloned
expression, and preserve them in the new column metadata. Restore the caller's
session after analysis. Cover both decimal setting directions, repeated LIKE,
SQL mode, and session restoration.
### Release note
Fix CREATE TABLE LIKE changing generated column results when the caller's
expression-related session settings differ from the source table's settings.
### Check List (For Author)
- Test: Manual test reproduced the source/clone precision difference on dev.
FE Checkstyle passed; FE compilation and added unit/regression tests pending.
- Behavior changed: Yes, cloned generated columns preserve source semantics.
- Does this need documentation: No
### What problem does this PR solve?
Problem Summary: Record automatically generated regression results for
CREATE TABLE LIKE preserving generated-column expression settings, and use
the current ExprToSqlVisitor API in the metadata unit test.
### Release note
None
### Check List (For Author)
- Test: Unit Test - all 11 CreateTableLikeTest tests passed via run-fe-ut.sh.
Regression test - test_generated_column_like_session,
test_generated_column, and test_generated_column_fault_tolerance_nereids
passed. Output generated with run-regression-test.sh and then verified.
FE build and Checkstyle passed with DISABLE_BUILD_UI=ON ./build.sh --fe.
Manual test verified decimal precision and SQL mode after replacing FE lib.
- Behavior changed: No
- Does this need documentation: No
96f85b0 to
cde9763
Compare
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
|
/review |
There was a problem hiding this comment.
Requesting changes for two production-reachable correctness gaps:
- The source session map is attached only after the rendered CREATE statement is parsed, so an expression whose tokenization depends on
NO_BACKSLASH_ESCAPEScan still makeCREATE TABLE LIKEfail before the guarded reparse. - Generated columns from metadata predating persisted session maps are reparsed from raw SQL under the caller's mode, although the source continues using its serialized analyzed expression; the clone can therefore silently change semantics.
Review checkpoint conclusions:
- Goal and behavior: the positive nonlegacy decimal and
PIPES_AS_CONCATcases are addressed, including repeated LIKE and success-path caller-session restoration, but the two cases above keep the change from being correct end to end. - Scope and architecture: the change is focused and reuses the existing
Column.svpersistence plusAutoCloseSessionVariable; no unrelated API, configuration, or BE/protocol changes were introduced. - Concurrency and lifecycle: source DDL and maps are captured under the same table read lock, and the per-column resource scope restores the exact caller session on analysis failures. The initial whole-DDL parse remains outside that scope.
- Compatibility and persistence: the existing serialized format is reused, but an absent map from older generated-column metadata is normalized to empty and must retain the persisted analyzed expression's behavior.
- Parallel paths and conditions: ordinary CREATE remains unchanged, ALTER ADD generated columns is unsupported, and current insert binding uses the persisted analyzed expression plus session guards. The unsafe branches are the pre-guard parse and the distinction between an absent/empty legacy map and a usable saved map.
- Tests: the added unit and regression coverage is deterministic and exercises successful mode transfer in both decimal directions, repeated cloning, NULLs, and
PIPES_AS_CONCAT. It does not cover a mode-dependent lexer boundary or legacy empty-map replay. - Operations and performance: no new operational surface, observability requirement, or material hot-path cost was found. No other distinct issue remained after three bounded review rounds.
Validation is static only: the review contract prohibited builds and tests, so author/CI test claims were not independently executed.
| createTableCommand = new CreateTableCommand(createTableCommand.getCtasQuery(), | ||
| createTableInfo.withTableNameAndIfNotExists(createTableLikeInfo.getTableName(), | ||
| createTableLikeInfo.isIfNotExists())); | ||
| for (ColumnDefinition column : createTableInfo.getColumnDefinitions()) { |
There was a problem hiding this comment.
[P1] Apply the source SQL mode before the first DDL parse
These saved settings are attached only after parseSingle has already lexed the rendered CREATE statement under the caller's sql_mode. Column.toSql emits GeneratedColumnInfo.exprSql verbatim, so a source created with NO_BACKSLASH_ESCAPES can validly store concat(a, 'C:\'); after the caller clears that mode, the first parse treats the backslash as escaping the closing quote and fails before this loop or GeneratedColumnDesc.getExpression() can run. The lexer test for a trailing backslash already demonstrates this token-boundary split. Please make the initial synthetic-DDL parse independent of the caller mode (or activate the saved mode before it), and cover this opposite-mode literal case.
| public Expression getExpression() { | ||
| return expression; | ||
| // CREATE TABLE LIKE must also parse the expression under its original session settings. | ||
| return sessionVariables.isPresent() ? new NereidsParser().parseExpression(exprSql) : expression; |
There was a problem hiding this comment.
[P1] Preserve legacy generated expressions without a session map
Generated columns existed before Column.sessionVariables, and replay normalizes those missing maps to empty. CREATE LIKE makes that empty map present, AutoCloseSessionVariable applies no override, and this line reparses raw exprSql under the caller. A legacy source created with PIPES_AS_CONCAT can therefore keep executing its serialized analyzed concat expression in BindSink, while LIKE under the default mode reparses a || 'x' as OR and silently creates a different target. For an empty legacy map, please derive the clone from the persisted analyzed expression (or a mode-neutral canonical rendering) and add an upgrade-compatibility test.
There was a problem hiding this comment.
no need to processed because in old version , user hardly use Generated columns.
### What problem does this PR solve? Related PR: apache#67809 Problem Summary: The initial synthetic CREATE TABLE parse uses the caller's SQL mode before saved generated-column settings are restored. A trailing backslash literal created under NO_BACKSLASH_ESCAPES can therefore prevent CREATE TABLE LIKE from reaching the guarded expression analysis. Snapshot original expression SQL together with the saved settings under the source read lock, use AS (NULL) only in internal LIKE DDL, and restore deferred descriptors before running the create command. Ordinary DDL rendering remains unchanged. Legacy empty session maps are outside this change. ### Release note Fix CREATE TABLE LIKE failing to clone generated columns whose string literals require the source SQL mode during the initial DDL parse. ### Check List (For Author) - Test: Unit Test - Added opposite backslash modes, repeated LIKE, dependent generated columns, unchanged DDL rendering, and session restoration on analysis failure. - All 13 CreateTableLikeTest cases passed for this implementation on the previous validation branch, commit 00df0febb96. - On this target branch, run-fe-ut.sh --run org.apache.doris.catalog.CreateTableLikeTest passed production compilation but was blocked during test compilation by the existing IvmNormalizeMTMVJoinTest.java:1053 IvmRewriteContext constructor mismatch. No unrelated test source was modified. - Checkstyle passed for fe-core and fe-catalog; git diff --check passed. - No live-cluster regression test was run for this incremental change. - Behavior changed: Yes; generated expressions are parsed under saved source settings. - Does this need documentation: No
|
run buildall |
TPC-H: Total hot run time: 27436 ms |
TPC-DS: Total hot run time: 153342 ms |
ClickBench: Total hot run time: 24.01 s |
|
/review |
|
run feut |
|
run cloud_p0 |
There was a problem hiding this comment.
Static review result: REQUEST_CHANGES.
I found one distinct blocking correctness issue: copied-schema Decimal256 validation still runs under the caller's setting before the source generated-column setting is applied. The inline comment contains the reproducer and requested regression coverage.
Checkpoint conclusions:
- Correctness and goal: placeholder reconstruction now avoids caller-SQL-mode tokenization for current managed-table metadata, and nonempty per-column maps flow through analysis, persistence, and later BindSink rebinding. The goal is still incomplete for declared or nested Decimal256 types above precision 38; the already-open legacy empty-map issue was not duplicated.
- Scope and parallel paths: placeholder rendering is confined to the sole managed CREATE TABLE LIKE reconstruction call. Ordinary SHOW/CREATE/ALTER and external rendering retain their prior paths.
- Concurrency and lifecycle: source descriptors are copied under the table read lock, then used locally; try-with-resources restores the exact caller session on success and failure. No new shared-state or lock-order issue was found, but the scope begins too late for copied type validation.
- Configuration, compatibility, and persistence: no new configuration, FE-BE protocol, or storage-format field is introduced. Existing nonempty session maps are defensively copied and persisted; upgrade-era absent/empty maps remain covered by an existing review thread.
- Tests: the added unit/regression cases are deterministic and cover both backslash modes, repeated LIKE, caller restoration, runtime decimal-derived semantics, and PIPES_AS_CONCAT. They omit a declared precision-above-38 case, which would expose the inline issue. This review was static-only; no build or tests were run under the review contract.
- Error handling, data safety, performance, and observability: failures abort before publication and restore caller state; no transaction or data-write path changes. The cold-DDL work is linear in schema size with small map copies, and no material performance or observability concern was found.
User focus: no additional user-provided review focus was specified. The review converged after two rounds with no unresolved candidates beyond the submitted issue and the existing duplicate-fenced threads.
| Expr e = expr.accept(translator, planTranslatorContext); | ||
| info.get().setExpr(e); | ||
| exprAndNames.add(new ExprAndName(e.clone(), column.getName())); | ||
| try (AutoCloseSessionVariable autoClose = new AutoCloseSessionVariable(ctx, |
There was a problem hiding this comment.
[P1] Validate reconstructed Decimal256 types independently of the caller
This scope starts only inside generatedColumnCheck, after validate() has already called ColumnDefinition.validate for every copied column. A source created with enable_decimal256=true, for example (k INT, g DECIMAL(40,0) AS (k)), is rendered and parsed successfully as decimal(40,0) AS (NULL); however, with the caller setting disabled, type.validateDataType() rejects that declared Decimal256 type before execution reaches this saved-session scope. This is distinct from the existing raw-expression lexer thread because the placeholder parse succeeds and the failure occurs later during schema validation. Please make validation of reconstructed catalog types independent of the caller, including recursively nested types, and add an opposite-setting case with declared precision above 38.
There was a problem hiding this comment.
should report error for this "CREATE TABLE dst LIKE src; " in :
SET enable_decimal256 = true;
CREATE TABLE src (
k INT,
g DECIMAL(40,0) AS (k)
)
DISTRIBUTED BY HASH(k) BUCKETS 1
PROPERTIES ("replication_num" = "1");
SET enable_decimal256 = false;
CREATE TABLE dst LIKE src;
What problem does this PR solve?
Problem Summary:
CREATE TABLE LIKE reconstructs generated columns from SQL using the caller's session settings, losing the expression settings saved in the source columns. For a generated expression
a * boverDECIMAL(20,5)andDECIMAL(21,6), a source created withenable_decimal256=trueproduces1.26212529751, but a clone created with the option disabled produces1.26212530000for the same input. SQL mode also affects parsing: a source expressiona || 'x'created withPIPES_AS_CONCATproducesax, while its clone can produce NULL under a different SQL mode.Snapshot each generated column's session settings under the source table's read lock and carry them through
GeneratedColumnDesc. Restore these settings temporarily when parsing and analyzing the cloned expression, then persist them in the newColumn. The caller's session is restored after analysis, including on exceptions. Ordinary CREATE TABLE retains its existing session behavior.Existing incorrectly created clones and stored data are not automatically repaired.
Release note
Fix CREATE TABLE LIKE changing generated column results when the caller's expression-related session settings differ from the source table's settings.
Check List (For Author)
Test
test_generated_column_like_session,test_generated_column, andtest_generated_column_fault_tolerance_nereidson the final branch's FE.run-regression-test.shand verified in subsequent runs.CreateTableLikeTesttests passed viarun-fe-ut.shon the initial validation baseline with the same repair and test files.IvmNormalizeMTMVJoinTest(an outdatedIvmRewriteContextconstructor call). Final FE packaging therefore usedDISABLE_BUILD_UI=ON MVN_OPT=-Dmaven.test.skip=true ./build.sh --fe. No unrelated test code was changed. Checkstyle passed.be_exec_version=13to match its existing BE.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)