sql/analyzer: resolve correctly GetField expressions indexes #181

mcarmonaa · 2018-05-04T12:00:13Z

Fixes #180

Signed-off-by: Manuel Carmona [email protected]

mcarmonaa · 2018-05-04T12:02:52Z

It seems st is failing, don't review yet

mcarmonaa · 2018-05-04T14:05:43Z

@ajnavarro now CI passes, it's ready to review!

ajnavarro · 2018-05-04T14:43:55Z

engine_test.go

@@ -206,6 +206,32 @@ func TestQueries(t *testing.T) {
 	}
 }

+func TestJoinsFields(t *testing.T) {


Can you add the query to the queries slice instead of create a new test for this?

ajnavarro · 2018-05-04T14:45:59Z

sql/analyzer/rules.go

@@ -809,3 +810,49 @@ func fixFieldIndexes(schema sql.Schema, exp sql.Expression) (sql.Expression, err
 		return e, nil
 	})
 }
+
+func fixJoinFields(ctx *sql.Context, a *Analyzer, node sql.Node) (sql.Node, error) {


Can you add some comments to this function explaining why this is needed and adding a TODO to fix it in a proper way?

ajnavarro · 2018-05-04T14:46:47Z

sql/analyzer/rules.go

+			return n, nil
+		}
+
+		_, ok := n.(*plan.InnerJoin)


Only for inner joins? should be for all kind of joins, right?

erizocosmico

Overall, except for what @ajnavarro said it looks good to me. Just a question, isn't it possible to fix this without a rule? Can you add a bit of context in the issue description about why does this happen and why it cannot be solved without a rule?

mcarmonaa · 2018-05-07T11:51:28Z

@ajnavarro @erizocosmico

I changed the PR to address all cases. Now there is a rule resolveGetFieldIndexes in charge of set the correct index for the GetField expressions referencing this value to the sql.Node that contains that expression.

Thus, the resolveColumns rule doesn't set these indexes anymore and we avoid the usage of the global row's indexes based on the final schema.

erizocosmico · 2018-05-07T13:48:46Z

sql/analyzer/rules.go

@@ -330,6 +326,8 @@ type column interface {
 	sql.Expression
 }

+const unresolvedGetFieldIndex = 2147483647


-1 would do the trick and then you can do index >= 0 to know if it's resolved. Plus, I think it's a more obvious value for "unresolved".

erizocosmico · 2018-05-07T13:50:20Z

sql/plan/innerjoin.go

 	return []sql.Expression{j.Cond}
 }
+
+// TransformExpressions implements the Expressioner interface.
+func (j *InnerJoin) TransformExpressions(f sql.TransformExprFunc) error {


why add this method when nodes already have TransformExpressionsUp?

Because TransformExpressionsUp transforms expressions for this node and its childrens meanwhile this method only transform the expressions for this node.

Ok. In any case, all nodes should be immutable, so they would need to return new instances, just as TransformExpressionsUp and TransformUp do.

ajnavarro · 2018-05-08T08:34:22Z

@mcarmonaa You can do the same logic calling Expressions() method on the nodes that you need into a rule. I cannot see the need of TransformExpression() method.

mcarmonaa · 2018-05-08T08:53:03Z

@ajnavarro, calling Expressions you will get the expressions for this node, but you must add a different implementation to generate a new node with its expressions changed for each node subtype(inner join, project, etc) in the rule.

Having the method TransformExpressions you have that specific implementation done in each node, which I think it's better.

ajnavarro · 2018-05-08T08:58:41Z

Yes, but you can do it in a rule checking the type of the node depending on the case.

smola

I think that it would be better to resolve unresolved column to get field in a single step. Instead of generating unresolved get field first and then get field.

In a rule iteration, if the children nodes are not resolved, you just skip it and it will be called.

ajnavarro · 2018-05-10T10:40:55Z

@erizocosmico can you have a look?

erizocosmico · 2018-05-10T12:14:01Z

sql/type.go

-			return true
+// Contains returns whether the schema contains a column with the given name
+// and the index of that column in the schema.
+func (s Schema) Contains(column string, source string) (int, bool) {


Instead of making contains return an int, which feels weird, maybe we could have a ColumnIndex or IndexOf method that returns the index or -1 when it's not found and the following Contains:

func (s Schema) Contains(column string, source string) bool { return s.IndexOf(column, source) >= 0 }

WDYT?

erizocosmico · 2018-05-10T12:14:48Z

sql/core.go

@@ -110,6 +110,10 @@ type Node interface {
 type Expressioner interface {
 	// Expressions returns the list of expressions contained by the node.
 	Expressions() []Expression
+	// TransformExprssions applies for each expression in this node


s/TransformExprssions/TransformExpressions/

Sounds like something that an intelligent code review bot should be able to detect, hint

…umn rule. Signed-off-by: Manuel Carmona <[email protected]>

ajnavarro · 2018-05-16T09:57:46Z

@erizocosmico ping

ajnavarro · 2018-05-21T13:33:10Z

@erizocosmico ping?

erizocosmico

Oops, sorry, didn't see this before

mcarmonaa requested review from jfontan, ajnavarro, erizocosmico and kuba-- and removed request for jfontan, erizocosmico and kuba-- May 4, 2018 12:00

mcarmonaa force-pushed the fix/rows-indexes-in-getfield branch 2 times, most recently from 06ebb75 to c3e0018 Compare May 4, 2018 14:02

ajnavarro suggested changes May 4, 2018

View reviewed changes

ajnavarro requested a review from erizocosmico May 4, 2018 14:48

erizocosmico reviewed May 4, 2018

View reviewed changes

mcarmonaa changed the title ~~sql/analyzer: add rule to fix field's indexes in inner joins~~ sql/analyzer: add rule to fix GetField's indexes May 7, 2018

mcarmonaa mentioned this pull request May 7, 2018

Row's indexes are not handled properly #180

Closed

erizocosmico reviewed May 7, 2018

View reviewed changes

erizocosmico suggested changes May 7, 2018

View reviewed changes

smola reviewed May 9, 2018

View reviewed changes

mcarmonaa force-pushed the fix/rows-indexes-in-getfield branch from 8db045c to 6172b02 Compare May 10, 2018 10:24

ajnavarro approved these changes May 10, 2018

View reviewed changes

mcarmonaa changed the title ~~sql/analyzer: add rule to fix GetField's indexes~~ sql/analyzer: resolve correctly GetField expressions indexes May 10, 2018

erizocosmico suggested changes May 10, 2018

View reviewed changes

sql: resolve correctly GetField expressions indexes in the resolveCol…

cfc5c19

…umn rule. Signed-off-by: Manuel Carmona <[email protected]>

mcarmonaa force-pushed the fix/rows-indexes-in-getfield branch from 6172b02 to cfc5c19 Compare May 10, 2018 12:52

mcarmonaa mentioned this pull request May 16, 2018

*: implement indexable interfaces on basic and relation tables src-d/gitbase#279

Merged

erizocosmico approved these changes May 21, 2018

View reviewed changes

ajnavarro merged commit 6dacde8 into src-d:master May 22, 2018

sql/analyzer: resolve correctly GetField expressions indexes #181

sql/analyzer: resolve correctly GetField expressions indexes #181

Uh oh!

Conversation

mcarmonaa commented May 4, 2018

Uh oh!

mcarmonaa commented May 4, 2018

Uh oh!

mcarmonaa commented May 4, 2018

Uh oh!

ajnavarro May 4, 2018

Choose a reason for hiding this comment

Uh oh!

ajnavarro May 4, 2018

Choose a reason for hiding this comment

Uh oh!

ajnavarro May 4, 2018

Choose a reason for hiding this comment

Uh oh!

erizocosmico left a comment

Choose a reason for hiding this comment

Uh oh!

mcarmonaa commented May 7, 2018

Uh oh!

erizocosmico May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erizocosmico May 7, 2018

Choose a reason for hiding this comment

Uh oh!

mcarmonaa May 7, 2018

Choose a reason for hiding this comment

Uh oh!

erizocosmico May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajnavarro commented May 8, 2018

Uh oh!

mcarmonaa commented May 8, 2018

Uh oh!

ajnavarro commented May 8, 2018

Uh oh!

smola left a comment

Choose a reason for hiding this comment

Uh oh!

ajnavarro commented May 10, 2018

Uh oh!

erizocosmico May 10, 2018

Choose a reason for hiding this comment

Uh oh!

erizocosmico May 10, 2018

Choose a reason for hiding this comment

Uh oh!

eiso May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajnavarro commented May 16, 2018

Uh oh!

ajnavarro commented May 21, 2018

Uh oh!

erizocosmico left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erizocosmico May 7, 2018 •

edited

Loading

erizocosmico May 7, 2018 •

edited

Loading

eiso May 11, 2018 •

edited

Loading