Skip to content

Commit

Permalink
Merge pull request #9121 from tamird/merge-master
Browse files Browse the repository at this point in the history
master -> develop
  • Loading branch information
tamird authored Sep 6, 2016
2 parents d63b41e + a35586f commit 1fe9e73
Show file tree
Hide file tree
Showing 44 changed files with 1,188 additions and 544 deletions.
7 changes: 5 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,11 @@ include .go-version

ifneq ($(SKIP_BOOTSTRAP),1)

GITHOOKS := $(subst githooks/,.git/hooks/,$(wildcard githooks/*))
.git/hooks/%: githooks/%
# If we're in a git worktree, the git hooks directory may not be in our root,
# so we ask git for the location.
GITHOOKSDIR := $(shell git rev-parse --git-path hooks)
GITHOOKS := $(subst githooks/,$(GITHOOKSDIR)/,$(wildcard githooks/*))
$(GITHOOKSDIR)/%: githooks/%
@echo installing $<
@rm -f $@
@mkdir -p $(dir $@)
Expand Down
7 changes: 7 additions & 0 deletions acceptance/cluster/localcluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import (
"github.com/cockroachdb/cockroach/security"
"github.com/cockroachdb/cockroach/util"
"github.com/cockroachdb/cockroach/util/log"
"github.com/cockroachdb/cockroach/util/log/logflags"
"github.com/cockroachdb/cockroach/util/stop"
"github.com/cockroachdb/cockroach/util/syncutil"
"github.com/cockroachdb/cockroach/util/timeutil"
Expand Down Expand Up @@ -460,6 +461,12 @@ func (l *LocalCluster) startNode(node *testNode) {
"--verbosity=1",
}

// Forward the vmodule flag to the nodes.
vmoduleFlag := flag.Lookup(logflags.VModuleName)
if vmoduleFlag.Value.String() != "" {
cmd = append(cmd, fmt.Sprintf("--%s=%s", vmoduleFlag.Name, vmoduleFlag.Value.String()))
}

for _, store := range node.stores {
storeSpec := base.StoreSpec{
Path: store.dataStr,
Expand Down
2 changes: 1 addition & 1 deletion acceptance/terrafarm/farmer.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func (f *Farmer) NumNodes() int {
func (f *Farmer) Add(nodes int) error {
nodes += f.NumNodes()
args := []string{
fmt.Sprintf("-var=num_instances=%d", nodes),
fmt.Sprintf("-var=num_instances=\"%d\"", nodes),
fmt.Sprintf("-var=stores=%s", f.Stores),
}

Expand Down
3 changes: 3 additions & 0 deletions acceptance/terraform/nodectl
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ if [ $# -ne 2 ]; then
usage
fi

# ~/.config/gcloud may be erroneously owned by root, so fix that.
sudo chown -R ${USER} "${HOME}/.config/gcloud"

# CockroachDB nodes are named blah-cockroach-[0-9]*, so extract the final
# numeric part.
gcs_url="$2"
Expand Down
2 changes: 1 addition & 1 deletion acceptance/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ func farmer(t *testing.T, prefix string) *terrafarm.Farmer {
Cwd: *flagCwd,
LogDir: logDir,
KeyName: *flagKeyName,
Stores: stores,
Stores: strconv.Quote(stores),
Prefix: name,
StateFile: name + ".tfstate",
AddVars: terraformVars,
Expand Down
1 change: 1 addition & 0 deletions build/build-docker-deploy.sh
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ if [ "${1-}" = "docker" ]; then
time make STATIC=1 release

check_static cockroach
strip -S cockroach

mv cockroach build/deploy/cockroach

Expand Down
2 changes: 1 addition & 1 deletion cli/debug.go
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,7 @@ var debugEnvCmd = &cobra.Command{
Output environment variables that influence configuration.
`,
Run: func(cmd *cobra.Command, args []string) {
env := envutil.GetEnvReport()
env := envutil.GetEnvReport(false)
fmt.Print(env)
},
}
Expand Down
10 changes: 5 additions & 5 deletions cli/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,9 @@ var nodesColumnHeaders = []string{
"value_bytes",
"intent_bytes",
"system_bytes",
"leader_ranges",
"repl_ranges", // Using abbreviations to avoid excessively wide output.
"avail_ranges",
"replicas_leaders",
"replicas_leaseholders",
"ranges_available",
}

var statusNodeCmd = &cobra.Command{
Expand Down Expand Up @@ -176,8 +176,8 @@ func nodeStatusesToRows(statuses []status.NodeStatus) [][]string {
strconv.FormatInt(int64(metricVals["valbytes"]), 10),
strconv.FormatInt(int64(metricVals["intentbytes"]), 10),
strconv.FormatInt(int64(metricVals["sysbytes"]), 10),
strconv.FormatInt(int64(metricVals["ranges.leader"]), 10),
strconv.FormatInt(int64(metricVals["ranges.replicated"]), 10),
strconv.FormatInt(int64(metricVals["replicas.leaders"]), 10),
strconv.FormatInt(int64(metricVals["replicas.leaseholders"]), 10),
strconv.FormatInt(int64(metricVals["ranges.available"]), 10),
})
}
Expand Down
3 changes: 3 additions & 0 deletions cli/start.go
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,9 @@ func runStart(_ *cobra.Command, args []string) error {
}

log.Info(startCtx, "starting cockroach node")
if envVarsUsed := envutil.GetEnvReport(true); envVarsUsed != "" {
log.Infof(startCtx, "using local environment variables:\n%s", envVarsUsed)
}
s, err := server.NewServer(serverCtx, stopper)
if err != nil {
return fmt.Errorf("failed to start Cockroach server: %s", err)
Expand Down
2 changes: 1 addition & 1 deletion cmd/zerosum/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ func (z *zeroSum) verify(d time.Duration) {
func (z *zeroSum) rangeInfo() (int, []int) {
replicas := make([]int, len(z.nodes))
client := z.clients[z.randNode(rand.Intn)]
rows, err := client.Scan(keys.Meta2Prefix, keys.Meta2KeyMax, 0)
rows, err := client.Scan(keys.Meta2Prefix, keys.Meta2Prefix.PrefixEnd(), 0)
if err != nil {
z.maybeLogError(err)
return -1, replicas
Expand Down
252 changes: 252 additions & 0 deletions docs/RFCS/type_annotations.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,252 @@
- Feature Name: SQL Type Annotations
- Status: completed
- Start Date: 2016-05-25
- Authors: Nathan, knz
- RFC PR: #6895
- Cockroach Issue: #6189


# Summary

This RFC proposes to add a new "type annotation" syntax to our SQL dialect
(referred to as CockroachSQL from now on). This was originally proposed as
part of the Summer typing system, but is being split up separately because
it is purely an extension which was not necessary for the immediate
correctness of the type system.

# Motivation

In order to clarify the typing rules during the design of the Summer
type system and to exercise the proposed system, we found it was useful
to "force" certain expressions to be of a certain type.

Unfortunately, the SQL cast expression (`CAST(... AS ...)` or
`...::...`) is not appropriate for this, because although it
guarantees a type to the surrounding expression it does not constrain
its argument. For example `sign(1.2)::int` does not disambiguate which
overload of `sign` to use.

Therefore we propose the following SQL extension, which was not
required for the correctness of the Summer typing system but offers
opportunities to better exercise it in tests. The extension also gives
users of our SQL dialect more control over the type inference decisions
of the type system.

The need for this type of extension was also implicitly
present/expressed in the alternate proposals Rick and Morty.

# Detailed design

The extension is a new "type annotation" expression node.

### Operator Syntax

We define the following SQL syntax: `E ::: T`, where `E` is a sub-expression
and `T` is a type. We chose this syntax because it does not collide (visually
or logically) with other SQL syntax, and because it draws parallels to the
cast operator syntax.

For example: `1:::int` or `1 ::: int`.

The meaning of this at a first order approximation is "interpret the
expression on the left using the type on the right".

### Keyword Syntax

In addition, we define the keyword syntax: `ANNOTATE_TYPE(E, T)`, where `E`
is a sub-expression and `T` is a type. We chose this syntax because it draws
parallels to the cast keyword syntax, without requiring special client handling.
We could have opted for an `ANNOTATE_TYPE(E AS T)` syntax, which would have been
more similar to the cast keyword syntax, but would not be usable by ORMs or other
SQL client abstraction layers because it would not look like a function call.

For example: `ANNOTATE_TYPE(1, int)`.

### Type Annotations vs. Type Casts

As expressed above, type annotations and type casts play different roles.
The primary purpose of a type cast is to take a given expression of type
`T1` and map it during evaluation to a type `T2`. This cast can be used to
work around disjoint types between a sub-expression and its parent context,
but it has no effect on typing of the sub-expression itself. In practice
with the Summer type system, this means that during type checking, a
`CastExpr` will throw away any recursively passed `desiredType`, and will
propagate down `NoPreference` to its sub-expressions.

On the other hand, type annotations have no runtime effect (as implied by
the term "annotation"). Instead, the annotation only has an effect during
semantic analysis. The annotation will do what it can to type its sub-expression
as its annotation type, and then assert that the sub-expression does get typed
as this type, throwing an error if not. In a "bottom-up" type system, the
only thing an annotation like this could do is assert after its sub-expression's
type has been resolved that it matches its annotation type. However, in a
bi-directional typing system like Summer, the annotation can be more effective
by also "desiring" its annotation type from its children. In effect, this means
that the sub-expression will become the desired type if possible.

Examples

```sql
SELECT 1.2
-> no preference for the return type of `1.2`
-> numeric constant without a preference defaults to DECIMAL
-> the expression returns a DECIMAL
SELECT 1.2::float
-> no preference for the return type of CAST(... as FLOAT)
-> no preference for the return type of `1.2`
-> numeric constant without a preference defaults to DECIMAL
-> CastExpr returns a FLOAT instead, which it will cast during evaluation
-> the expression returns a FLOAT
SELECT 1.2::string
-> no preference for the return type of CAST(... as STRING)
-> no preference for the return type of `1.2`
-> numeric constant without a preference defaults to DECIMAL
-> CastExpr returns a STRING instead, which it will cast (format) during evaluation
-> the expression returns a STRING
SELECT 1.2:::float
-> no preference for the return type of `... ::: FLOAT`
-> FLOAT preference passed for the return type of `1.2`
-> numeric constant contains FLOAT in its "resolvable type set", so it resolves
itself as a FLOAT
-> the type annotation correctly asserts that its sub-expression returns a FLOAT
-> the type assertion itself returns a float, and does nothing during evaluation
-> the expression returns a FLOAT
SELECT 1.2:::string
-> no preference for the return type of `... ::: STRING`
-> STRING preference passed for the return type of `1.2`
-> numeric constant does not contains STRING in its "resolvable type set", so it
resolves itself to the default type of DECIMAL instead
-> the type annotation throws an error when asserting that its sub-expression
returns a STRING, type checking fails

SELECT sign(1.2)
-> no preference for the return type of `sign`
-> no preference for the return type of `1.2`
-> `1.2` defaults to a DECIMAL
-> overload resolution chooses the DECIMAL implementation
-> the expression returns a DECIMAL
SELECT sign(1.2)::float
-> no preference for the return type of CAST(... as FLOAT)
-> no preference for the return type of `sign`
-> no preference for the return type of `1.2`
-> `1.2` defaults to a DECIMAL
-> overload resolution chooses the DECIMAL implementation
-> CastExpr returns a FLOAT instead, which it will cast during evaluation
-> the expression returns a FLOAT
SELECT sign(1.2)::string
-> no preference for the return type of CAST(... as STRING)
-> no preference for the return type of `sign`
-> no preference for the return type of `1.2`
-> `1.2` defaults to a DECIMAL
-> overload resolution chooses the DECIMAL implementation
-> CastExpr returns a STRING instead, which it will cast (format) during evaluation
-> the expression returns a STRING
SELECT sign(1.2):::float
-> no preference for the return type of `... ::: FLOAT`
-> FLOAT preference passed for the return type of `sign`
-> FLOAT used in overload resolution to determine float overload and
to resolve the numeric constant as a float
-> the type annotation correctly asserts that its sub-expression returns a FLOAT
-> the type assertion itself returns a float, and does nothing during evaluation
-> the expression returns a FLOAT
SELECT sign(1.2):::string
-> no preference for the return type of `... ::: STRING`
-> STRING preference passed for the return type of `sign`
-> STRING ignore in overload resolution because no overloads return a STRING,
so DECIMAL implementation is defaulted to
-> the type annotation throws an error when asserting that its sub-expression
returns a STRING, type checking fails
```

Note that in the above examples, adding a type annotation onto the top
of a SELECT clause has an almost identical effect on type checking as
INSERTing into a column with the annotation type.

### Interaction with Placeholders

The initial Summer RFC defined a set of rules for determining the type of
placeholders based on type casts and type annotations. The proposed rules
are:

- if any given placeholder appears as immediate argument of an
explicit annotation, then assign that type to the placeholder (and
reject conflicting annotations after the 1st).

- otherwise (no direct annotations on a placeholder), if all
occurrences of a placeholder appear as immediate argument
to a cast expression then:

- if all the cast(s) are homogeneous,
then assign the placeholder the type indicated by the cast.

- otherwise, assign the type "string" to the placeholder.

- for anything else, defer to type checking to determine the placeholder type.

Examples:

```sql
SELECT $1:::float, $1::string
-> $1 ::: float, execution will perform explicit cast float->string
SELECT $1:::float, $1:::string
-> error: conflicting types
SELECT $1::float, $1::float
-> $1 ::: float
SELECT $1::float, $1::string
-> $1 ::: string, execution will perform explicit cast $1 -> float
SELECT $1:::float, $1
-> $1 ::: float
SELECT $1::float, $1
-> nothing done during 1st pass, type checking will resolve
```

(Note that this rule does not interfere with the separate rule,
customary in SQL interpreters, that the client may choose to disregard
the stated type of a placeholder during execute and instead pass the
value as a string. The query executor must then convert the string to
the type for the placeholder that was determined during type checking.
For example if a client prepares `select $1:::int + 2` and passes "123" (a string),
the executor must convert "123" to 123 (an int) before running the query. The
annotation expression is a type assertion, not conversion, at run-time.)

This set of rules has already been partially implemented in the
`parser.placeholderAnnotationVisitor`, which is used through
`parser.ProcessPlaceholderAnnotations`.

### Implementation Plan

A `TypeAnnotationExpr` or `AnnotationExpr` will be created in the `parser`
package. It's implementation will mirror that of the `ParenExpr`, except
in its `TypeCheck` method, where it will call `typeCheckAndRequire` on
its sub-expression with its annotation type. The expression could also
make a similar type assertion in its `Eval` method, but this shouldn't
be necessary.

The `ProcessPlaceholderAnnotations` will need to be adjusted to adopt the
rules listed above for interactions between casts and annotions. This is
already stubbed out.

# Drawbacks

Type annotations will be a Cockroach specific language extension to SQL.
In general, we have tried to avoid language extensions, as they limit
portability of SQL statements and create a higher learning curve for
moving to Cockroach. However, we concluded that this was ok, because
these annotations are fully opt-in, and are not a requirement of the
dialect.

# Alternatives

None. This is strictly an isolated extension.

# Unresolved questions

### EXPLAIN(TYPES)

An initial proof of concept implementation of type annotations made it
clear that `EXPLAIN(TYPES)` in conjunction with type annotations results
in a fairly verbose output. It might make sense to make type annotations
"invisible" in the output of `EXPLAIN(TYPES)`, as they are annotations
for the type system, as opposed to expressions which affect evaluation.
Furthermore, if the annotation passes type checking, its child will already
have the same type, so including it in the output is unnecessary.
Loading

0 comments on commit 1fe9e73

Please sign in to comment.