kasakrisz commented on code in PR #5498:
URL: https://github.com/apache/hive/pull/5498#discussion_r1810219560


##########
ql/src/java/org/apache/hadoop/hive/ql/metadata/DummyPartition.java:
##########
@@ -51,11 +48,15 @@ public DummyPartition(Table tbl, String name) {
     this.name = name;
   }  
 
-  public DummyPartition(Table tbl, String name,
-      Map<String, String> partSpec) {
-    setTable(tbl);
+  public DummyPartition(Table tbl, String name, Map<String, String> partSpec) 
throws HiveException {

Review Comment:
   Seems that the current class hierarchy doesn't represent our needs:
   * `Partition` class represents a partition stored on HMS.
   * `DummyPartition` extends `Partition` so it seems like a special HMS stored 
partition but this is not the case.
   
   Maybe defining a Partition interface or abstract class with a minimal 
contract (getName, getValues) would be better. And two class could 
implement/extend it: one for HMS stored partition and another for non-HMS.
   
   It seems to be a bigger refactor because the current `Partition` class is 
widely used.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org

Reply via email to