Skip to content

Comments

[refractor](schema) refractor function Schema::get_column_by_field to make it simple#16027

Merged
yiguolei merged 1 commit intoapache:masterfrom
cambyzju:fix-0117
Jan 18, 2023
Merged

[refractor](schema) refractor function Schema::get_column_by_field to make it simple#16027
yiguolei merged 1 commit intoapache:masterfrom
cambyzju:fix-0117

Conversation

@cambyzju
Copy link
Contributor

@cambyzju cambyzju commented Jan 17, 2023

Proposed changes

Issue Number: close #xxx

Problem summary

DataTypeFactory::instance().create_data_type will consider Field.is_nullable, may return type T or Nullable(T).
Then datatype create_column, will create Column or ColumnNullable(Column).

So we do not need to create ColumnNullable manually.

Checklist(Required)

  1. Does it affect the original behavior:
    • Yes
    • No
    • I don't know
  2. Has unit tests been added:
    • Yes
    • No
    • No Need
  3. Has document been added or modified:
    • Yes
    • No
    • No Need
  4. Does it need to update dependencies:
    • Yes
    • No
  5. Are there any changes that cannot be rolled back:
    • Yes (If Yes, please explain WHY)
    • No

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@hello-stephen
Copy link
Contributor

TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 35.51 seconds
load time: 500 seconds
storage size: 17122462582 Bytes
https://doris-community-test-1308700295.cos.ap-hongkong.myqcloud.com/tmp/20230117182551_clickbench_pr_82539.html

Copy link
Contributor

@yinzhijian yinzhijian left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@yiguolei yiguolei merged commit b2fe385 into apache:master Jan 18, 2023
dutyu pushed a commit to dutyu/doris that referenced this pull request Feb 1, 2023
… make it simple apache#16027

Co-authored-by: cambyzju <zhuxiaoli01@baidu.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants