Skip to content

Conversation

@HappenLee
Copy link
Contributor

@HappenLee HappenLee commented Nov 23, 2020

  1. Support modify column type CHAR to TINYINT/SMALLINT/INT/BIGINT/LARGEINT/FLOAT/DOUBLE/DATE
    and TINYINT/SMALLINT/INT/BIGINT/LARGEINT/FLOAT/DOUBLE convert to a wider range of numeric types ([Proposal] Support more column type in schema change #4937)

  2. Use template to refactor code of types.h and schema_change.cpp to delete redundant code.

fix issue:#4937

Proposed changes

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.

Types of changes

What types of changes does your code introduce to Doris?
Put an x in the boxes that apply

  • [] Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • [] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [] Documentation Update (if none of the other choices apply)
  • Code refactor (Modify the code structure, format the code, etc...)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have create an issue on (Fix #ISSUE), and have described the bug/feature there in detail
  • Compiling and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • If this change need a document change, I have updated the document
  • Any dependent changes have been merged

1. Support modify column type CHAR to TINYINT/SMALLINT/INT/BIGINT/LARGEINT/FLOAT/DOUBLE/DATE
and TINYINT/SMALLINT/INT/BIGINT/LARGEINT/FLOAT/DOUBLE convert to a wider range of numeric types (apache#4937)

2. Use template to refactor code of types.h and schema_change.cpp to delete redundant code.
return StringParser::string_to_int<T>(src_value->get_data(), src_value->get_size(), &parse_res);
}

template <typename T>
Copy link
Contributor

Choose a reason for hiding this comment

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

There may use traits ?
Or use if constexpr (std::is_floating_point_v)) to distinguish the two cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, chaoyli.

if constexpr only support in c++17 and std::is_floating_point_v support in c++14.

Now, Doris only support C++11, So I use label assignment to distinguish the float and int.

};
template <FieldType fieldType>
struct FieldTypeTraits : public ArithmeTicFieldtypeTraits<fieldType,
std::is_arithmetic<typename BaseFieldtypeTraits<fieldType>::CppType>::value && std::is_signed<typename BaseFieldtypeTraits<fieldType>::CppType>::value> {};
Copy link
Contributor

Choose a reason for hiding this comment

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

use std::is_signed for what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now, ArithmeticField just to keep the small with old code.
old code only Partial Template Specialization the OLAP_FIELD_TYPE_INT but not OLAP_FIELD_TYPE_UNSIGNED_INT. There is no need to Partial Template Specialization for unsign type, so there check the CppType whether is_signed

// Using ArithmeTicFieldtypeTraits to Derived code for OLAP_FIELD_TYPE_XXXINT, OLAP_FIELD_TYPE_FLOAT,
// OLAP_FIELD_TYPE_DOUBLE, to reduce redundant code
template <FieldType fieldType, bool isArithmetic>
struct ArithmeTicFieldtypeTraits : public BaseFieldtypeTraits<fieldType> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Numeric may be a better name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK,i will consider to change the name of it

@morningman morningman merged commit 55ce88d into apache:master Nov 28, 2020
@yangzhg yangzhg mentioned this pull request Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants