Scale timestamps to milliseconds for split queries

Scales all timestamps for split queries to milliseconds. It's important to maintain consistent units between all the partial queries that make up the bigger one.
OpenTSDB · johann8384 · Feb 24, 2021 · Oct 16, 2019 · Oct 16, 2019 · Oct 28, 2019
commit 6fbc98ba42bf88db36f6b876c7c0a35b425f7e83
diff --git a/src/core/SplitRollupQuery.java b/src/core/SplitRollupQuery.java
@@ -60,7 +60,7 @@ public long getStartTime() {
     }
 
     /**
-     * Sets the start time of the graph.
+     * Sets the start time of the graph. Converts the timestamp to milliseconds if necessary.
      *
      * @param timestamp The start time, all the data points returned will have a
      *                  timestamp greater than or equal to this one.
@@ -71,6 +71,8 @@ public long getStartTime() {
      */
     @Override
     public void setStartTime(long timestamp) {
+        if ((timestamp & Const.SECOND_MASK) == 0) { timestamp *= 1000L; }
+
         if (rollupQuery == null) {
             rawQuery.setStartTime(timestamp);
             return;
@@ -101,7 +103,7 @@ public long getEndTime() {
     }
 
     /**
-     * Sets the end time of the graph.
+     * Sets the end time of the graph. Converts the timestamp to milliseconds if necessary.
      *
      * @param timestamp The end time, all the data points returned will have a
      *                  timestamp less than or equal to this one.
@@ -112,6 +114,8 @@ public long getEndTime() {
      */
     @Override
     public void setEndTime(long timestamp) {
+        if ((timestamp & Const.SECOND_MASK) == 0) { timestamp *= 1000L; }
+
         rawQuery.setEndTime(timestamp);
     }
 

diff --git a/src/core/TsdbQuery.java b/src/core/TsdbQuery.java
@@ -448,17 +448,44 @@ public Deferred<Object> split(final TSQuery query, final int index, final TsdbQu
 
     Deferred<Object> rawResolutionDeferred = rawQuery.configureFromQuery(query, index, true);
 
-    long lastAvailableMillis = rollup_query.getLastRollupTimestampSeconds() * 1000L;
-    boolean isRawOnlyQuery = lastAvailableMillis <= getStartTime();
+    long lastRollupTimestampMillis = rollup_query.getLastRollupTimestampSeconds() * 1000L;
 
+    boolean isRawOnlyQuery = QueryUtil.isTimestampBefore(lastRollupTimestampMillis, getStartTime());
     if (!isRawOnlyQuery) {
-      setEndTime(lastAvailableMillis);
-      rawQuery.setStartTime(lastAvailableMillis);
+      updateRollupSplitTimes(rawQuery, lastRollupTimestampMillis);
     }
 
     return rawResolutionDeferred;
   }
 
+  /**
+   * Updates the timestamp of this query and the corresponding raw part in the case of a split.
+   *
+   * Sets the start and end times for this query so that it hits the rollup table until the given timestamp.
+   * Also updates the passed {@param rawQuery} with the new start time so that it hits the raw table for points from the
+   * given timestamp onwards.
+   *
+   * Makes sure that all timestamps are in milliseconds.
+   *
+   * @param rawQuery        The raw query part
+   * @param splitTimestamp  The timestamp until when rollup data is guaranteed to be available
+   */
+  private void updateRollupSplitTimes(final TsdbQuery rawQuery, long splitTimestamp) {
+    setEndTime(splitTimestamp);
+
+    boolean isStartTimeInSeconds = (getStartTime() & Const.SECOND_MASK) == 0;
+    if (isStartTimeInSeconds) {
+      setStartTime(getStartTime() * 1000L);
+    }
+
+    boolean isRawEndTimeInSeconds = (rawQuery.getEndTime() & Const.SECOND_MASK) == 0;
+    if (isRawEndTimeInSeconds) {
+      rawQuery.setEndTime(rawQuery.getEndTime() * 1000L);
+    }
+
+    rawQuery.setStartTime(splitTimestamp);
+  }
+
   @Override
   public Deferred<Object> configureFromQuery(final TSQuery query,
                                              final int index) {

diff --git a/src/query/QueryUtil.java b/src/query/QueryUtil.java
@@ -403,4 +403,24 @@ public static String byteRegexToString(final String regexp) {
     }
     return buf.toString();
   }
+
+  /**
+   * Compares two timestamps where either can be in seconds or milliseconds.
+   *
+   * @param ts1 The first timestamp in either seconds or milliseconds.
+   * @param ts2 The second timestamp in either seconds or milliseconds.
+   * @return Whether the first timestamp is before the second
+   */
+  public static boolean isTimestampBefore(long ts1, long ts2) {
+    boolean ts1InSeconds = (ts1 & Const.SECOND_MASK) == 0;
+    boolean ts2InSeconds = (ts2 & Const.SECOND_MASK) == 0;
+
+    if (ts1InSeconds && !ts2InSeconds) {
+      ts1 *= 1000L;
+    } else if (!ts1InSeconds && ts2InSeconds) {
+      ts2 *= 1000L;
+    }
+
+    return ts1 < ts2;
+  }
 }
diff --git a/test/core/TestSplitRollupQuery.java b/test/core/TestSplitRollupQuery.java
@@ -44,8 +44,8 @@ public void beforeLocal() {
     @Test
     public void setStartTime() {
         queryUnderTest.setStartTime(42L);
-        assertEquals(42L, queryUnderTest.getStartTime());
-        assertEquals(42L, rollupQuery.getStartTime());
+        assertEquals(42000L, queryUnderTest.getStartTime());
+        assertEquals(42000L, rollupQuery.getStartTime());
     }
 
     @Test
@@ -56,7 +56,7 @@ public void setStartTimeBeyondOriginalEnd() {
         rollupQuery.setEndTime(41L);
         queryUnderTest.setStartTime(42L);
 
-        assertEquals(42L, queryUnderTest.getStartTime());
+        assertEquals(42000L, queryUnderTest.getStartTime());
     }
 
     @Test
@@ -67,28 +67,29 @@ public void setStartTimeWithoutRollupQuery() {
 
         queryUnderTest.setStartTime(42L);
 
-        assertEquals(42L, queryUnderTest.getStartTime());
+        assertEquals(42000L, queryUnderTest.getStartTime());
     }
 
     @Test
     public void setEndTime() {
         TsdbQuery rawQuery = new TsdbQuery(tsdb);
         Whitebox.setInternalState(queryUnderTest, "rawQuery", rawQuery);
 
-        rollupQuery.setEndTime(21L);
-        rawQuery.setStartTime(21L);
+        rollupQuery.setStartTime(0);
+        rollupQuery.setEndTime(21000L);
+        rawQuery.setStartTime(21000L);
 
         queryUnderTest.setEndTime(42L);
 
-        assertEquals(42L, queryUnderTest.getEndTime());
-        assertEquals(21L, rollupQuery.getEndTime());
-        assertEquals(42L, rawQuery.getEndTime());
+        assertEquals(42000L, queryUnderTest.getEndTime());
+        assertEquals(21000L, rollupQuery.getEndTime());
+        assertEquals(42000L, rawQuery.getEndTime());
     }
 
     @Test(expected = IllegalArgumentException.class)
     public void setEndTimeBeforeRawStartTime() {
         TsdbQuery rawQuery = new TsdbQuery(tsdb);
-        rawQuery.setStartTime(84L);
+        rawQuery.setStartTime(DateTime.currentTimeMillis());
 
         Whitebox.setInternalState(queryUnderTest, "rawQuery", rawQuery);
 

diff --git a/test/core/TestTsdbQuery.java b/test/core/TestTsdbQuery.java
@@ -53,6 +53,9 @@
 @RunWith(PowerMockRunner.class)
 @PrepareForTest({ DateTime.class, TsdbQuery.class })
 public final class TestTsdbQuery extends BaseTsdbTest {
+
+  private static final long ONE_DAY_MS = 24 * 60 * 60 * 1000;
+
   private TsdbQuery query = null;
 
   @Before
@@ -694,19 +697,25 @@ public void needsSplittingReturnsTrueIfStartAndEndInBlackoutPeriod() {
 
   @Test
   public void split() {
-    mockSystemTime(1356998400000L);
+    long mockSystemTime = 1356998400000L;
+    mockSystemTime(mockSystemTime);
     mockEnableRollupQuerySplitting();
 
     TSQuery tsQuery = getTSQuery();
     TsdbQuery rawQuery = spy(new TsdbQuery(tsdb));
 
-    query.setStartTime(0);
-    rawQuery.setStartTime(42);
+    query.setStartTime(mockSystemTime - 7 * ONE_DAY_MS);
+
     doReturn(Deferred.fromResult(null)).when(rawQuery).configureFromQuery(eq(tsQuery), eq(0), eq(true));
 
     query.split(tsQuery, 0, rawQuery);
 
     verify(rawQuery).configureFromQuery(eq(tsQuery), eq(0), eq(true));
+
+    assertEquals(mockSystemTime - 7 * ONE_DAY_MS, query.getStartTime());
+    assertEquals(mockSystemTime - 2 * ONE_DAY_MS, query.getEndTime());
+    assertEquals(mockSystemTime - 2 * ONE_DAY_MS, rawQuery.getStartTime());
+    assertEquals(mockSystemTime, rawQuery.getEndTime());
   }
 
   @Test(expected = IllegalStateException.class)

diff --git a/test/query/TestQueryUtil.java b/test/query/TestQueryUtil.java
@@ -12,13 +12,17 @@
 // see <http://www.gnu.org/licenses/>.
 package net.opentsdb.query;
 
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import net.opentsdb.core.Query;
+import net.opentsdb.utils.DateTime;
 import org.hbase.async.Bytes.ByteMap;
 import org.hbase.async.FilterList;
 import org.hbase.async.KeyRegexpFilter;
@@ -145,4 +149,20 @@ public void setDataTableScanFilterEnableBoth() throws Exception {
     verify(scanner, times(1)).setStartKey(any(byte[].class));
     verify(scanner, times(1)).setStopKey(any(byte[].class));
   }
+
+  @Test
+  public void timestampComparison() {
+    long now = DateTime.currentTimeMillis() / 1000L;
+    assertTrue(QueryUtil.isTimestampBefore(now*1000, now+1));
+    assertTrue(QueryUtil.isTimestampBefore(now-1, now*1000L));
+    assertTrue(QueryUtil.isTimestampBefore(now-1, now));
+    assertTrue(QueryUtil.isTimestampBefore((now-1)*1000L, now*1000L));
+
+    assertFalse(QueryUtil.isTimestampBefore(now+1, now*1000L));
+    assertFalse(QueryUtil.isTimestampBefore(now*1000L, now-1));
+    assertFalse(QueryUtil.isTimestampBefore(now, now-1));
+    assertFalse(QueryUtil.isTimestampBefore(now*1000L, (now-1)*1000L));
+
+    assertFalse(QueryUtil.isTimestampBefore(now, now));
+  }
 }