compare flux tables for equality #291

jpacik · 2018-11-16T01:24:17Z

Related to #236 .

This PR exposes some utility functions for comparing Flux tables for equality. All values are compared for exact equality except floating point values - they are compared using a combination of absolute tolerance and ULPs (units in the last place).

When two values fail an equality comparison, a table formatter along with an appropriate error message is returned. This allows the caller of the function to print out both formatted tables along with the row and column number where the equality comparison failed.

wolffcm

This looks very nice. I think you should change the types to be exportable though.

It would be good to make the existing transformations_test use this if it's not too much trouble. It would help cement this code as the go-to place for comparing results.

A future enhancement might be to allow results/streams/tables to appear in different orders if a non-deterministic (but still valid) ordering is possible. This would avoid having to add a sort operation to each query if (for example) in the future we do something to execution that tweaks the order.

wolffcm · 2018-11-16T15:20:31Z

querytest/compare.go

+// float values must be within in order to be considered equal.
+const FloatTolerance float64 = 1e-50
+
+// FloatULP is the maximum number of floats that are allowed to lie


It might be worth saying what ULP stands for here---I had to look it up.

wolffcm · 2018-11-16T15:25:07Z

querytest/compare.go

+}
+
+func (t *tables) EqualWithTolerance(s *tables, tol float64, ulp uint) *results {
+	defer t.data.Release()


Might the callers want to do more stuff with the results? Intuitively if tables is not creating these results, then maybe it should be up to the callers to release them. If there's a reason this method should be releasing the results, it would be good to note it in a method comment.

You're right, caller should be responsible for releasing tables.

wolffcm · 2018-11-16T15:29:24Z

querytest/compare.go

+	return r.fmt
+}
+
+func (t *tables) EqualWithTolerance(s *tables, tol float64, ulp uint) *results {


It would be good to note which tables instances are expected/actual or want/got here, and in error messages.

I gave tables a name param which the caller provides, so that the caller doesn't have to remember if "want" should be on the left or right. See https://github.com/influxdata/platform/pull/1450/files#diff-afa3e17b057923f1e8fcb59b758370ddR273 for usage. Perhaps I should explain that here?

Ah, okay. That makes sense.

It would be good to add comments to all the exported items here, especially since this code could also be used by contributors as they add new functions to Flux.

Agreed, I'll get get these comments up with the rest of the changes.

wolffcm · 2018-11-16T15:47:54Z

querytest/compare.go

+	data flux.ResultIterator
+}
+
+func CreateTables(name string, data flux.ResultIterator) *tables {


Returning unexported types from exported functions seems to be discouraged as a practice---for example, golint will complain about this.
golang/lint#210
I think the structs returned from functions in this file should be exported.

Rather than introducing a new type, perhaps we can add a Name() function to the result iterator interface? then we can just return a ResultIterator which would help clarify the code.

Yes I can export the types.

nathanielc

I took a quick glance and it looks good. I am curious how this code is different from the execute test package https://github.com/influxdata/flux/blob/master/execute/executetest/result.go ? Is it that instead of converting the tables to a common test type it simply iterates over them to compare them, meaning it can compare any result implementations?

jpacik · 2018-11-16T16:59:29Z

@nathanielc yes, if it implements the Result interface then it can be compared for equality.

nathanielc · 2018-11-16T17:52:21Z

@jlapacik Sounds good, I am wondering if we should move things around so its clear why you would use one over the other. Or maybe we should just replace all existing instances if comparing executetest.Result types with this new code?

Basically if we have two ways to do the same thing it needs to be clear why we use one over the other. Otherwise we should remove one of the way to do it.

jpacik · 2018-11-16T17:55:04Z

@nathanielc yeah I totally forgot about executetest. These methods should probably just operate on the Result and Table types in executetest.

aanthony1243

Had a few thoughts about the types. I think we should clean up/disambiguate some of the names.

I know that the go mentality is that types are often associated with a package name so that the type name can be a common word like table however, the table concept is so critical in our code, I am hesitant to have a table type that doesn't implement our execute.Table interface.

aanthony1243 · 2018-11-16T16:37:51Z

querytest/compare.go

+	data flux.ResultIterator
+}
+
+func CreateTables(name string, data flux.ResultIterator) *tables {


Rather than introducing a new type, perhaps we can add a Name() function to the result iterator interface? then we can just return a ResultIterator which would help clarify the code.

aanthony1243 · 2018-11-16T16:48:20Z

querytest/compare.go

+	})
+}
+
+func equalFloats(a, b []float64, tol float64, ulp uint) (int, bool) {


We could add these helpers to the ColumnReader interface, or else adjacent to the column reader interface--ComparableColumnReader? If they aren't useful in production, I would still recommend making these exportable in a separate file so that other tests may know about and share this code.

Should I just add the Equals() method to the ColumnReader and Table interfaces?

That is probably OK. If you don't want to deal with the interface cascade, you can just export these so others can use them. I'm on the fence whether we need this code outside of a testing scenario, but it might be useful some day, and odds are if we keep it in querytest we would forget about it and re-implement the same feature in execute

@nathanielc what do you think?

aanthony1243 · 2018-11-16T18:25:05Z

querytest/compare.go

+				t.name, l.Name(), s.name, r.Name()),
+			}
+		}
+		var lt, rt []table


I feel like table is overloaded as a keyword in this code. There's a flux.Table, then tables then table and below table is a loop variable.

Why do we need the table type? it seems above that the flux.Table type has a Name() function.

Yeah you're right I need better names. I just couldn't think of good descriptions. The reason tables have a Name param is so that the caller can label which result/table corresponds to "want" and which one corresponds to "got".

jpacik · 2018-11-19T22:10:06Z

@aanthony1243 @nathanielc can you give this another review please. I've decided to just use the framework that already exists in the executetest package. I just added some extra options for comparing floats as well as an extra method for comparing a list of Flux Results. This option requires zero refactoring of any execute tests. And using go-cmp means we won't have to write code to test our testing code. Thoughts?

execute/executetest/transformation.go

nathanielc · 2018-11-19T22:48:50Z

execute/executetest/result.go

@@ -45,3 +48,47 @@ func (ti *TableIterator) Do(f func(flux.Table) error) error {
 	}
 	return nil
 }
+
+// EqualResults compares two lists of Flux Results for equlity
+func EqualResults(want, got []flux.Result) (bool, error) {


Where is this function used?

It will be used in platform for the generated ht tests.

equate floating point values using a combination of absolute error and ULP (unit of least precision)

jpacik self-assigned this Nov 16, 2018

jpacik requested review from wolffcm and aanthony1243 November 16, 2018 01:24

wolffcm suggested changes Nov 16, 2018

View reviewed changes

nathanielc reviewed Nov 16, 2018

View reviewed changes

aanthony1243 suggested changes Nov 16, 2018

View reviewed changes

nathanielc reviewed Nov 19, 2018

View reviewed changes

execute/executetest/transformation.go Show resolved Hide resolved

nathanielc reviewed Nov 19, 2018

View reviewed changes

nathanielc approved these changes Nov 20, 2018

View reviewed changes

approximate floating point values when comparing tables

5452351

equate floating point values using a combination of absolute error and ULP (unit of least precision)

jpacik force-pushed the jl-compare-tables branch from 76138d4 to 5452351 Compare November 21, 2018 00:17

jpacik merged commit 54f9e3f into master Nov 21, 2018

jpacik deleted the jl-compare-tables branch November 21, 2018 00:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compare flux tables for equality #291

compare flux tables for equality #291

jpacik commented Nov 16, 2018

wolffcm left a comment

wolffcm Nov 16, 2018

wolffcm Nov 16, 2018

jpacik Nov 16, 2018

wolffcm Nov 16, 2018

jpacik Nov 16, 2018

wolffcm Nov 16, 2018

jpacik Nov 16, 2018

wolffcm Nov 16, 2018

aanthony1243 Nov 16, 2018

jpacik Nov 16, 2018

nathanielc left a comment

jpacik commented Nov 16, 2018

nathanielc commented Nov 16, 2018

jpacik commented Nov 16, 2018

aanthony1243 left a comment

aanthony1243 Nov 16, 2018

aanthony1243 Nov 16, 2018

jpacik Nov 16, 2018

aanthony1243 Nov 19, 2018

aanthony1243 Nov 16, 2018

jpacik Nov 16, 2018

jpacik commented Nov 19, 2018

nathanielc Nov 19, 2018

jpacik Nov 19, 2018

compare flux tables for equality #291

compare flux tables for equality #291

Conversation

jpacik commented Nov 16, 2018

wolffcm 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanielc left a comment

Choose a reason for hiding this comment

jpacik commented Nov 16, 2018

nathanielc commented Nov 16, 2018

jpacik commented Nov 16, 2018

aanthony1243 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

jpacik commented Nov 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment