[HUDI-2456] support 'show partitions' sql #3693
Conversation
|
@leesf can you have time to review this? |
There was a problem hiding this comment.
This is a great contribution. Few comments.
@pengzhiwei2018 are you able to review this and probably all follow ups from the spark SQL work. I am seeing adoption really picking up.
| @@ -40,7 +40,7 @@ | |||
| protected static final String NULL_RECORDKEY_PLACEHOLDER = "__null__"; | |||
| protected static final String EMPTY_RECORDKEY_PLACEHOLDER = "__empty__"; | |||
|
|
|||
| protected static final String DEFAULT_PARTITION_PATH = "default"; | |||
| protected static final String DEFAULT_PARTITION_PATH = PartitionPathEncodeUtils.HUDI_DEFAULT_PARTITION_PATH; | |||
There was a problem hiding this comment.
rename: HOODIE_DEFAULT_PARTITION_PATH
| @@ -25,6 +25,8 @@ | |||
| */ | |||
| public class PartitionPathEncodeUtils { | |||
|
|
|||
| public static final String HUDI_DEFAULT_PARTITION_PATH = "default"; | |||
There was a problem hiding this comment.
actually lets just do DEFAULT_PARTITION_PATH
| @@ -71,7 +73,7 @@ public static String escapePathName(String path, String defaultPath) { | |||
| if (defaultPath == null) { | |||
| //previously, when path is empty or null and no default path is specified, | |||
| // __HIVE_DEFAULT_PARTITION__ was the return value for escapePathName | |||
| return "__HIVE_DEFAULT_PARTITION__"; | |||
| return HUDI_DEFAULT_PARTITION_PATH; | |||
There was a problem hiding this comment.
why is this change needed
There was a problem hiding this comment.
When the partition's value is null, hudi use "default" as the part of path, rather than "HIVE_DEFAULT_PARTITION".
And I review the implement when need to encode partition path before #2645, like getRecordPartitionPath and getPartitionPath in KeyGenUtils. I think this just encode path, and use "default" when partitionPath is null or empty.
So i guess maybe this is forgotten to change.
| /** | ||
| * Helper trait for all hoodie commands. | ||
| */ | ||
| trait HoodieCommand { |
There was a problem hiding this comment.
in the interest of keeping class hierarchies tenable, can this become a helper method?
There was a problem hiding this comment.
there is already HoodieSqlUtils class, can we move to HoodieSqlUtils?
There was a problem hiding this comment.
OK, i'll amend it.
| * @param expectedAnswer the expected result in a [[Seq]] of [[Row]]s. | ||
| * @param checkToRDD whether to verify deserialization to an RDD. This runs the query twice. | ||
| */ | ||
| def checkAnswer( |
There was a problem hiding this comment.
there is already checkAnswer method in TestHoodieSqlBase, can we reuse that and avoid copying from Spark 2.X (org.apache.spark.sql.QueryTest) ?
There was a problem hiding this comment.
checkAnswer in TestHoodieSqlBase doesn't work for this unit tests. If two sequences have the same elements, but not in the same order, it is judged to be different.
checkAnswer method in QueryTest will sort the two sequences and then compare them.
And The implements's inconsistent between Spark2 and Spark3 is the reason to copy codes rather than extend QueryTest. So I copy some codes needed here.
|
@vinothchandar @leesf did i make myself clear for |
| @@ -71,7 +73,7 @@ public static String escapePathName(String path, String defaultPath) { | |||
| if (defaultPath == null) { | |||
| //previously, when path is empty or null and no default path is specified, | |||
There was a problem hiding this comment.
the description should be updated accordingly?
No description provided.