Skip to content

Conversation

@amorynan
Copy link
Contributor

@amorynan amorynan commented Jan 16, 2023

Proposed changes

TO support doris complex type for map
Issue Number: close #xxx

Problem summary

Describe your changes.

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 github-actions bot added area/planner Issues or PRs related to the query planner area/sql/function Issues or PRs related to the SQL functions area/vectorization labels Jan 16, 2023
Copy link
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.

clang-tidy made some suggestions

Comment on lines 124 to 125
if (!keys->equals(*rhs_map.keys))
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: statement should be inside braces [readability-braces-around-statements]

Suggested change
if (!keys->equals(*rhs_map.keys))
return false;
if (!keys->equals(*rhs_map.keys)) {
return false;
}

@amorynan amorynan changed the base branch from master to struct-type January 16, 2023 10:05
@morningman
Copy link
Contributor

Please change the title and rebase the code

@amorynan amorynan changed the title feature-map [Feature](map)support complex struct for doris Jan 17, 2023
Copy link
Contributor

@xiaokang xiaokang left a comment

Choose a reason for hiding this comment

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

fe reviewed

Copy link
Contributor

@xiaokang xiaokang left a comment

Choose a reason for hiding this comment

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

be code reviewed

@amorynan amorynan requested review from xiaokang and xy720 and removed request for xiaokang January 29, 2023 07:38
Copy link
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.

clang-tidy made some suggestions

Copy link
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.

clang-tidy made some suggestions

@amorynan amorynan requested review from xiaokang and removed request for xiaokang February 1, 2023 10:48
@amorynan amorynan requested a review from xiaokang February 1, 2023 10:49
Copy link
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.

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 27. Check the log or trigger a new build to see more.

create_data_type(col_desc.children[1], col_desc.contains_nulls[1]));
break;
}
case INVALID_TYPE:
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'case' statement not in switch statement [clang-diagnostic-error]

case INVALID_TYPE:
^

for (int i = 0; i < _children.size(); ++i) {
if (!_children[i]->is_constant()) {
return false;
std::string VExpr::debug_string(const std::vector<VExprContext*>& ctxs) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function definition is not allowed here [clang-diagnostic-error]

    std::string VExpr::debug_string(const std::vector<VExprContext*>& ctxs) {
                                                                            ^


return true;
}
bool VExpr::is_constant() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function definition is not allowed here [clang-diagnostic-error]

    bool VExpr::is_constant() const {
                                    ^

}

if (_constant_col != nullptr) {
Status VExpr::get_const_col(VExprContext * context, ColumnPtrWrapper * *output) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function definition is not allowed here [clang-diagnostic-error]

    Status VExpr::get_const_col(VExprContext * context, ColumnPtrWrapper * *output) {
                                                                                    ^

return Status::OK();
}

Status VExpr::create_tree_from_thrift(doris::ObjectPool* pool,
Copy link
Member

Choose a reason for hiding this comment

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

Why modify here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when I use clang-format.sh , it make these change...

Copy link
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.

clang-tidy made some suggestions

Copy link
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.

clang-tidy made some suggestions

@amorynan amorynan changed the title [Feature](map)support complex struct for doris [Feature](map) add map type to doris Feb 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

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

@amorynan amorynan requested review from xy720 and removed request for xiaokang and xy720 February 2, 2023 14:50
Copy link
Member

@xy720 xy720 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 github-actions bot added the approved Indicates a PR has been approved by one committer. label Feb 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2023

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

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2023

PR approved by anyone and no changes requested.

@amorynan amorynan requested a review from xiaokang February 3, 2023 04:04
@xy720 xy720 merged commit af9234b into apache:struct-type Feb 3, 2023
xy720 pushed a commit that referenced this pull request Feb 6, 2023
Add complex type map to doris on vectorized engine
xy720 pushed a commit that referenced this pull request Feb 9, 2023
Add complex type map to doris on vectorized engine
xy720 pushed a commit that referenced this pull request Feb 9, 2023
Add complex type map to doris on vectorized engine
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. area/planner Issues or PRs related to the query planner area/sql/function Issues or PRs related to the SQL functions area/vectorization reviewed

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants