Skip to content

Conversation

@yiguolei
Copy link
Contributor

@yiguolei yiguolei commented Aug 17, 2022

Proposed changes

This is part of #10136

This PR also decouple delete hander from reader.

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

reader_params.delete_predicates.begin()));
}

TabletSchemaSPtr merge_tablet_schema = std::make_shared<TabletSchema>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't repeat yourself. same as the line 50, maybe we need a function

std::vector<bool> _is_upper_keys_included;
// contains condition on key columns in agg or unique table or all column in dup tables
Conditions _conditions;
std::unique_ptr<Conditions> _conditions;
Copy link
Contributor

Choose a reason for hiding this comment

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

why change unique_ptr, need more malloc operator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because I change tablet schema as a parameter in Conditions constructor and remove set tablet schema method because the condtion should always have a tablet schema and should not be changed. But tablet schema is not required in reader's constructor.

new_load_id->set_lo(load_id.lo());
}

bool delete_flag() const { return _rowset_meta_pb.delete_flag(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

this code useless also ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, it is useless. Because it is never be set, but collector depend on it and many logic depend on it, I do not know the meaning of the flag.

@Lchangliang
Copy link
Contributor

LGTM

@Lchangliang
Copy link
Contributor

LGTM

return (*it)->tablet_schema();
}
++it;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

_rs_version_map in Tablet is much more efficient.

Status Conditions::append_condition(const TCondition& tcond) {
DCHECK(_schema != nullptr);
int32_t index = _schema->field_index(tcond.column_name);
int32_t index = _schema->field_index(tcond.column_unique_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we hide index in the TabletSchema? We can use a function like this _schema->get_clolumn_by_unique_id(tcond.column_unique_id).

Copy link
Contributor

@dataroaring dataroaring 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 Aug 26, 2022
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@yiguolei yiguolei merged commit ccff3f5 into apache:master Aug 26, 2022
GoGoWen pushed a commit to GoGoWen/incubator-doris that referenced this pull request Aug 31, 2022
…ma change (apache#11869)

* [bugfix](light weight schema change) support delete condition in schema change


Co-authored-by: yiguolei <yiguolei@gmail.com>
@yiguolei yiguolei deleted the using_unique_id branch March 30, 2023 10:17
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/vectorization kind/test reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants