enhancement(reduce transform): Add additional merge strategies #8559

dbcfd · 2021-08-02T17:18:28Z

Adds additional merge strategies: longest, shortest, retain, unique.

This issue provides additional support to reduce in lieu of #8036 and #4258 being closed.

netlify · 2021-08-02T17:18:34Z

✔️ Deploy Preview for vector-project canceled.

🔨 Explore the source changes: 67b5013

🔍 Inspect the deploy log: https://app.netlify.com/sites/vector-project/deploys/6131327067085d00072ffd62

spencergilbert

Thanks for the contribution! I've left a couple initial comments, and it also appears to need a make fmt 👍

src/transforms/reduce/merge_strategy.rs

jszwedko

Nice! Thanks for these additions @dbcfd . I think the added merge strategies make sense.

I left a couple of comments. make check-clippy also has some linter complaints as highlighted by CI.

src/transforms/reduce/merge_strategy.rs

lib/vector-core/src/event/value.rs

blt · 2021-08-06T21:26:09Z

lib/vector-core/src/event/value.rs

+            }
+            Value::Float(v) => {
+                if v.is_finite() {
+                    format!("{}", v).hash(state);


I think it's reasonable to expect that Hash for Value will be called in hot paths. Ignoring concerns about the hashability of floats I am opposed to having format! here, considering it implies allocation.

blt · 2021-08-06T21:29:04Z

src/transforms/reduce/merge_strategy.rs

+
+impl UniqueMerger {
+    fn new(v: Value) -> Self {
+        let mut h = HashSet::default();


Small note that you might want to consider parameterizing over a different hash function than default.

Any suggestion on which hash function?

IIRC we already have twox-hash in the project but the exact choice would need to be guided by a benchmark. I'd be satisfied with an issue to follow up on that and you're welcome to assign it to me.

dbcfd · 2021-08-17T17:06:12Z

@jszwedko @blt I've adjusted the hashing functions, let me know if that looks correct. The dependency issues are also pulled out into a single commit to be pulled to another PR if need be.

jszwedko

Thanks for the updates @dbcfd ! I left a few last comments, but nothing major. Thanks for this work!

jszwedko · 2021-08-17T21:08:42Z

lib/vector-core/src/event/value.rs

+                if v.is_finite() {
+                    let trunc: u64 = unsafe { std::mem::transmute(v.trunc()) };
+                    if trunc == 0 {
+                        v.is_sign_negative().hash(state);
+                    }
+                    trunc.hash(state);


Could we add a comment to this match arm with how we are doing the hashing? Something like:

// This hashes floats with the following rules: // * NaNs hash as equal // * -0 and +0 hash to different values // * otherwise transmute to u64 and hash

This one, in particular, makes me think we should have some tests around this hash function. Would you mind adding a few unit tests here?

src/transforms/reduce/merge_strategy.rs

jszwedko · 2021-08-17T21:13:01Z

Cargo.toml

-nom = { version = "6.1.2", default-features = false, optional = true }
+#https://github.com/wyyerd/pulsar-rs/issues/167
+nom = { version="=7.0.0-alpha1", default-features = false, features=["alloc"], optional = true}
+#nom = { version = "6.1.2", default-features = false, optional = true }


I'm a little surprised this brought in a nom update. Did you mean to?

Also is the indexmap change needed? I do know we have another PR that pins it too due to the circular dependency, but it doesn't seem like this PR does?

I can probably get rid of it by reverting the lock and not running cargo update.

dbcfd · 2021-08-18T15:02:07Z

@jszwedko Should be good fore review again

jszwedko

Thanks for the updates @dbcfd! I left a couple last comments.

jszwedko · 2021-08-18T15:08:31Z

lib/vector-core/src/event/value.rs

+            assert_ne!(hash(Value::Boolean(true)), hash(Value::Integer(2)));
+            assert_eq!(hash(Value::Float(1.2)), hash(Value::Float(1.4)));
+            assert_ne!(hash(Value::Float(-0.0)), hash(Value::Float(0.0)));
+            assert_eq!(hash(Value::Float(f64::NEG_INFINITY)), hash(Value::Float(f64::INFINITY)));


I neglected infinity 😄 . I think maybe we should have these hash to different values?

jszwedko · 2021-08-18T15:10:55Z

website/cue/reference/components/transforms/reduce.cue

+								longest:        "Retains the longest array seen"
+								shortest:       "Retains the shortest array seen"


This may be a bit pedantic, but could we call these longest_array and shortest_array? I could also see people thinking they could be used with strings otherwise.

jszwedko · 2021-08-18T15:13:51Z

@dbcfd also, could you satisfy the DCO check here? https://github.com/timberio/vector/pull/8559/checks?check_run_id=3362512094 It looks like some of the commits are unsigned. There's more info about this requirement here: https://github.com/timberio/vector/blob/master/docs/CONTRIBUTING.md#dco

dbcfd · 2021-08-18T16:49:41Z

@jszwedko Fixing DCO went a little weird, but it should be fixed now. Most recent commit for review feedback was 6a57a5c. Did a final fmt after that.

Signed-off-by: dbcfd <bdbrowning2@gmail.com>

jszwedko

Awesome, thanks for making these final changes. This looks good to me!

blt

There's one clippy ding that needs resolved and/or ignored but I'm happy with this.

dbcfd · 2021-08-18T20:50:26Z

@blt There's 2 clippy dings, one for deriving PartialEq, and one for the transmute. I'll do the transmute one, but should I also implement PartialEq (seems weird since it's derivable), or just do an allow on it?

jszwedko · 2021-08-18T21:24:27Z

@blt There's 2 clippy dings, one for deriving PartialEq, and one for the transmute. I'll do the transmute one, but should I also implement PartialEq (seems weird since it's derivable), or just do an allow on it?

I think we should implement PartialEq here too. I agree with clippy that it would be confusing if they don't match. Things that hash to the same value are not considered "partially equal" (admitting there may be hash collisions).

blt · 2021-08-18T21:33:48Z

@blt There's 2 clippy dings, one for deriving PartialEq, and one for the transmute. I'll do the transmute one, but should I also implement PartialEq (seems weird since it's derivable), or just do an allow on it?

Oh, I missed one and only caught the PartialEq. The ding is this one https://rust-lang.github.io/rust-clippy/v0.0.212/#derive_hash_xor_eq. It's probably wise to go ahead and implement PartialEq since someone could accidentally break k1 == k2 -> hash(k1) == hash(k2) without being aware of it, if there's not an explicit implementation to look at.

dbcfd · 2021-08-24T16:42:46Z

@jszwedko @blt Implemented partial eq which should match the hashing logic. Mind taking a look again?

jszwedko

This looks good to me, thanks for this contribution @dbcfd !

jszwedko · 2021-08-24T21:00:20Z

@dbcfd sorry, I realized the DCO check is failing again 😅 . Do you mind signing the unsigned commits?

Signed-off-by: dbcfd <bdbrowning2@gmail.com>

dbcfd · 2021-08-24T23:01:32Z

@jszwedko signed now :D Maybe someday I'll remember to that.

Signed-off-by: dbcfd <bdbrowning2@gmail.com>

dbcfd · 2021-09-02T13:20:36Z

@jszwedko @blt There's another clippy error, but it looks to be spurious (mutable key type). Is this good to set an ignore on?

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

resolved

jszwedko · 2021-09-02T20:23:44Z

@dbcfd yeah, that appears to be a false positive due to Value::Bytes(bytes::Bytes). I pushed a commit adding the clippy allow. Thanks again for this work! It should get merged soon when CI passes.

* Add additional merge strategies: longest, shortest, retain, unique Signed-off-by: dbcfd <bdbrowning2@gmail.com> * Fix tests, fmt Signed-off-by: dbcfd <bdbrowning2@gmail.com> * Fix tests, adjust float hashing Signed-off-by: dbcfd <bdbrowning2@gmail.com> * Dependency fixes Signed-off-by: dbcfd <bdbrowning2@gmail.com> * Error for shortest/longest when not array Signed-off-by: dbcfd <bdbrowning2@gmail.com> * Revert dependency changes Signed-off-by: dbcfd <bdbrowning2@gmail.com> * Review changes Signed-off-by: dbcfd <bdbrowning2@gmail.com> * Revert nom change Signed-off-by: dbcfd <bdbrowning2@gmail.com> * Revert build pin Signed-off-by: dbcfd <bdbrowning2@gmail.com> * Rename strategy to indicate it is for array, handle infinity properly Signed-off-by: dbcfd <bdbrowning2@gmail.com> * fmt Signed-off-by: dbcfd <bdbrowning2@gmail.com> * Fix clippy warnings by implementing PartialEq and using bits Signed-off-by: dbcfd <bdbrowning2@gmail.com> * Another clippy fix Signed-off-by: dbcfd <bdbrowning2@gmail.com>

dbcfd mentioned this pull request Aug 2, 2021

Drop Vector WASM support #8036

Closed

3 tasks

spencergilbert changed the title ~~enhancement (reduce): Add additional merge strategies~~ enhancement(reduce transform): Add additional merge strategies Aug 3, 2021

spencergilbert added the transform: reduce Anything `reduce` transform related label Aug 3, 2021

spencergilbert previously requested changes Aug 3, 2021

View reviewed changes

src/transforms/reduce/merge_strategy.rs Outdated Show resolved Hide resolved

src/transforms/reduce/merge_strategy.rs Outdated Show resolved Hide resolved

spencergilbert requested review from jszwedko and spencergilbert August 6, 2021 12:44

jszwedko reviewed Aug 6, 2021

View reviewed changes

blt suggested changes Aug 6, 2021

View reviewed changes

jszwedko reviewed Aug 17, 2021

View reviewed changes

jszwedko reviewed Aug 18, 2021

View reviewed changes

dbcfd force-pushed the more-merge-strategies branch 3 times, most recently from 3b824e7 to 6a57a5c Compare August 18, 2021 16:44

dbcfd added 11 commits August 18, 2021 10:54

Add additional merge strategies: longest, shortest, retain, unique

9613418

Signed-off-by: dbcfd <bdbrowning2@gmail.com>

Fix tests, fmt

0fa4393

Signed-off-by: dbcfd <bdbrowning2@gmail.com>

Fix tests, adjust float hashing

e8eb2fb

Signed-off-by: dbcfd <bdbrowning2@gmail.com>

Dependency fixes

8a8f00f

Signed-off-by: dbcfd <bdbrowning2@gmail.com>

Error for shortest/longest when not array

9e45f7f

Signed-off-by: dbcfd <bdbrowning2@gmail.com>

Revert dependency changes

b27efe7

Signed-off-by: dbcfd <bdbrowning2@gmail.com>

Review changes

7106ae7

Signed-off-by: dbcfd <bdbrowning2@gmail.com>

Revert nom change

9a879ff

Signed-off-by: dbcfd <bdbrowning2@gmail.com>

Revert build pin

3975648

Signed-off-by: dbcfd <bdbrowning2@gmail.com>

Rename strategy to indicate it is for array, handle infinity properly

e1713ee

Signed-off-by: dbcfd <bdbrowning2@gmail.com>

fmt

42a8693

Signed-off-by: dbcfd <bdbrowning2@gmail.com>

dbcfd force-pushed the more-merge-strategies branch from 6ee94a7 to 42a8693 Compare August 18, 2021 16:55

jszwedko approved these changes Aug 18, 2021

View reviewed changes

jszwedko enabled auto-merge (squash) August 18, 2021 19:55

blt approved these changes Aug 18, 2021

View reviewed changes

auto-merge was automatically disabled August 24, 2021 16:41
Head branch was pushed to by a user without write access

jszwedko approved these changes Aug 24, 2021

View reviewed changes

Fix clippy warnings by implementing PartialEq and using bits

e846eee

Signed-off-by: dbcfd <bdbrowning2@gmail.com>

dbcfd force-pushed the more-merge-strategies branch from 6bbb2d5 to e846eee Compare August 24, 2021 23:00

Another clippy fix

35d8b57

Signed-off-by: dbcfd <bdbrowning2@gmail.com>

jszwedko enabled auto-merge (squash) August 25, 2021 00:12

Supress false positive mutable_key_type

67b5013

Signed-off-by: Jesse Szwedko <jesse@szwedko.me>

jszwedko merged commit 0f95401 into vectordotdev:master Sep 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhancement(reduce transform): Add additional merge strategies #8559

enhancement(reduce transform): Add additional merge strategies #8559

dbcfd commented Aug 2, 2021

netlify bot commented Aug 2, 2021 •

edited

spencergilbert left a comment •

edited

jszwedko left a comment

blt Aug 6, 2021

blt Aug 6, 2021

dbcfd Aug 17, 2021

blt Aug 17, 2021

dbcfd commented Aug 17, 2021

jszwedko left a comment

jszwedko Aug 17, 2021

jszwedko Aug 17, 2021

jszwedko Aug 17, 2021

dbcfd Aug 17, 2021

dbcfd commented Aug 18, 2021

jszwedko left a comment

jszwedko Aug 18, 2021

jszwedko Aug 18, 2021

jszwedko commented Aug 18, 2021

dbcfd commented Aug 18, 2021

jszwedko left a comment

blt left a comment

dbcfd commented Aug 18, 2021

jszwedko commented Aug 18, 2021

blt commented Aug 18, 2021

dbcfd commented Aug 24, 2021

jszwedko left a comment

jszwedko commented Aug 24, 2021

dbcfd commented Aug 24, 2021

dbcfd commented Sep 2, 2021

jszwedko commented Sep 2, 2021 •

edited

		longest: "Retains the longest array seen"
		shortest: "Retains the shortest array seen"

enhancement(reduce transform): Add additional merge strategies #8559

enhancement(reduce transform): Add additional merge strategies #8559

Conversation

dbcfd commented Aug 2, 2021

netlify bot commented Aug 2, 2021 • edited

spencergilbert left a comment • edited

Choose a reason for hiding this comment

jszwedko left a 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

dbcfd commented Aug 17, 2021

jszwedko left a 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

dbcfd commented Aug 18, 2021

jszwedko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jszwedko commented Aug 18, 2021

dbcfd commented Aug 18, 2021

jszwedko left a comment

Choose a reason for hiding this comment

blt left a comment

Choose a reason for hiding this comment

dbcfd commented Aug 18, 2021

jszwedko commented Aug 18, 2021

blt commented Aug 18, 2021

dbcfd commented Aug 24, 2021

jszwedko left a comment

Choose a reason for hiding this comment

jszwedko commented Aug 24, 2021

dbcfd commented Aug 24, 2021

dbcfd commented Sep 2, 2021

jszwedko commented Sep 2, 2021 • edited

netlify bot commented Aug 2, 2021 •

edited

spencergilbert left a comment •

edited

jszwedko commented Sep 2, 2021 •

edited