[#7332] feat(api): Support statistic interfaces #7333
Conversation
jerqi
force-pushed
the
ISSUE-7270
branch
4 times, most recently
from
55ab5d2 to
c5f0cc3
Compare
api/src/main/java/org/apache/gravitino/stats/StatisticValue.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/gravitino/stats/StatisticValue.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/gravitino/stats/StatisticValue.java
Outdated
Show resolved
Hide resolved
| */ | ||
| List<Statistic> updateStatistics(Map<String, Literal> statistics) | ||
| throws UnmodifiableStatisticException, IllegalStatisticNameException; | ||
| } |
There was a problem hiding this comment.
Do we need an interface to drop the statistics?
There was a problem hiding this comment.
I seems more clear to add dropStatistics. I can add it.
| * @param values the list of statistic values to be held by this statistic value | ||
| * @return a ListValue instance containing the provided list of statistic values | ||
| */ | ||
| public static ListValue listValue(List<StatisticValue> values) { |
There was a problem hiding this comment.
Should you change to ListValue<T>, what is the type of StatisticValue in the list?
| } | ||
|
|
||
| /** A statistic value that holds a Map of String keys to other statistic values. */ | ||
| public static class ObjectValue implements StatisticValue<Map<String, StatisticValue>> { |
There was a problem hiding this comment.
Why call it ObjectValue, not MapValue?
There was a problem hiding this comment.
In our type system, MapValue need the same value type. I refer to the json style, named it to objectValue.
| * @param values the map of string keys to statistic values to be held by this statistic value | ||
| * @return an ObjectValue instance containing the provided map of statistic values | ||
| */ | ||
| public static ObjectValue objectValue(Map<String, StatisticValue> values) { |
There was a problem hiding this comment.
Java will complain about removing the type for StatisticValue here, can you figure out a better solution?
There was a problem hiding this comment.
I addressed the warning.
| * @param statistics a map of statistic names to their values | ||
| * @return a list of updated statistics | ||
| */ | ||
| List<Statistic> updateStatistics(Map<String, StatisticValue> statistics) |
There was a problem hiding this comment.
Also here, since you introduce the type definition for StatisticValue, removing the type will make Java compiler warning.
There was a problem hiding this comment.
I addressed the warning.
|
|
||
| /** | ||
| * The statistic is predefined by Gravitino if the value is true. The statistic is defined by | ||
| * users if the value if false. For example, the statistic "row_count" is a reserved statistic, A |
| public void testLiterals() { | ||
| StatisticValues.BooleanValue booleanValue = StatisticValues.booleanValue(true); | ||
| Assertions.assertTrue(booleanValue.value()); | ||
| Assertions.assertEquals(booleanValue.dataType(), Types.BooleanType.get()); |
There was a problem hiding this comment.
StatisticValues.BooleanValue b1 = StatisticValues.booleanValue(true);
StatisticValues.BooleanValue b2 = StatisticValues.booleanValue(true);
Will these two statistic values be equals?
There was a problem hiding this comment.
I added the equals and hashCode to fix this issue.
What changes were proposed in this pull request?
Support statistic interfaces
Why are the changes needed?
Fix: #7332
Does this PR introduce any user-facing change?
Yes, I will add the document
How was this patch tested?
Code review