-
Notifications
You must be signed in to change notification settings - Fork 387
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#1576] feat(api): add partition management API #1577
Conversation
api/src/main/java/com/datastrato/gravitino/rel/SupportsPartitions.java
Outdated
Show resolved
Hide resolved
api/src/main/java/com/datastrato/gravitino/rel/SupportsPartitions.java
Outdated
Show resolved
Hide resolved
import java.util.Map; | ||
|
||
/** A partition represents a result of partitioning a table. */ | ||
public interface Partition extends Expression { |
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.
why Partitioin extens Expression?
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.
Because partitions often need to be explained by literals and field references, so I make it extends from Expression
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.
+1,
Because partitions often need to be explained by literals and field references, so I make it extends from Expression
I do not see any requirement for it to extend Expression
.
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.
All right, it seems that we don't need it at this phase, I will remove this inheritance relationship for now.
import java.util.Map; | ||
import java.util.Objects; | ||
|
||
/** The helper class for partition expressions. */ |
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.
please provide more descriptions about IdentityPartition, rangePartition and listPartition, I'm confused
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.
More comments are added to the static method, and UTs can also be referred to
import java.util.Map; | ||
|
||
/** A partition represents a result of partitioning a table. */ | ||
public interface Partition extends Expression { |
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.
+1,
Because partitions often need to be explained by literals and field references, so I make it extends from Expression
I do not see any requirement for it to extend Expression
.
api/src/main/java/com/datastrato/gravitino/rel/expressions/partitions/Partitions.java
Outdated
Show resolved
Hide resolved
api/src/main/java/com/datastrato/gravitino/rel/SupportsPartitions.java
Outdated
Show resolved
Hide resolved
* @return The altered partition. | ||
* @throws NoSuchPartitionException If the partition does not exist. | ||
*/ | ||
default Partition alterPartition(String partitionName, PartitionChange... changes) |
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.
What kind of partition changes are we planning to support now?
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.
We currently have no plans to support this.
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.
We can remove this if we don't support it currently.
api/src/main/java/com/datastrato/gravitino/rel/SupportsPartitions.java
Outdated
Show resolved
Hide resolved
@TEOTEO520 can you also help to review if you have time. :) |
api/src/main/java/com/datastrato/gravitino/rel/expressions/partitions/Partition.java
Outdated
Show resolved
Hide resolved
api/src/main/java/com/datastrato/gravitino/rel/partitions/Partitions.java
Show resolved
Hide resolved
api/src/main/java/com/datastrato/gravitino/rel/PartitionChange.java
Outdated
Show resolved
Hide resolved
* @return The altered partition. | ||
* @throws NoSuchPartitionException If the partition does not exist. | ||
*/ | ||
default Partition alterPartition(String partitionName, PartitionChange... changes) |
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.
We can remove this if we don't support it currently.
api/src/main/java/com/datastrato/gravitino/rel/partitions/Partitions.java
Show resolved
Hide resolved
api/src/main/java/com/datastrato/gravitino/rel/partitions/Partitions.java
Show resolved
Hide resolved
api/src/main/java/com/datastrato/gravitino/rel/partitions/Partitions.java
Outdated
Show resolved
Hide resolved
@jerryshao All comments have been resolved. Please review again. |
Partition[] listPartitions(); | ||
|
||
/** | ||
* Get a partition by partition name |
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.
You'd better add more examples about how to get list partition or range partition as we discussed offline.
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.
added
What changes were proposed in this pull request?
table.supportPartitions().xxxPartition(xx, xx)
Why are the changes needed?
Fix: #1576
Does this PR introduce any user-facing change?
no really, does not implement yet
How was this patch tested?
UTs added