Skip to content

[fix](agg) fix be crash when double type be key in agg table#61798

Merged
yiguolei merged 1 commit intoapache:branch-2.1from
LuGuangming:2.1-addAggColumncrash
Mar 31, 2026
Merged

[fix](agg) fix be crash when double type be key in agg table#61798
yiguolei merged 1 commit intoapache:branch-2.1from
LuGuangming:2.1-addAggColumncrash

Conversation

@LuGuangming
Copy link
Copy Markdown
Contributor

@LuGuangming LuGuangming commented Mar 27, 2026

What problem does this PR solve?

Issue Number: close #61797

Related PR: #xxx

Problem Summary:
be crash when double type be key in agg table

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@LuGuangming LuGuangming requested a review from yiguolei as a code owner March 27, 2026 03:58
@Thearas
Copy link
Copy Markdown
Contributor

Thearas commented Mar 27, 2026

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@Vallishp
Copy link
Copy Markdown
Contributor

run buildall

@LuGuangming LuGuangming changed the title [fix][agg] fix be crash when double type be key in agg table [fix](agg) fix be crash when double type be key in agg table Mar 27, 2026
}
for (ColumnDef colDef : columnDefs) {
if (tableName != null) {
Table table = Env.getCurrentInternalCatalog().getDbOrAnalysisException(tableName.getDb())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this problem only exist in 2.1? what about 3.1 4.0 4.1?

Copy link
Copy Markdown
Contributor Author

@LuGuangming LuGuangming Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all branch are exists, I'll backport others branch

@liutang123
Copy link
Copy Markdown
Contributor

/review

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

This PR adds logic to AddColumnsClause.analyze() to automatically mark columns as key columns in AGG_KEYS tables when no aggregate type is specified, mirroring existing logic in AddColumnClause. While the intent is correct (fixing a BE crash), the implementation has two issues that need to be addressed:

Critical Issues

  1. Missing setKeysType() call (Bug): Both AddColumnClause (line 83-85) and ModifyColumnClause (line 83) call colDef.setKeysType(((OlapTable) table).getKeysType()) after the setIsKey() block. This PR omits this call. The keysType field in ColumnDef is used during analyze() (line 361) to validate complex types (BITMAP, HLL, QUANTILE_STATE). Without setting it, keysType remains null, which causes the condition keysType == null || keysType == KeysType.AGG_KEYS to always be true, potentially producing incorrect validation errors for non-AGG_KEYS tables.

  2. Table lookup inside the loop (Performance): The Env.getCurrentInternalCatalog().getDbOrAnalysisException(...).getTableOrAnalysisException(...) call is placed inside the for loop, causing a redundant database+table lookup for every column in the list. This should be hoisted before the loop since tableName is invariant across iterations.

Minor Issues

  1. Inconsistent exception method: Uses getDbOrAnalysisException/getTableOrAnalysisException while the sibling classes (AddColumnClause, ModifyColumnClause) use getDbOrDdlException/getTableOrDdlException.

  2. Missing regression test: The PR description checks the "Regression test" box but no test file is included in the changeset.

Checklist

  • Correctness: Issues found (missing setKeysType, table lookup in loop)
  • Performance: Table lookup should be moved outside the loop
  • Test coverage: Missing despite checkbox being ticked

import org.apache.doris.alter.AlterOpType;
import org.apache.doris.catalog.Column;
import org.apache.doris.catalog.Env;
import org.apache.doris.catalog.KeysType;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Missing setKeysType() call + Performance: Table lookup should be outside the loop.

Comparing with AddColumnClause.analyze() (lines 76-86), there are two problems:

  1. Missing setKeysType(): AddColumnClause and ModifyColumnClause both call colDef.setKeysType(((OlapTable) table).getKeysType()) after the setIsKey() block. Without this, ColumnDef.keysType remains null, which affects the validation at ColumnDef.java:361 where keysType == null is treated the same as AGG_KEYS, potentially causing incorrect errors for complex types in non-AGG tables.

  2. Table lookup in loop: The table lookup is inside the for loop but tableName doesn't change between iterations. This causes redundant DB+table lookups for each column.

Suggested fix:

if (tableName != null) {
    Table table = Env.getCurrentInternalCatalog().getDbOrAnalysisException(tableName.getDb())
            .getTableOrAnalysisException(tableName.getTbl());
    if (table instanceof OlapTable) {
        OlapTable olapTable = (OlapTable) table;
        for (ColumnDef colDef : columnDefs) {
            if (olapTable.getKeysType() == KeysType.AGG_KEYS
                    && colDef.getAggregateType() == null) {
                colDef.setIsKey(true);
            }
            colDef.setKeysType(olapTable.getKeysType());
        }
    }
}
for (ColumnDef colDef : columnDefs) {
    colDef.analyze(true);
    ...
}

Or alternatively, keep the single loop but move the table lookup before it.

@yiguolei
Copy link
Copy Markdown
Contributor

skip buildall

@github-actions
Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@github-actions github-actions bot added approved Indicates a PR has been approved by one committer. reviewed labels Mar 31, 2026
@github-actions
Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

@yiguolei yiguolei merged commit a3052de into apache:branch-2.1 Mar 31, 2026
29 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants