You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/08/15 00:16:11 UTC
[GitHub] [incubator-pinot] Ruoyingw opened a new pull request #5869: Roundup decimal points when compressing AnomalyTimelinesView
Ruoyingw opened a new pull request #5869:
URL: https://github.com/apache/incubator-pinot/pull/5869
## Description
Previously it is found that many saved prediction results in AnomalyTimelinesView contain up to 14 decimal points. This is a waste of storage with little added information on the values. This pr first round up the time series to 3 decimal points before compressing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Ruoyingw commented on a change in pull request #5869: Roundup decimal points when compressing AnomalyTimelinesView
Posted by GitBox <gi...@apache.org>.
Ruoyingw commented on a change in pull request #5869:
URL: https://github.com/apache/incubator-pinot/pull/5869#discussion_r471787196
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/anomaly/views/CondensedAnomalyTimelinesView.java
##########
@@ -182,9 +183,33 @@ public static CondensedAnomalyTimelinesView fromAnomalyTimelinesView(AnomalyTime
* @return a compressed CondensedAnomalyTimelinesView
*/
public CondensedAnomalyTimelinesView compress() {
+ if (timeStamps.size() == 0) {
+ return this;
+ }
+ try {
+ if (this.toJsonString().length() > DEFAULT_MAX_LENGTH) {
+ // First try rounding up
+ roundUp();
Review comment:
Good suggestion. That also avoids some code duplications. At first I wanted to avoid putting it in compress(DEFAULT_MAX_LENGTH) since that one is called recursively. But now I think it is actually more robust to have this roundup every time we do compression. modified.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Ruoyingw commented on a change in pull request #5869: Roundup decimal points when compressing AnomalyTimelinesView
Posted by GitBox <gi...@apache.org>.
Ruoyingw commented on a change in pull request #5869:
URL: https://github.com/apache/incubator-pinot/pull/5869#discussion_r471787378
##########
File path: thirdeye/thirdeye-pinot/src/test/java/org/apache/pinot/thirdeye/anomaly/views/TestCondensedAnomalyTimelinesView.java
##########
@@ -73,35 +74,46 @@ public void testFromJsonString() throws Exception{
}
}
+ /** Compression Test case 1: anomaly view could satisfy requirement after rounding up the decimals.*/
+ @Test
+ public void testCompressWithRoundUp() throws Exception {
+ int testNum = 500;
+ CondensedAnomalyTimelinesView condensedView = CondensedAnomalyTimelinesView.fromAnomalyTimelinesView(getTestData(testNum));
+ Assert.assertTrue(condensedView.toJsonString().length() > CondensedAnomalyTimelinesView.DEFAULT_MAX_LENGTH);
+ CondensedAnomalyTimelinesView compressedView = condensedView.compress();
+ Assert.assertTrue(compressedView.toJsonString().length() < CondensedAnomalyTimelinesView.DEFAULT_MAX_LENGTH);
+ Assert.assertEquals(testNum, compressedView.timeStamps.size());
+ }
+
+ /** Compression Test case 2: The anomaly view is still too large after rounding up, and needed to be further compressed */
@Test
public void testCompress() throws Exception {
- int testNum = 1500;
+ int testNum = 600;
long minBucketMillis = CondensedAnomalyTimelinesView.DEFAULT_MIN_BUCKET_UNIT;
CondensedAnomalyTimelinesView condensedView = CondensedAnomalyTimelinesView.fromAnomalyTimelinesView(getTestData(testNum));
Assert.assertTrue(condensedView.toJsonString().length() > CondensedAnomalyTimelinesView.DEFAULT_MAX_LENGTH);
-
CondensedAnomalyTimelinesView compressedView = condensedView.compress();
Assert.assertTrue(compressedView.toJsonString().length() < CondensedAnomalyTimelinesView.DEFAULT_MAX_LENGTH);
- Assert.assertEquals(compressedView.bucketMillis.longValue(), 240l);
+ Assert.assertEquals(300, compressedView.timeStamps.size());
+ Assert.assertEquals(compressedView.bucketMillis.longValue(), 10);
DateTime date = new DateTime(2018, 1, 1, 0, 0, 0);
for (int i = 0; i < compressedView.getTimeStamps().size(); i++) {
Assert.assertEquals(compressedView.getTimeStamps().get(i).longValue(),
(date.getMillis() - condensedView.timestampOffset)/minBucketMillis);
- Assert.assertEquals(compressedView.getCurrentValues().get(i), i * 4 + 1.5, 0.000001);
- Assert.assertEquals(compressedView.getBaselineValues().get(i), i * 4 + 1.6, 0.000001);
- date = date.plusHours(4);
+ Assert.assertEquals(compressedView.getCurrentValues().get(i), i * 2 + 0.5, 0.000001);
+ Assert.assertEquals(compressedView.getBaselineValues().get(i), i * 2 + 0.833, 0.000001);
Review comment:
right I should do that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jihaozh merged pull request #5869: Roundup decimal points when compressing AnomalyTimelinesView
Posted by GitBox <gi...@apache.org>.
jihaozh merged pull request #5869:
URL: https://github.com/apache/incubator-pinot/pull/5869
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] vincentchenjl commented on a change in pull request #5869: Roundup decimal points when compressing AnomalyTimelinesView
Posted by GitBox <gi...@apache.org>.
vincentchenjl commented on a change in pull request #5869:
URL: https://github.com/apache/incubator-pinot/pull/5869#discussion_r471774644
##########
File path: thirdeye/thirdeye-pinot/src/test/java/org/apache/pinot/thirdeye/anomaly/views/TestCondensedAnomalyTimelinesView.java
##########
@@ -73,35 +74,46 @@ public void testFromJsonString() throws Exception{
}
}
+ /** Compression Test case 1: anomaly view could satisfy requirement after rounding up the decimals.*/
+ @Test
+ public void testCompressWithRoundUp() throws Exception {
+ int testNum = 500;
+ CondensedAnomalyTimelinesView condensedView = CondensedAnomalyTimelinesView.fromAnomalyTimelinesView(getTestData(testNum));
+ Assert.assertTrue(condensedView.toJsonString().length() > CondensedAnomalyTimelinesView.DEFAULT_MAX_LENGTH);
+ CondensedAnomalyTimelinesView compressedView = condensedView.compress();
+ Assert.assertTrue(compressedView.toJsonString().length() < CondensedAnomalyTimelinesView.DEFAULT_MAX_LENGTH);
+ Assert.assertEquals(testNum, compressedView.timeStamps.size());
+ }
+
+ /** Compression Test case 2: The anomaly view is still too large after rounding up, and needed to be further compressed */
@Test
public void testCompress() throws Exception {
- int testNum = 1500;
+ int testNum = 600;
long minBucketMillis = CondensedAnomalyTimelinesView.DEFAULT_MIN_BUCKET_UNIT;
CondensedAnomalyTimelinesView condensedView = CondensedAnomalyTimelinesView.fromAnomalyTimelinesView(getTestData(testNum));
Assert.assertTrue(condensedView.toJsonString().length() > CondensedAnomalyTimelinesView.DEFAULT_MAX_LENGTH);
-
CondensedAnomalyTimelinesView compressedView = condensedView.compress();
Assert.assertTrue(compressedView.toJsonString().length() < CondensedAnomalyTimelinesView.DEFAULT_MAX_LENGTH);
- Assert.assertEquals(compressedView.bucketMillis.longValue(), 240l);
+ Assert.assertEquals(300, compressedView.timeStamps.size());
+ Assert.assertEquals(compressedView.bucketMillis.longValue(), 10);
DateTime date = new DateTime(2018, 1, 1, 0, 0, 0);
for (int i = 0; i < compressedView.getTimeStamps().size(); i++) {
Assert.assertEquals(compressedView.getTimeStamps().get(i).longValue(),
(date.getMillis() - condensedView.timestampOffset)/minBucketMillis);
- Assert.assertEquals(compressedView.getCurrentValues().get(i), i * 4 + 1.5, 0.000001);
- Assert.assertEquals(compressedView.getBaselineValues().get(i), i * 4 + 1.6, 0.000001);
- date = date.plusHours(4);
+ Assert.assertEquals(compressedView.getCurrentValues().get(i), i * 2 + 0.5, 0.000001);
+ Assert.assertEquals(compressedView.getBaselineValues().get(i), i * 2 + 0.833, 0.000001);
Review comment:
Could we add another check to make sure that the JSON exported also has 3 decimal points? For example `Assert.assertEquals(OBJECT_MAPPER.writeValueAsString(compressedView.getBaselineValues().get(i)), "2.833")`
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/anomaly/views/CondensedAnomalyTimelinesView.java
##########
@@ -182,9 +183,33 @@ public static CondensedAnomalyTimelinesView fromAnomalyTimelinesView(AnomalyTime
* @return a compressed CondensedAnomalyTimelinesView
*/
public CondensedAnomalyTimelinesView compress() {
+ if (timeStamps.size() == 0) {
+ return this;
+ }
+ try {
+ if (this.toJsonString().length() > DEFAULT_MAX_LENGTH) {
+ // First try rounding up
+ roundUp();
Review comment:
I think that the roundup logic is better added into `compress(DEFAULT_MAX_LENGTH)`, because people can call either these two methods to compress and `roundup` should run in both cases.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org