Skip to content

Low cardinality group by#3138

Merged
alexey-milovidov merged 20 commits intomasterfrom
low-cardinality-group-by
Sep 21, 2018
Merged

Low cardinality group by#3138
alexey-milovidov merged 20 commits intomasterfrom
low-cardinality-group-by

Conversation

@KochetovNicolai
Copy link
Copy Markdown
Member

/// 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
Copy Markdown
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).

static MutableColumnUniquePtr createColumnUniqueImpl(const IDataType & keys_type, const Creator & creator);
};

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

Choose a reason for hiding this comment

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

Missing comment.

/** 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
Copy Markdown
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?

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

TODO?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

TODO

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

.

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