Skip to content
This repository was archived by the owner on Jun 14, 2024. It is now read-only.

Conversation

@sezruby
Copy link
Collaborator

@sezruby sezruby commented Dec 9, 2021

What is the context for this pull request?

What changes were proposed in this pull request?

CoveringIndex refactoring to support other covering index type - ZOrderCoveringIndex (#495)
Added CoveringIndexTrait & CoveringIndexConfigTrait to reduce duplicate code.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Refactoring change - tested by existing tests

@sezruby sezruby self-assigned this Dec 10, 2021
@sezruby sezruby requested a review from clee704 December 10, 2021 00:12
@sezruby sezruby mentioned this pull request Dec 10, 2021
4 tasks
Comment on lines +33 to +34
val schema: StructType
val includedColumns: Seq[String]
Copy link

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! I'll address your comments in the next PR.


override def statistics(extended: Boolean = false): Map[String, String] = {
simpleStatistics ++ (if (extended) extendedStatistics else Map.empty)
protected def copyIndex(
Copy link

Choose a reason for hiding this comment

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

nit: override

override def canHandleDeletedFiles: Boolean = hasLineageColumn

override def write(ctx: IndexerContext, indexData: DataFrame): Unit = {
protected def write(ctx: IndexerContext, indexData: DataFrame, mode: SaveMode): Unit = {
Copy link

Choose a reason for hiding this comment

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

nit: override

sortColumnNames = indexedColumns))

private def simpleStatistics: Map[String, String] = {
protected def simpleStatistics: Map[String, String] = {
Copy link

Choose a reason for hiding this comment

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

nit: override

}

private def extendedStatistics: Map[String, String] = {
protected def extendedStatistics: Map[String, String] = {
Copy link

Choose a reason for hiding this comment

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

nit: override

@sezruby sezruby merged commit 64aae2f into microsoft:master Dec 13, 2021
@sezruby sezruby added the enhancement New feature or request label Dec 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants