Refactoring for an extensible Index API: Part 3 #475
Conversation
| @@ -76,7 +77,7 @@ class IndexLogEntryTest extends HyperspaceSuite with SQLHelper { | |||
| |{ | |||
| | "name" : "indexName", | |||
| | "derivedDataset" : { | |||
| | "type" : "com.microsoft.hyperspace.index.CoveringIndex", | |||
| | "type" : "com.microsoft.hyperspace.index.types.covering.CoveringIndex", | |||
There was a problem hiding this comment.
Can we do types.CoveringIndex instead of types.covering.coveringIndex?
hmm.. is that for another type of covering index?
There was a problem hiding this comment.
I'd like to give each index type their own package, like how Spark put data source implementations in the org.apache.spark.sql.execution.datasources.{parquet, orc, json, csv, ...} packages.
If this seems verbose, then how about com.microsoft.hyperspace.indexes.covering? That is, com.microsoft.hyperspace.index is for index-related stuff, and com.microsoft.hyperspace.indexes is for index implementations. Another option is com.microsoft.hyperspace.index.covering.
Note that data skipping indexes will be put in the dataskipping package.
Summary
- Option 1:
com.microsoft.hyperspace.index.types.{covering, dataskipping, ...} - Option 2:
com.microsoft.hyperspace.indexes.{coverinng, dataskipping, ...} - Option 3:
com.microsoft.hyperspace.index.{covering, dataskipping, ...}
|
Other than the comment, LGTM thanks @clee704 👍 |
- Move CoveringIndex specific code into the com.microsoft.hyperspace.index.types.covering package - Move traits/classes/objects in the com.microsoft.hyperspace.index.rules package into their own files - Add alias for CoveringIndexConfig for backward compatibility
|
Changed |
python/hyperspace/indexconfig.py
Outdated
| """ | ||
| self.indexName = indexName | ||
| self.indexedColumns = indexedColumns | ||
| self.includedColumns = includedColumns | ||
|
|
||
| # TODO(ChungminL): Add deprecation warning |
There was a problem hiding this comment.
Will you resolve this soon? Otherwise please create an issue for this and add its link here.
In any case it's not good to add your alias here
There was a problem hiding this comment.
Actually specifying names for TODO comments is a thing that many developers do for a good reason. To quote the Google C++ Style Guide:
TODOs should include the string TODO in all caps, followed by the name, e-mail address, bug ID, or other identifier of the person or issue with the best context about the problem referenced by the TODO. The main purpose is to have a consistent TODO that can be searched to find out how to get more details upon request. A TODO is not a commitment that the person referenced will fix the problem. Thus when you create a TODO with a name, it is almost always your name that is given.
There are TODO comments with names in the Spark codebase too.
There was a problem hiding this comment.
I searched Spark codebase and the first TODO with a name was committed 2 years ago. Let's be consistent with the current TODO policy - an issue link or a follow-up PR soon.
There was a problem hiding this comment.
I didn't know there is a TODO policy - could you share a link or something?
The issue is created as #477.
There was a problem hiding this comment.
No documentation yet😅 but recently added TODOs have issue links. (via code review)
I suggest to add the issue link like below, but I'm fine w/o the comment as the issue itself explains well.
# TODO: Add deprecation warning.
# See https://github.com/microsoft/hyperspace/issues/477
clee704
added
the
refactoring
What is the context for this pull request?
What changes were proposed in this pull request?
com.microsoft.hyperspace.index.types.covering package
com.microsoft.hyperspace.index.rules package into their own files
Does this PR introduce any user-facing change?
No
How was this patch tested?
With existing tests