Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Low cardinality group by #3138

Merged
merged 20 commits into from
Sep 21, 2018
Merged

Low cardinality group by #3138

merged 20 commits into from
Sep 21, 2018

Conversation

KochetovNicolai
Copy link
Member

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

@@ -16,6 +18,8 @@ class IColumnUnique : public IColumn
/// The same as getNestedColumn, but removes null map if nested column is nullable.
virtual const ColumnPtr & getNestedNotNullableColumn() const = 0;

virtual const UInt64 * tryGetSavedHash() const = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Missing comment (about the purpose of this method).

@@ -161,4 +161,6 @@ class DataTypeWithDictionary : public IDataType
static MutableColumnUniquePtr createColumnUniqueImpl(const IDataType & keys_type, const Creator & creator);
};

DataTypePtr removeLowCardinality(const DataTypePtr & type);
Copy link
Member

Choose a reason for hiding this comment

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

Missing comment.

@@ -145,14 +160,23 @@ struct AggregationMethodOneNumber

/** Do not use optimization for consecutive keys.
*/
static const bool no_consecutive_keys_optimization = false;
static const bool no_consecutive_keys_optimization = true;
Copy link
Member

Choose a reason for hiding this comment

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

Why it was changed?


/** Insert the key from the hash table into columns.
*/
static void insertKeyIntoColumns(const typename Data::value_type & value, MutableColumns & key_columns, size_t /*keys_size*/, const Sizes & /*key_sizes*/)
{
static_cast<ColumnVector<FieldType> *>(key_columns[0].get())->insertData(reinterpret_cast<const char *>(&value.first), sizeof(value.first));
}

static StringRef getRef(const typename Data::value_type & value)
Copy link
Member

Choose a reason for hiding this comment

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

Does it mean "getKeyRef"? If so, why we pass value_type instead of key?

static const bool no_consecutive_keys_optimization = false;
static const bool no_consecutive_keys_optimization = true;
/// Use optimization for low cardinality.
static constexpr bool low_cardinality_optimization = false;
Copy link
Member

Choose a reason for hiding this comment

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

Inconsistent const/constexpr.

@@ -549,15 +596,19 @@ void NO_INLINE Aggregator::executeImplCase(
/// NOTE When editing this code, also pay attention to SpecializedAggregator.h.
Copy link
Member

Choose a reason for hiding this comment

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

TODO?

Copy link
Member Author

Choose a reason for hiding this comment

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

TODO

@@ -789,7 +871,7 @@ bool Aggregator::executeOnBlock(const Block & block, AggregatedDataVariants & re
#define M(NAME, IS_TWO_LEVEL) \
else if (result.type == AggregatedDataVariants::Type::NAME) \
executeImpl(*result.NAME, result.aggregates_pool, rows, key_columns, aggregate_functions_instructions.data(), \
key, no_more_keys, overflow_row_ptr);
key, no_more_keys, overflow_row_ptr);
Copy link
Member

Choose a reason for hiding this comment

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

.

Copy link
Member

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

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

Almost Ok.

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.

2 participants