feat(planner): Add EquiJoinPredicateRule #4665

scbrickley · 2022-04-12T15:53:40Z

This patch adds a few things:

A new planner rule called EquiJoinPredicateRule that checks if a join predicate defines an equi-join (Closes Add a planner rule that can analyze a predicate function #4625)
Stubs for a new join package, which will be fully implemented & documented later (See Add a join package to stdlib #4629 and Document join package #4631)
A new check in runtime.Eval that panics if builtins are not finalized, with a clear error message describing how to fix the issue. This is the result of the difficulty I had trying to evaluate flux scripts in go unit tests within stdlib. The solution was to import fluxinit/static

I have one concern with the new planner rule that I could use some feedback on: Originally the intent was to have the join transformation check what kind of procedure spec it was passed, and to throw an error if it was not an EquiJoinProcedureSpec, but due to the scope of #4625, it was easier to just have the planner rule itself return an error. This may increase the workload for adding other implementations of join later on, but not by much. Still, I wonder if it's better to adjust the rule to not throw an error if it doesn't find an equi-join, and instead just return the original procedure spec.

OfTheDelmer · 2022-04-14T20:45:27Z

libflux/go/libflux/buildinfo.gen.go

@@ -268,6 +268,7 @@ var sourceHashes = map[string]string{
 	"stdlib/internal/testutil/testutil.flux":                                                      "f2969ae7c30d030a0e94b803c1eb2306de3d13ac60109bb3f9a3d756643fa2dd",
 	"stdlib/interpolate/interpolate.flux":                                                         "3d480c9058c584b65841db7889c459ad9bfed679f4ce24d17a84ec020cce768b",
 	"stdlib/interpolate/interpolate_test.flux":                                                    "077562d9840b7e4e5ead9565413fb2d88cf5dfac15b55175062afdff5da67305",
+	"stdlib/join/join.flux":                                                                       "c431da575b754952dd4a4b2f71c0b6e56bd1b204520999b2192a83f5c6ad719f",


OfTheDelmer · 2022-04-14T21:07:57Z

stdlib/join/equijoin.go

+	"github.com/influxdata/flux/semantic"
+)
+
+const EquiJoinKind = "equijoin"


should this be camelcase?

Suggested change

const EquiJoinKind = "equijoin"

const EquiJoinKind = "equiJoin"

stdlib/join/equijoin.go

OfTheDelmer · 2022-04-14T22:34:20Z

stdlib/join/equijoin_test.go

+			join.join(
+				left: left,
+				right: right,
+				on: (l, r) => r.a == l.b and r.d == l.b,


Should we explicitly test multiple comparison with an operator error for something like on: (l, r,) => r.a == l.b and !(r.d == l.b)?

Yeah, that's probably a good idea. Adding a testcase for that now.

OfTheDelmer · 2022-04-14T22:35:18Z

stdlib/join/join.flux

+// - on:
+// - as:
+// - method:
+builtin join : (


wolffcm

This looks great.

My only issue is that the error messages aren't what I would want users seeing if they began using the new join package in anger. These can be hard to get right, especially when there are lots of cases that we can't yet support. We can work on that in this PR, or you could also file an issue to clean it up later.

wolffcm · 2022-04-18T16:04:05Z

interpreter/interpreter_test.go

+	scope.Set("r", values.Null)
+
+	intrp := interpreter.NewInterpreter(nil, nil)
+	sideEffects, _ := intrp.Eval(context.Background(), &node, scope, nil)


This is great, and it also doesn't quite capture all of the places where we handle exists.

We actually evaluate expressions in two places, in the interpreter as you're doing here, and also in the Compiler which is a really confusing name. The Compiler is the thing that evaluates, e.g., the lambda that's passed into map. You could use an acceptance test to do this, if it doesn't produce an error.

Moreover, there's really two kinds of things that should get tested here:

When we know the names and types of all the fields statically, e.g., what the input data to a function like map is produced with array.from

When we don't know all the fields statically, e.g., when the input data is generated via csv.from

If I understand right #4626 would need to test both of these cases for both the interpreter and the compiler.

wolffcm · 2022-04-18T16:04:27Z

runtime/runtime.go

+	// to evaluate flux scripts in go unit tests
+	if !r.finalized {
+		panic("runtime is not finalized - consider importing package fluxinit or fluxinit/static")
+	}


wolffcm · 2022-04-18T16:15:17Z

stdlib/join/equijoin.go

+	fnBody := spec.On.Fn.Block.Body
+
+	if len(fnBody) != 1 {
+		return nil, false, errors.New(codes.Invalid, "predicate should only contain a single return statement")


Users might be confused by this error, since a valid lambda to get passed in might look something like (l, r) => l.id == r.id and l.foo == r.bar and doesn't need an explicit return statement. (I believe that the semantic graph will have a Return statement for that case, but it can be elided in the syntax)

Also, the message should note that there are some forms of lambda that are just not supported yet, so the user understands that they didn't do something wrong, it's just that we have more work to do.

Ah, I see your point. I'm open to any suggestions you have for the wording. Maybe something like this?

some lambda expressions are not yet supported in join predicates - function body should only contain a single logical expression comparing equality of columns from the left and right tables

That seems kinda long winded, but I'm having trouble thinking of a more succinct way to communicate the problem.

It would be easier if we could link them to some documentation explaining what is and isn't allowed in a predicate.

Maybe once #4631 is finished, we can circle back and update these errors to just link to the generated fluxdoc page for the new join package.

Sure, I like your message, you could also explicitly refer to the name of the parameter:

some expressions are not yet supported in the "on" parameter - function body should only contain a single logical expression comparing equality of columns from the left and right tables

(I guess the error message will have prefixed with something like error in function join or some such)

wolffcm

This looks good, but I don't think that this PR should close #4626, it should stay open until there are more tests.

scbrickley · 2022-04-18T19:26:39Z

Right. Completely forgot about that. I'll move that test to a separate branch and link the issue in a new PR.

scbrickley · 2022-04-18T20:21:24Z

@noramullen1 What are your thoughts on this PR from a docs perspective? There's a new package, but no real docs. Just the bare minimum to get the fluxdoc CI check to pass. There's an open issue to document this package that we intend to get to later: #4631

I am operating under the assumption that we don't want to document things that are not ready for users yet, but I'm not sure how you and your team feel about this. What should we do here?

sanderson · 2022-04-18T20:24:45Z

@scbrickley I think it's fine to not include full docs at this point. Just make sure that the existing, bare bones docs include something like: "This package is in actively development and is not currently ready to be used". Same for the functions inside the package.

Just in case someone pulls the docs assets from a Flux version where the join package isn't ready yet.

scbrickley · 2022-04-18T20:25:54Z

Thanks @sanderson! Making the changes now.

scbrickley force-pushed the join-package branch 2 times, most recently from 829bfef to 7fc35db Compare April 13, 2022 18:47

scbrickley changed the title ~~feat: Add EquiJoinPredicateRule to planner~~ feat(planner): Add EquiJoinPredicateRule Apr 13, 2022

scbrickley force-pushed the join-package branch 6 times, most recently from 8afcc62 to ba59a45 Compare April 14, 2022 20:05

scbrickley marked this pull request as ready for review April 14, 2022 20:41

scbrickley requested review from a team as code owners April 14, 2022 20:41

scbrickley requested review from jsternberg and noramullen1 and removed request for a team April 14, 2022 20:41

OfTheDelmer reviewed Apr 14, 2022

View reviewed changes

scbrickley requested a review from wolffcm April 18, 2022 14:24

wolffcm suggested changes Apr 18, 2022

View reviewed changes

scbrickley requested a review from wolffcm April 18, 2022 19:19

wolffcm approved these changes Apr 18, 2022

View reviewed changes

feat(planner): AddEquiJoinPredicateRule

f05d619

scbrickley force-pushed the join-package branch from f575c49 to f05d619 Compare April 18, 2022 19:32

sanderson requested review from sanderson and removed request for noramullen1 April 18, 2022 20:24

scbrickley force-pushed the join-package branch from a23ab6b to cbddae6 Compare April 18, 2022 20:30

docs(join): placeholder for doc comments

db8a38f

scbrickley force-pushed the join-package branch from cbddae6 to db8a38f Compare April 18, 2022 20:37

scbrickley merged commit b42886e into master Apr 19, 2022

scbrickley deleted the join-package branch April 19, 2022 14:03

scbrickley mentioned this pull request May 17, 2022

feat: Add a planner rule that expands logical join nodes #4765

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(planner): Add EquiJoinPredicateRule #4665

feat(planner): Add EquiJoinPredicateRule #4665

scbrickley commented Apr 12, 2022 •

edited

Loading

OfTheDelmer Apr 14, 2022

OfTheDelmer Apr 14, 2022

OfTheDelmer Apr 14, 2022

scbrickley Apr 18, 2022

OfTheDelmer Apr 14, 2022

wolffcm left a comment

wolffcm Apr 18, 2022 •

edited

Loading

wolffcm Apr 18, 2022

wolffcm Apr 18, 2022

scbrickley Apr 18, 2022

scbrickley Apr 18, 2022

scbrickley Apr 18, 2022

wolffcm Apr 18, 2022

wolffcm left a comment

scbrickley commented Apr 18, 2022

scbrickley commented Apr 18, 2022

sanderson commented Apr 18, 2022

scbrickley commented Apr 18, 2022

	const EquiJoinKind = "equijoin"
	const EquiJoinKind = "equiJoin"

feat(planner): Add EquiJoinPredicateRule #4665

feat(planner): Add EquiJoinPredicateRule #4665

Conversation

scbrickley commented Apr 12, 2022 • 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

wolffcm left a comment

Choose a reason for hiding this comment

wolffcm Apr 18, 2022 • 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

wolffcm left a comment

Choose a reason for hiding this comment

scbrickley commented Apr 18, 2022

scbrickley commented Apr 18, 2022

sanderson commented Apr 18, 2022

scbrickley commented Apr 18, 2022

scbrickley commented Apr 12, 2022 •

edited

Loading

wolffcm Apr 18, 2022 •

edited

Loading