Skip to content

types: use PlanValue in vtgate#3011

Merged
sougou merged 3 commits intovitessio:masterfrom
sougou:types
Jul 27, 2017
Merged

types: use PlanValue in vtgate#3011
sougou merged 3 commits intovitessio:masterfrom
sougou:types

Conversation

@sougou
Copy link
Contributor

@sougou sougou commented Jul 25, 2017

VTGate was slightly trickier than tabletserver, which resulted in three commits:

  • The Values for the insert plan were stored row-wise. The new PlanValue rule requires them to be column-wise. The first commit makes that switch first.
  • vtgate is changed to use PlanValue. This affected the various primitives in engine. Additionally, the route builder was cheating: It was temporarily storing AST elements in those values and later replacing them into actual values during the wire-up phase. The route builder now has its own condition field where it stores the routing condition, and later resolves it into the Values into the engine's route.
  • Vindexes are also converted to use sqltypes.Value.

With these changes, more functions in sqltypes.Value should be obsolete. The next phase of changes will be the cleanup and tightening of sqltypes.

sougou added 3 commits July 25, 2017 14:12
Issue #2919

Insert plan values were stored by row, which are not compatible
with the new PlanValue scheme, which requires each PlanValue to
store the values of one column each . This change stores the values
column-wise, which also serendipitously simplifies the code.
Issue #2919

Converted 'Values' in Route, Generate and Limit from
interface{} to PlanValue types. Followed the convention
that each PlanValue object represents the value(s) of
one column.

There is still some transitional code for the vindex glue
which will be removed in the next commit.
Issue #2919

The vindex API has been changed to accept sqltype.Value
instead of interface. This eliminates many unnecessary
validations.
@sougou sougou requested a review from alainjobart July 25, 2017 21:22
Copy link
Contributor

@alainjobart alainjobart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big change, but seems safe if it compiles and passes all the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants