Windowing and general refactoring #11

akshaisarma · 2018-01-25T06:44:20Z

No description provided.

coveralls · 2018-01-25T06:56:32Z

Coverage increased (+0.02%) to 99.534% when pulling 612521e on akshaisarma:3.0-refactor into 443071f on yahoo:master.

hbxie

Nice. I've successfully used it in my spark for windowless cases.
Sent out comments for my first round of looking.

hbxie · 2018-02-01T17:56:06Z

src/main/java/com/yahoo/bullet/aggregations/GroupAll.java

+     * @param config The config that has relevant configs for this strategy. It is unused currently.
     */
-    public GroupAll(Aggregation aggregation) {
+    public GroupAll(Aggregation aggregation, BulletConfig config) {


config is never been used.

Just wanted to keep the constructors the same. Will remove

hbxie · 2018-02-01T18:00:29Z

src/main/java/com/yahoo/bullet/aggregations/GroupAll.java

+
+    @Override
+    public List<BulletRecord> getRecords() {
+        List<BulletRecord> asList = new ArrayList<>();


rename it? it's the same as the Method Arrays.asList

hbxie · 2018-02-01T18:45:04Z

src/main/java/com/yahoo/bullet/aggregations/sketches/Sketch.java

-    protected boolean updated = false;
-    protected boolean unioned = false;
+    // While this class could implement Monoidal, it does not need the full breadth of those methods and it would
+    // receiving data as BulletRecord for one off operations, which is cumbersome.


Agree. Remember to remove it before merging.

Remove what? This comment?

hbxie · 2018-02-01T18:55:03Z

src/main/java/com/yahoo/bullet/aggregations/sketches/DualSketch.java

+ * {@link #collect()} before performing any operations that change data in the sketch such as getting metadata or
+ * results or resetting.
+ */
+public abstract class DualSketch extends Sketch {


Can you add
protected Map<String, Object> addMetadata(Map<String, String> conceptKeys) {
collet();
return super.addMetadata(conceptKeys);
}

So all the subclasses don't need to call collect in their addMetadata method.

I didn't want anything in DualSketch that called collect. Just wanted it to be about the managing metadata related to how collect is idempotent. KMVSketch already does it for Theta and Tuple, so this will only affect QuantileSketch really.

I guess right now the lowest class in the class hierarchy that has a method defined is responsible for calling collect. Maybe it's easier to understand to keep it that way?

hbxie · 2018-02-01T19:10:52Z

src/main/java/com/yahoo/bullet/parsing/Query.java

+     * Default constructor. GSON recommended.
+     */
+    public Query() {
+        filters = null;


Default value is already null?

hbxie · 2018-02-01T19:18:01Z

src/main/java/com/yahoo/bullet/querying/Querier.java

+            timestampKey = config.getAs(BulletConfig.RECORD_INJECT_TIMESTAMP_KEY, String.class);
+        }
+
+        boolean isRateLimited = config.getAs(BulletConfig.RATE_LIMIT_ENABLE, Boolean.class);


s/isRateLimited/isRateLimitEnabled

hbxie · 2018-02-01T19:19:45Z

src/main/java/com/yahoo/bullet/querying/Querier.java

+    private BulletConfig config;
+    private Map<String, String> metaKeys;
+    private String timestampKey;
+    private boolean haveData = false;


s/havaData/hasData

hbxie · 2018-02-02T19:13:46Z

src/main/java/com/yahoo/bullet/aggregations/Raw.java

+
+    @Override
+    public void consume(BulletRecord data) {
+        if (isClosed() || data == null) {


For consume and combine, RAW checks isClosed, but others don't. Is it because no sketch in RAW? It's kinda not consistent with other queries.

Yup, I can remove it. You're right - the inconsistency is weird. Since we made the change to make Querier not do any checking, we should remove it. It was just there as a safety to make sure users, if not honoring isClosed (or isClosedForPartition), don't consume and cause OOM issues.

hbxie · 2018-02-05T20:41:37Z

src/main/java/com/yahoo/bullet/querying/Querier.java

+
+    private boolean timeIsUp() {
+        // Never add to query.getDuration since it can be infinite (Long.MAX_VALUE)
+        return System.currentTimeMillis() - runningQuery.getStartTime() >= runningQuery.getQuery().getDuration();


Could you extract this function to RunningQuery class, and change here as runningQuery.timeIsUp()? Thus we can mock it when writing tests.

Good idea. Will do

hbxie · 2018-02-06T20:46:33Z

src/main/java/com/yahoo/bullet/querying/RunningQuery.java

+     *
+     * @return A boolean denoting whether this query has timed out.
+     */
+    public boolean isTimedOut() {


I think isTimedOut sounds better with RunningQuery.isTimedOut

hbxie · 2018-02-06T20:48:18Z

src/test/java/com/yahoo/bullet/querying/RunningQueryTest.java

+    }
+
+    @Test
+    public void testTimingOut() throws Exception {


Mock System.currentTimeMillis() and get rid of sleep if not too difficult? This test may fail due to system clock problem.

I don't want to use things like Powermock to mock static methods if possible. Any other suggestions? I agree it may fail. By the way, there are multiple tests that use this kind of sleep in the Tumbling window tests.

I don't have better suggestions. Then just leave it as.

…eck for closed

NathanSpeidel · 2018-02-06T19:30:49Z

src/main/java/com/yahoo/bullet/aggregations/Distribution.java

        // No points or if type is QUANTILE, invalid range if the start < 0 or end > 1
-        return points.length < 1 || (type == DistributionType.QUANTILE && (points[0] < 0.0 ||
+        return points.length < 1 || (type == Type.QUANTILE && (points[0] < 0.0 ||
                                                                           points[points.length - 1] > 1.0));


Indentation?

NathanSpeidel · 2018-02-07T20:05:53Z

src/main/java/com/yahoo/bullet/aggregations/Strategy.java

+            makeError("This aggregation type requires at least one field", REQUIRES_FEED_RESOLUTION);
+
+    /**
+     * Returns false if more data will be not be consumed or combined. This method can be used to avoid passing more


NathanSpeidel · 2018-02-07T23:38:33Z

src/main/java/com/yahoo/bullet/aggregations/TopK.java

        int maxMapSize = config.getAs(BulletConfig.TOP_K_AGGREGATION_SKETCH_ENTRIES, Integer.class);
        Number threshold = getThreshold(attributes);
+
+        int maxSize = config.getAs(BulletConfig.AGGREGATION_MAX_SIZE, Integer.class);


Where is this used?

It is not. Removing

NathanSpeidel · 2018-02-08T01:10:57Z

src/main/java/com/yahoo/bullet/aggregations/sketches/QuantileSketch.java

-        return metadata;
+    @Override
+    protected void collectUnionSketch() {
+        merged = unionSketch.getResult();


So here after collect() is called we are not reseting unionSketch and updateSketch. That may produce incorrect results if a user:

calls getResult() (which calls collect())

calls update()

calls getResult() again

In this case all the data that was in the sketch before the first call to getResult() will be "counted" twice since updateSketch was never reset all that data will still be in there and will be merged into "merged" again.

Maybe reset those sketches after collect() is called?

Is this a use case we want to worry about?

(same thing for ThetaSketch)

NathanSpeidel · 2018-02-08T01:47:02Z

src/main/java/com/yahoo/bullet/aggregations/sketches/KMVSketch.java

+        addIfNonNull(metadata, conceptKeys, Concept.SKETCH_THETA, this::getTheta);
        return metadata;
    }



It looks like getStandardDeviations() (below) will give weird standard deviations if it's called before collect() or getResults(). Maybe add a warning about this to the comments for this function?

It's a private method so no one else can call it. Only this class controls when to call it and it calls it after calling merge()

NathanSpeidel · 2018-02-08T21:39:48Z

src/main/java/com/yahoo/bullet/parsing/Window.java

+                                                              "Please set \"type\" to one of: \"TIME\" or \"RECORD\"");
+    public static final BulletError IMPROPER_EVERY = makeError("The \"every\" field was missing or had bad values",
+                                                              "Please set \"every\" to a positive integer");
+    public static final BulletError IMPROPER_INCLUDE = makeError("The \"include\" field has to match \"emit\" or not be set",


Include can be "ALL"... maybe mention that here as well.

NathanSpeidel · 2018-02-08T21:43:45Z

src/main/java/com/yahoo/bullet/parsing/Window.java

+            int minEmitTime = config.getAs(BulletConfig.WINDOW_MIN_EMIT_EVERY, Integer.class);
+            // Clamp upward to minimum
+            if (every.intValue() < minEmitTime) {
+                emit.put(EMIT_EVERY_FIELD, minEmitTime);


Log a warning here?

No, we don't want to log things when we apply configuration. Otherwise, logs will be cluttered with query values being changed (like duration etc).

NathanSpeidel · 2018-02-08T21:51:01Z

src/main/java/com/yahoo/bullet/parsing/Window.java

+
+    @Override
+    public Optional<List<BulletError>> initialize() {
+        if (emitType == null || emitType == Unit.ALL) {


If they omit "window" from the query completely, will this return an error? If window is emitted GSON will parse it to null, then configure calls getUnit() to set emitType, which sets it to null if there is now window? I think I recall us discussing how to handle an omitted "window" optimally, but I don't see that happening here.

If the window is null, the Query class will not call initialize on it.

NathanSpeidel · 2018-02-09T22:46:43Z

src/main/java/com/yahoo/bullet/querying/FilterOperations.java

+    // to t's type causing the stream to be empty, the any/all/none matches will behave as above.
+    // For example:
+    // SOME_LONG_VALUE EQ [1.23, 35.2] will be false
+    // SOME_LONG_VALUE NE [1.23. 425.3] will be false


It looks to me like this case would throw a runtime exception. NE would do (i -> t.compareTo(i)) where "t" is a long. Then TypedObject.compareTo would try to cast the double to a Long and throw a runtime exception?

It would if it were longs but these are actually all strings since our filter values are a list of strings. See the cast function in this class where the arg is a List<String>. In the future, if we want to add bidirectional casting (force the data to the type of the filter value provided by the user) then we'd have to worry about this stuff.

NathanSpeidel · 2018-02-09T23:36:15Z

src/main/java/com/yahoo/bullet/result/Meta.java

+     *                 supplier produces null, it is not added.
+     */
+    public static void addIfNonNull(Map<String, Object> meta, Map<String, String> names, Concept concept,
+                                    Supplier<Object> supplier) {


Why use a Supplier? Why not just pass the object?

Because getting that Object might be expensive and we only want to run the getting of it if the key had a mapping. That is, if the user of this did not want a particular meta key, we shouldn't needlessly run possibly expensive code (like getting Sketch metadata etc).

akshaisarma added 30 commits December 14, 2017 09:29

Temp commit

3a5b842

Moved operations out

7af5721

Moving operations

d381d74

Runner

b978b88

Cleaning up aggregation

64c1830

Working on tests

c6af195

Working on tests

6b72c13

Working on Querier. Isolated and moved tests

f900de0

Fixing Raw to not auto reset

8586cff

Using a DualSketch concept

c1bc4b2

Windowing outline

cfcea96

Broke up Error

fee3016

Starting to move Strategy into Scheme. Closable

8b4391f

Breaking out a Queryable

c89c7eb

Breaking out a Queryable

64c85d5

Calling it monoidal

6cbc66c

Adding windows. Renaming result.Metadata to result.Meta

ae13c25

Fixing aggregation size. Adding rate limiting. Working on docs

c7a1fb0

Fixing defaults. Tweaking windowing

07cb362

Docs and cleanup

5f76a8b

Test fixing

09e1127

Metadata reconciling

ef486b7

Fixing tests and bugs

822ac82

Fixing more tests

5f69729

Fixing FilterOperationsTest

64dcbf0

Pulling out utilities. Refactoring. Fixing more tests

5663721

Tests pass

746a2fa

Sketches, PubSub, Result and Common coverage

4053ada

Refactoring and moving enums into parsing and qualifying them

4346121

Testing aggregations

ec0e3dc

akshaisarma assigned NathanSpeidel and hbxie Jan 25, 2018

akshaisarma requested review from NathanSpeidel and hbxie January 25, 2018 06:44

Fixing checkstyles on test sources

554c3a7

akshaisarma added 4 commits January 26, 2018 07:41

Making sure Query is readonly after configure

6f9917d

Making initialize not auto called for querier

7e05b39

Fixing dropped coverage by removing unneeded method

fd46d7a

Fixing concept name. Making the check for isClosed not done on consume

b3bd480

hbxie suggested changes Feb 1, 2018

View reviewed changes

Review changes

f3be104

hbxie reviewed Feb 2, 2018

View reviewed changes

akshaisarma added 3 commits February 2, 2018 11:28

Removing Raw closed check for consuming and combining

7c448c0

Validating after merging

d021975

Fixing docs for Querier

705b0bc

hbxie reviewed Feb 5, 2018

View reviewed changes

Moving time checking to RunningQuery

fd8a39f

hbxie reviewed Feb 6, 2018

View reviewed changes

akshaisarma added 4 commits February 6, 2018 19:07

Adding a way to get the original validator

50bfe6f

Dual Sketch bug fix

c5dfd1b

Fixing bad javadoc

8b7a84a

Making all windows only be closed on their criteria. Restoring RAW ch…

ba4e7b7

…eck for closed

NathanSpeidel requested changes Feb 10, 2018

View reviewed changes

akshaisarma added 2 commits February 9, 2018 19:38

Removing the forcing of windows. Adding new errors

bca724e

Review comments

612521e

NathanSpeidel approved these changes Feb 13, 2018

View reviewed changes

hbxie approved these changes Feb 13, 2018

View reviewed changes

akshaisarma merged commit b60686e into bullet-db:master Feb 13, 2018

akshaisarma deleted the 3.0-refactor branch February 13, 2018 21:56

Windowing and general refactoring #11

Windowing and general refactoring #11

Uh oh!

Conversation

akshaisarma commented Jan 25, 2018

Uh oh!

coveralls commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hbxie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akshaisarma Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 25, 2018 •

edited

Loading

akshaisarma Feb 10, 2018 •

edited

Loading

akshaisarma Feb 10, 2018 •

edited

Loading