Parse nodrop #49

akdor1154 · 2019-01-10T13:30:45Z

Implement nodrop option for parse, which leaves non-matching lines in the logs with the parsed fields set to None.

Code comments welcome however trivial, I'm quite new to Rust.

tstack

Would be good to add an integration test.

[Russell, editing @tstack's review comment] See integration.rs & the structured_test! macro

tstack · 2019-01-10T15:16:50Z

src/typecheck.rs

            } => {
                let regex = pattern.to_regex();

+                let options: HashSet<lang::ParseOption> = parse_options.iter().cloned().collect();


Is this needed if you're only checking if the vector contains the flag or not?

tstack · 2019-01-10T15:19:10Z

src/typecheck.rs

@@ -81,6 +85,9 @@ impl lang::InlineOperator {
                        regex,
                        fields,
                        input_column.map(|e| e.into()),
+                        operator::ParseOptions {
+                            drop_nonmatching: options.contains(&lang::ParseOption::NoDrop)


Is this condition backwards? drop_nonmatching will be true if there's a "nodrop" option.

tstack · 2019-01-10T15:20:07Z

src/operator.rs

@@ -600,7 +612,16 @@ impl UnaryPreAggFunction for Parse {
    fn process(&self, rec: Record) -> Result<Option<Record>, EvalError> {
        let matches = self.matches(&rec)?;
        match matches {
-            None => Ok(None),
+            None => match self.options.drop_nonmatching {


I think the rust style is to use an if-expression instead of matching on a boolean.

I'd probably use tuple matching and do something like

match (self.options,drop_nonmatching, matches) { None, true => None, None, false => addFields(rec) Some(rec), _ => rec }

tstack · 2019-01-10T15:21:32Z

src/operator.rs

+                    let rec = self
+                        .fields
+                        .iter()
+                        .fold(rec, |rec, field| rec.put(field, data::Value::None));


Maybe make 'rec' mutable and use for_each() ? Not sure what the style is...

this is OK as is. This is probably a little more readable though: (as @tstack mentioned, you need mut here)

for field <- fields { rec = rec.put(field, data::Value::None) }

Another totally fine option is adding a put_all method on rec to handle this case

rcoh

Awesome! A few small tweaks to simplify but the overall direction is good. I'm going to land @tstack's #44 land first because these definitely conflict, but it will much easier to resolve conflicts in yours than in their's.

As a side note, it looks like the CI job failed because you need to run cargo fmt. I'll add an improved error for that for new contributors.

rcoh · 2019-01-10T17:39:46Z

src/lang.rs

@@ -93,6 +93,7 @@ pub enum InlineOperator {
        pattern: Keyword,
        fields: Vec<String>,
        input_column: Option<Expr>,
+        parse_options: Vec<ParseOption>,


lets just have this be a boolean no_drop -- I appreciate the forward thinking but might as well just keep it simple for now. It will simplify the downstream code as well

rcoh · 2019-01-10T17:41:53Z

src/operator.rs

@@ -600,7 +612,16 @@ impl UnaryPreAggFunction for Parse {
    fn process(&self, rec: Record) -> Result<Option<Record>, EvalError> {
        let matches = self.matches(&rec)?;
        match matches {
-            None => Ok(None),
+            None => match self.options.drop_nonmatching {


I'd probably use tuple matching and do something like

match (self.options,drop_nonmatching, matches) { None, true => None, None, false => addFields(rec) Some(rec), _ => rec }

rcoh · 2019-01-10T17:44:48Z

src/operator.rs

+                    let rec = self
+                        .fields
+                        .iter()
+                        .fold(rec, |rec, field| rec.put(field, data::Value::None));


this is OK as is. This is probably a little more readable though: (as @tstack mentioned, you need mut here)

for field <- fields { rec = rec.put(field, data::Value::None) }

Another totally fine option is adding a put_all method on rec to handle this case

rcoh · 2019-01-10T17:46:22Z

src/operator.rs

@@ -931,6 +952,9 @@ mod tests {
                "length".to_string(),
            ],
            None,
+            ParseOptions {
+                drop_nonmatching: true,


as evidenced by getting it backwards, this name is kind of confusing. Lets just call it no_drop to be parallel with lang

as evidenced by getting it backwards, this name is kind of confusing. Lets just call it no_drop to be parallel with lang

I was actually going for the "don't have negated variable/settings names" rule - let me know if you still want to change it after the other changes, it's a bit clearer now.

I might add unit tests in typecheck.ts as well, as that would have caught this brainfart before the integration test.

I love the "don't have negated variable names" rule!

rcoh · 2019-01-10T23:35:29Z

src/operator.rs

+            (None, false) => {
+                let mut rec = rec;
+                for field in &self.fields {
+                    rec = rec.put(field, data::Value::None);


thinking a bit more, we should only do this if the field doesn't already exist in the record

rcoh

Looks good, see one inline comment. Rebase from latest master to pick up the parser changes (happy to help if you get sticky conflicts)

rcoh · 2019-01-11T00:31:55Z

Did the merge in the github online view, we'll see if I got it right 😅

akdor1154 · 2019-01-11T00:43:04Z

haha you're game. I can rebase, was just waiting until i got to a non-windows machine to do it on.

tstack reviewed Jan 10, 2019

View reviewed changes

rcoh requested changes Jan 10, 2019

View reviewed changes

rcoh reviewed Jan 10, 2019

View reviewed changes

rcoh approved these changes Jan 10, 2019

View reviewed changes

akdor1154 force-pushed the parse-nodrop branch from 9d7b78c to 96cff48 Compare January 11, 2019 00:45

akdor1154 added 5 commits January 11, 2019 12:07

parse 'nodrop' parse option

b5b692c

implement parse nodrop

1c3f03a

readme

e3f5dd1

integration tests and pr changes

45cc1ab

stop nodrop from clobbering existing fields

172cfcc

akdor1154 force-pushed the parse-nodrop branch from 96cff48 to 172cfcc Compare January 11, 2019 01:11

rcoh merged commit 748e07e into rcoh:master Jan 11, 2019

rcoh mentioned this pull request Jan 11, 2019

Support for (or equivalent to) sumo's parse nodrop #45

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse nodrop #49

Parse nodrop #49

akdor1154 commented Jan 10, 2019

tstack left a comment •

edited by rcoh

tstack Jan 10, 2019

tstack Jan 10, 2019

tstack Jan 10, 2019

rcoh Jan 10, 2019

tstack Jan 10, 2019

rcoh Jan 10, 2019

rcoh left a comment

rcoh Jan 10, 2019

rcoh Jan 10, 2019

rcoh Jan 10, 2019

rcoh Jan 10, 2019

akdor1154 Jan 10, 2019

tstack Jan 10, 2019

rcoh Jan 10, 2019

rcoh left a comment

rcoh commented Jan 11, 2019

akdor1154 commented Jan 11, 2019

Parse nodrop #49

Parse nodrop #49

Conversation

akdor1154 commented Jan 10, 2019

tstack left a comment • edited by rcoh

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

rcoh 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcoh left a comment

Choose a reason for hiding this comment

rcoh commented Jan 11, 2019

akdor1154 commented Jan 11, 2019

tstack left a comment •

edited by rcoh