-
Notifications
You must be signed in to change notification settings - Fork 111
TAJO-1952: Implement PartitionFileFragment #846
Conversation
/** | ||
* Generate the list of files and make them into FileSplits. | ||
* | ||
* @throws IOException | ||
*/ | ||
public List<Fragment> getSplits(String tableName, TableMeta meta, Schema schema, Path... inputs) | ||
public List<Fragment> getSplits(String tableName, TableMeta meta, Schema schema, String[] partitions, Path... inputs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to give a suggestion. We need to keep each method for one purpose if possible. That approach would keep logic simpler.
Could you rebase it against the latest revision? I'd like to try the patch on my machine. |
Thank you for your review. I've just rebased it against the latest version. |
repeated string hosts = 5; | ||
repeated int32 disk_ids = 6; | ||
// Partition Name: country=KOREA/city=SEOUL | ||
required string partitionName = 7; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opionion, partitionKeys would be more proper because the attribute includes concatenated partition keys.
Thank your for your review. I've just reflected your comments. |
I updated the patch as following:
For the reference, I've tried to maintain existing codes as far as possible. |
public PartitionContent() { | ||
} | ||
|
||
public PartitionContent(Path[] partitionPaths) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused constructor
Updated the patch as following:
|
Added unit test cases for PartitionedTableRewriter |
} | ||
|
||
List<Map.Entry<String, Integer>> entries = new ArrayList<>(hostsBlockMap.entrySet()); | ||
Collections.sort(entries, new Comparator<Map.Entry<String, Integer>>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be simplified with lambda.
Thank you for your detailed review. I reflected your comments. |
I changed
|
Evaluated patch testing before and after
The results were same as following:
|
The Travis CI Build seems like a fail by another reason. Here is the pre-commit report by lasted patch as follows. https://builds.apache.org/job/PreCommit-TAJO-Build/900//console |
Is there a problem that needs to be fixed? |
I updated this PR as following:
|
This patch contains following modifications: