quickcheck zebra reading #499

tranma · 2017-02-21T07:02:01Z

This adds a test that just savages some random Icicle facts to Zebra, reads them into an input struct, and peeks the result.

depends on icicle-lang/zebra-ambiata#64

! @jystic @amosr
/jury approved @jystic @amosr

jacobstanley · 2017-02-21T07:20:04Z

icicle-compiler/test/Icicle/Test/Sea/Zebra.hs

+          (bools, ls, rs) <- List.unzip3 <$> mapM (fromVSum (tableOf l) (tableOf r)) rows
+          lcols <- mergeValues l ls
+          rcols <- mergeValues r rs
+          pure $ [ Zebra.IntColumn (Storable.fromList bools) ] <> lcols <> rcols


jacobstanley · 2017-02-21T07:20:56Z

icicle-compiler/test/Icicle/Test/Sea/Zebra.hs

+
+        SumT ErrorT t -> do
+          (bools, vs) <- List.unzip <$> mapM (fromVSumError (tableOf t)) rows
+          pure $ [ Zebra.IntColumn (Storable.fromList bools) ] <> concat vs


this is how icicle does it, but not sure that this specialisation should end up in a zebra file

should probably be (tags, errs, vs) instead?

or just get rid of this case and let SumT handle it maybe

jacobstanley · 2017-02-21T07:22:26Z

icicle-compiler/test/test.hs

@@ -65,6 +66,7 @@ main
        , Icicle.Test.Avalanche.Melt.tests

        , Icicle.Test.Sea.Psv.tests
+--        , Icicle.Test.Sea.Zebra.tests


not enabled?

jacobstanley · 2017-02-21T07:28:13Z

It might be easier to convert the icicle type to a Schema then convert the icicle value to a zebra Value, then you can use:

fromRow :: Schema -> Value -> Either (TableError Schema) (Table Schema)

might be a safer test because otherwise you're testing your encoding of tables vs the icicle generated code.

tranma · 2017-02-21T07:30:04Z

true, that is less savage actually -- I'll convert to a Schema

tranma · 2017-02-27T03:29:54Z

using zebra Schema now

jacobstanley · 2017-02-27T04:51:04Z

icicle-compiler/test/Icicle/Test/Arbitrary/Core.hs

+         , pure TimeT
+         , pure StringT
+         , StructT <$> arbitrary
+         , OptionT <$> arbitrary


the stuff in the "small" list should probably be non-recursive, otherwise you might generate OptionT (OptionT (OptionT etc and never terminate

right ok. This was because I wanted more structs

they should all be equally likely except when the size is 1 or less

jacobstanley · 2017-02-28T21:51:32Z

icicle-compiler/test/Icicle/Test/Sea/Zebra.hs

+                 -- iprogram: { *mempool, input, ... }
+                 -- input: { *chord_time, *fact_count, *tombstone, *input_start, ... }
+                 programs0_ptr <- peekWordOff fleet_ptr 4
+                 tombstones_ptr <- peekWordOff programs0_ptr 3


Some serious pointer twiddling :D

jacobstanley · 2017-02-28T21:52:48Z

icicle-compiler/test/Icicle/Test/Sea/Zebra.hs

+    pure Schema.Int
+
+  UnitT ->
+    Nothing


This can be mapped to Schema.Struct Boxed.empty

jacobstanley · 2017-02-28T21:57:28Z

icicle-compiler/test/Icicle/Test/Sea/Zebra.hs

+            Schema.Field (Schema.FieldName (nameOfStructField f)) <$> schemaOfType t
+        in Schema.Struct . Boxed.fromList <$> mapM fieldOf (Map.toList fields)
+
+  _ -> Nothing


I think we should be able to map all of ValType to zebra? probably doesn't matter for a test I guess

this is a savage way to let you comment out cases for debugging :D

jacobstanley · 2017-02-28T22:00:24Z

icicle-compiler/test/Icicle/Test/Sea/Zebra.hs

+    pure . Zebra.Double $ x
+
+  VUnit ->
+    pure . Zebra.Int $ 0


same as above

jacobstanley · 2017-02-28T22:01:29Z

icicle-compiler/test/Icicle/Test/Sea/Zebra.hs

+    pure . Zebra.Int . fromIntegral . Icicle.packedOfTime $ x
+
+  VString x ->
+    pure . Zebra.ByteArray . ByteString.pack . Text.unpack $ x


s/ByteString.pack . Text.unpack/Data.Text.Encoding.encodeUtf8/ ?

jacobstanley · 2017-02-28T22:02:32Z

icicle-compiler/test/Icicle/Test/Sea/Zebra.hs

+
+  VNone
+    | OptionT t <- ty
+    -> Zebra.Enum 0 <$> zebraOfValue t (defaultOfType t)


This should be
Zebra.Enum 0 (Zebra.Struct Boxed.empty)
or
Zebra.Enum 0 <$> zebraOfValue UnitT VUnit

jacobstanley · 2017-02-28T22:04:18Z

icicle-compiler/test/Icicle/Test/Sea/Zebra.hs

+  OptionT t ->
+    let
+      noneOf x =
+        Schema.Variant (Schema.VariantName "none") <$> schemaOfType x


This should be:
Schema.Variant (Schema.VariantName "none") (Schema.Struct Boxed.empty)
or
Schema.Variant (Schema.VariantName "none") <$> schemaOfType UnitT

jacobstanley · 2017-02-28T22:05:32Z

icicle-compiler/test/Icicle/Test/Sea/Zebra.hs

+    -> Zebra.Array . Boxed.fromList <$> mapM (zebraOfValue t) xs
+
+  VFactIdentifier x ->
+    pure . Zebra.Int . fromIntegral . getFactIdentifierIndex $ x


This case (fact identifier) is missing from schemaOfType

jacobstanley · 2017-02-28T22:15:05Z

icicle-compiler/test/Icicle/Test/Sea/Zebra.hs

+    v' <- schemaOfType' v
+    pure . Schema.Struct . Boxed.fromList $
+      [ Schema.Field (Schema.FieldName "keys") (Schema.Array k')
+      , Schema.Field (Schema.FieldName "vals") (Schema.Array v') ]


This is cool for a test, but maybe we want Array (Struct [("key", k'), ("val", v')]) for production? not sure just wanted to bring it up for discussion. I can imagine when zebra is pretty printed having the key/value pair together would be useful.

jacobstanley · 2017-02-28T22:16:51Z

icicle-compiler/test/Icicle/Test/Sea/Zebra.hs

+    | StructT struct <- ty
+    , types <- getStructType struct
+    -> do let vs = Map.elems (Map.intersectionWith (,) types xs)
+          Zebra.Struct . Boxed.fromList <$> mapM (uncurry zebraOfValue) vs


Might be good to have a guard saying length vs == length types or similar

jacobstanley · 2017-02-28T22:20:00Z

💯 for the test

👍 after we resolve the mapping from icicle types to zebra, happy to discuss IRL, maybe it's not so important, but I can see that testing stuff becoming the basis for our production mapping

amosr · 2017-03-01T03:52:44Z

data/sea/54-zebra.h

@@ -155,7 +156,7 @@ static int64_t zebra_translate_column
                istring_t *strings = anemone_mempool_alloc (mempool, count * sizeof (istring_t));
                int offset = 1;
                for (int i = 0; i != count; ++i) {
-                    int64_t index = i + elem_start;
+                    int64_t index = i;


I'm not sure about this. I think this should include the elem_start, but that elem_start should not have added += len below, and also the recursive call should use start instead of elem_start.

the elem_start refers to the current level - that is the indices/lengths array. then start refers to the nested values, so you need to use it when calling zebra_translate_table

I think this code would pass as long as elem_start = 0 for nested arrays (strings), but not when you start splitting nested arrays up with max rows

does that make sense?

yes, I think I did that in the other branch -- (reading zebra facts in chunk)

like this?

int64_t index = i + elem_start; int64_t start = data.a.n[index] - data.a.n[0]; int64_t len = data.a.n[index+1] - data.a.n[index]; - offset = zebra_translate_table (mempool, elem_start, len, (void**)(strings + i), &data.a.table); - elem_start += len; + offset = zebra_translate_array (mempool, start, len, (void**)(strings + i), &data);

example probably better

nested array of strings ["beef", "helps", "keep", "order"] lengths: [4, 5, 4, 5] scan (data.a.n): [0, 4, 9, 13, 18] values (data.a.table): "beefhelpskeeporder" zebra_translate_table(elem_start = 0, count = 1, table) = ["beef"] zebra_translate_table(elem_start = 0, count = 2, table) = ["beef", "helps"] zebra_translate_table(elem_start = 2, count = 2, table) = ["keep", "order"]

the last one is the most interesting because it needs to start looking at scan array 2 and 3 (data.a.n), so to pull out the value "beef" it looks up scan[2] = 9 for the start index, and scan[3] = 13 for the end index, meaning the length is 13-9=4.

sorry I wrote that example comment above before I saw your reply

amosr · 2017-03-01T04:06:10Z

icicle-compiler/src/Icicle/Sea/FromAvalanche/Type.hs

+auxillary :: [ValType] -> [ValType]
+auxillary [] = []
+auxillary (t@(BufT _ a) : ts) = (ArrayT a : t : auxillary ts)
+auxillary (t : ts) = t : auxillary ts


silly question, no chance of bufs inside other types at this point?

amosr · 2017-03-01T04:25:49Z

icicle-compiler/test/Icicle/Test/Sea/Zebra.hs

+                        e ->
+                          pure (VLeft (VError e))
+
+                     peekInputs (x:xs) (index - 1)


this peekInputs might be worth moving in to sea eval, wherever peekOutput is

amosr · 2017-03-01T04:29:26Z

🥇 wow

tranma · 2017-03-06T00:36:02Z

I'm going to rebase and merge this, ignoring the unmelted value failure (see #500)

…read

tranma added 2 commits February 21, 2017 11:33

Psv: move constants (max_row_count, buffer_size, etc.) into config

f55e317

Test: reading random zebra

4117fe2

ambiata-ci assigned amosr and jacobstanley Feb 21, 2017

jacobstanley reviewed Feb 21, 2017

View reviewed changes

tranma force-pushed the topic/input-test branch 2 times, most recently from a4dce76 to 566607b Compare February 27, 2017 03:29

tranma force-pushed the topic/input-test branch 2 times, most recently from 4cb4399 to 4079cfe Compare February 27, 2017 03:38

jacobstanley reviewed Feb 27, 2017

View reviewed changes

tranma force-pushed the topic/input-test branch 4 times, most recently from 732b087 to dac0214 Compare February 28, 2017 05:12

jacobstanley reviewed Feb 28, 2017

View reviewed changes

icicle-compiler/test/Icicle/Test/Sea/Zebra.hs Outdated

pure . Zebra.Double $ x

VUnit ->

pure . Zebra.Int $ 0

Copy link

Contributor

jacobstanley Feb 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

jacobstanley reviewed Feb 28, 2017

View reviewed changes

tranma added 3 commits March 1, 2017 13:54

Test: convert Icicle type to Zebra schema

b142fcc

Sea: make array_t when buf_t is used

6a2e73c

Test: fail hard when generating a program with an unmelted type

b45b766

tranma force-pushed the topic/input-test branch from 45a9de5 to b45b766 Compare March 1, 2017 02:54

amosr reviewed Mar 1, 2017

View reviewed changes

tranma added 3 commits March 6, 2017 11:44

Test: restrict input type to what is currently implemented for zebra …

dd9d2b5

…read

Test: update zebra to schema-in-header which includes variant fix

9a2d92a

Update aeson and zebra

e8fdcc4

tranma force-pushed the topic/input-test branch from 2e31527 to e8fdcc4 Compare March 6, 2017 00:44

tranma merged commit d515064 into master Mar 6, 2017

tranma deleted the topic/input-test branch March 6, 2017 02:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quickcheck zebra reading #499

quickcheck zebra reading #499

tranma commented Feb 21, 2017 •

edited by ambiata-ci

Loading

jacobstanley Feb 21, 2017

jacobstanley Feb 21, 2017

jacobstanley Feb 21, 2017

jacobstanley Feb 21, 2017

jacobstanley commented Feb 21, 2017

tranma commented Feb 21, 2017

tranma commented Feb 27, 2017

jacobstanley Feb 27, 2017

tranma Feb 27, 2017

jacobstanley Feb 27, 2017

jacobstanley Feb 28, 2017

jacobstanley Feb 28, 2017

jacobstanley Feb 28, 2017

tranma Mar 1, 2017

jacobstanley Feb 28, 2017

jacobstanley Feb 28, 2017

jacobstanley Feb 28, 2017 •

edited

Loading

jacobstanley Feb 28, 2017

jacobstanley Feb 28, 2017

jacobstanley Feb 28, 2017

jacobstanley Feb 28, 2017

jacobstanley commented Feb 28, 2017

amosr Mar 1, 2017 •

edited

Loading

tranma Mar 1, 2017

tranma Mar 1, 2017

amosr Mar 1, 2017

amosr Mar 1, 2017

amosr Mar 1, 2017

amosr Mar 1, 2017

amosr commented Mar 1, 2017

tranma commented Mar 6, 2017

quickcheck zebra reading #499

quickcheck zebra reading #499

Conversation

tranma commented Feb 21, 2017 • edited by ambiata-ci Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobstanley commented Feb 21, 2017

tranma commented Feb 21, 2017

tranma commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobstanley Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobstanley commented Feb 28, 2017

amosr Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amosr commented Mar 1, 2017

tranma commented Mar 6, 2017

tranma commented Feb 21, 2017 •

edited by ambiata-ci

Loading

jacobstanley Feb 28, 2017 •

edited

Loading

amosr Mar 1, 2017 •

edited

Loading