Skip to content
This repository has been archived by the owner on Jan 28, 2021. It is now read-only.

Reduce amount of implemented code on Expressions #772

Closed
ajnavarro opened this issue Jun 25, 2019 · 1 comment · Fixed by #773
Closed

Reduce amount of implemented code on Expressions #772

ajnavarro opened this issue Jun 25, 2019 · 1 comment · Fixed by #773
Assignees
Labels
enhancement New feature or request proposal proposal for new additions or changes

Comments

@ajnavarro
Copy link
Contributor

ajnavarro commented Jun 25, 2019

To reduce the amount of implemented code on Expressions and Nodes, we can heavily use UnaryExpression, BinaryExpression, and so on.

Right now, these interfaces are just implementing Children(), Resolved() and IsNullable().

We could implement also TransformUp function as following:

  • Create a new interface in charge of return a new instance of the implemented expression:
type ExpressionInstanceGenerator interface {
	Generate(exprs ...sql.Expression) (sql.Expression, error)
}
  • With this, we will be able to implement this interface on Unary and Binary expressions:
type BinaryExpression struct {
	ExpressionInstanceGenerator
	Left  sql.Expression
	Right sql.Expression
}
  • After that, TransformUp method can be implemented on BinaryExpression. Generate method is the one that will be implemented on the other Expressions:
func (p *BinaryExpression) TransformUp(f sql.TransformExprFunc) (sql.Expression, error) {
	var err error

	var l sql.Expression
	if p.Left != nil {
		l, err = p.Left.TransformUp(f)
		if err != nil {
			return nil, err
		}
	}
	var r sql.Expression
	if p.Left != nil {
		r, err = p.Right.TransformUp(f)
		if err != nil {
			return nil, err
		}
	}

	exp, err := p.Generate(l, r)
	if err != nil {
		return nil, err
	}

	return f(exp)
}

Doing this, we will reduce bugs on bad implemented TransformUp functions, also we can create generic Tests suites for this kind of expressions.

@ajnavarro ajnavarro added enhancement New feature or request proposal proposal for new additions or changes labels Jun 25, 2019
@erizocosmico
Copy link
Contributor

erizocosmico commented Jun 25, 2019

What about this adding a WithChildren method to the Expression interface and leverage the already present Children method.

type Expression interface {
  WithChildren(...sql.Expression) (sql.Expression, error) // add this
  // remove TransformUp
}

Then have a:

package expression

func TransformUp(e sql.Expression, f sql.TransformExprFunc) (sql.Expression, error) {
  var children []sql.Expression
  for _, c := range e.Children() {
    c, err := TransformUp(c, f)
    if err != nil {
        return nil, err
    }
    children = append(children, c)
  }

  return e.WithChildren(children)
}

In fact, we could extend this to Node:

type Node interface {
  // remove TransformUp, TransformExpressions and TransformExpressionsUp
  Expressions() []Expression
  WithChildren(...Node) (Node, error)
  WithExpressions(...Expression) (Node, error)
}

Then have:

package plan

func TransformUp(n sql.Node, f sql.TransformNodeFunc) (sql.Node, error) {
  var children []sql.Node
  for _, c := range n.Children() {
    c, err := TransformUp(c, f)
    if err != nil {
        return nil, err
    }
    children = append(children, c)
  }

  return f(n.WithChildren(children))
}

func TransformExpressions(n sql.Node, f sql.TransformExprFunc) (sql.Node, error) {
  var exprs []sql.Expression
  for _, e := range n.Expressions() {
    e, err := expression.TransformUp(e, f)
    if err != nil {
        return nil, err
    }
    exprs = append(exprs, e)
  }

  return n.WithExpressions(exprs)
}

func TransformExpressionsUp(n sql.Node, f sql.TransformExprFunc) (sql.Node, error) {
  var children []sql.Node
  for _, c := range n.Children() {
    c, err := TransformExpressionsUp(c, f)
    if err != nil {
        return nil, err
    }
    children = append(children, c)
  }

  return TransformExpressions(n.WithChildren(children), f)
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request proposal proposal for new additions or changes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants