apply gcd on fastfield as preprocessing #1418

PSeitz · 2022-07-20T10:41:33Z

No description provided.

fastfield_codecs/src/bitpacked.rs

fastfield_codecs/src/lib.rs

src/fastfield/bytes/mod.rs

src/fastfield/mod.rs

src/fastfield/serializer/mod.rs

src/fastfield/reader.rs

fulmicoton · 2022-07-21T07:45:03Z

src/fastfield/reader.rs

+    pub fn open_from_id(
+        bytes: OwnedBytes,
+        id: u8,
+        gcd: u64,


hmmm so you choose to make gcd somewhat special... Could it have been just another Codec? (well wrapping another codec)

Yes, currently it's its own layer, e.g.

Create: i64 -> convert to u64 -> find/apply gcd -> detect/serialize codec
Read: read u64 from codec -> apply gcd -> convert to i64

When we move it into the codec, every codec needs to be gcd aware.

On the read side, the codecs are already wrapped in FastFieldReaderCodecWrapper. We could have a separate wrapper like FastFieldReaderCodecGCDWrapper, but that would be a performance optimization.

No I was suggesting moving it as one codec.

- codec1 - codec2 - gcd<C: Codec>

I assumed you would do this because it seemed like the natural solution to have:

one dynamic dispatch
as opposed to

having two (dynamic dispatch or if statement).

on the read path that is.

src/fastfield/reader.rs

src/fastfield/serializer/gcd.rs

Co-authored-by: Paul Masurel <[email protected]>

codecov-commenter · 2022-07-22T16:00:55Z

Codecov Report

Merging #1418 (ce8d6b2) into main (23fe73a) will increase coverage by 0.02%.
The diff coverage is 95.87%.

@@            Coverage Diff             @@
##             main    #1418      +/-   ##
==========================================
+ Coverage   94.22%   94.24%   +0.02%     
==========================================
  Files         236      237       +1     
  Lines       43778    44239     +461     
==========================================
+ Hits        41250    41694     +444     
- Misses       2528     2545      +17

Impacted Files	Coverage Δ
fastfield_codecs/src/lib.rs	`96.87% <ø> (ø)`
src/fastfield/reader.rs	`87.80% <81.13%> (-1.28%)`	⬇️
src/fastfield/serializer/mod.rs	`91.48% <93.84%> (+0.18%)`	⬆️
fastfield_codecs/src/bitpacked.rs	`100.00% <100.00%> (ø)`
fastfield_codecs/src/linearinterpol.rs	`99.54% <100.00%> (ø)`
fastfield_codecs/src/multilinearinterpol.rs	`100.00% <100.00%> (ø)`
src/fastfield/gcd.rs	`100.00% <100.00%> (ø)`
src/fastfield/mod.rs	`94.76% <100.00%> (+0.34%)`	⬆️
src/fastfield/multivalued/mod.rs	`97.05% <100.00%> (+0.04%)`	⬆️
src/fastfield/writer.rs	`91.08% <100.00%> (+0.03%)`	⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23fe73a...ce8d6b2. Read the comment docs.

fastfield_codecs/src/multilinearinterpol.rs

src/fastfield/mod.rs

src/fastfield/serializer/gcd.rs

fulmicoton · 2022-07-25T00:43:45Z

src/fastfield/serializer/gcd.rs

+            break;
+        }
+    }
+    let mut gcd = (num1).gcd(num2);


I'm not sure what the gcd crate returns on value 0, but we want gcd(0, n) = n

with the 0-filter this case is covered

src/fastfield/serializer/gcd.rs

src/fastfield/serializer/mod.rs

src/fastfield/serializer/gcd.rs

fulmicoton · 2022-07-25T08:32:52Z

src/fastfield/serializer/gcd.rs

+        }
+        gcd = gcd.gcd(val);
+        if gcd == 1 {
+            return None;


return Some(1);

I think the Option<_> is mostly nice to return None when there are no element. L/ike min for instance.

fulmicoton · 2022-07-25T08:36:44Z

src/fastfield/serializer/gcd.rs

+#[cfg(test)]
+mod tests {
+    use std::collections::HashMap;
+    use std::path::Path;


add unit test for gcd....

in particular, what happens with

[0] -> Some(1) [0, 10]-> Some(10) [10, 0]-> Some(10) [] -> None [15,30,5,10] -> 5 [15,16,10] -> 1 [0,5,5,5] -> 5

src/fastfield/reader.rs

fulmicoton · 2022-07-29T01:33:44Z

src/fastfield/gcd.rs

+        let mut docs = vec![];
+        for i in 1..=num_vals {
+            let val = i as i64 * 1000i64;
+            docs.push(doc!(*FIELDI64=>val));


great you had a i64 test.

src/fastfield/serializer/mod.rs

fulmicoton · 2022-07-29T01:43:28Z

src/fastfield/serializer/mod.rs

+        field_write: &mut CountingWriter<W>,
+        stats: FastFieldStats,
+        fastfield_accessor: impl FastFieldDataAccess,
+        iter1: impl Iterator<Item = u64>,


can you think of a way to ditch iter1/iter2 stuff as well as the Fn() -> impl Iterator stuff? (can be done in a separate ticket)

Would

impl IntoIterator for &SomeType

work? (the reference is Copy)...
or maybe can We afford materializing a Vec?

PSeitz force-pushed the gcd_encoding branch 2 times, most recently from 2f6daa2 to 4592fc7 Compare July 20, 2022 13:12

apply gcd on fastfield as preprocessing

7e032a9

PSeitz force-pushed the gcd_encoding branch from 4592fc7 to 7e032a9 Compare July 20, 2022 14:19