Merge pull request #465 from travisturner/document-shift

Add comments warning that Shift() is unsupported

docs/query-language.md

@@ -595,34 +595,6 @@ Count(Row(stargazer=1))
 
 * Result is the number of repositories that user 1 has starred.
 
-#### Shift
-**Spec:**
-
-```
-Shift(<ROW_CALL>, [n=UINT])
-```
-
-**Description:**
-
-Returns the row specified by `ROW_CALL` shifted by `n` bits.
-
-**Result Type:** object with attrs and columns
-
-attrs will always be empty
-
-**Examples:**
-
-Query all columns with a bit set in row 1 of the field `stargazer`
-and shift the result by 2:
-```request
-Shift(Row(stargazer=1), n=2)
-```
-```response
-{"attrs":{},"columns":[12, 22]}
-```
-
-* columns are the repositories which user 1 has starred shifted by 2 bits.
-
 #### TopN
 
 **Spec:**

executor_test.go

@@ -5173,6 +5173,10 @@ func runCallTest(t *testing.T, writeQuery string, readQueries []string, indexOpt
 	return responses
 }
 
+// NOTE: The shift function in its current state is unsupported.
+// If any of these tests fail due to improvements made to the roaring
+// code, it is reasonable to remove these tests. See the `Shift()`
+// method on `Row` in `row.go`.
 func TestExecutor_Execute_Shift(t *testing.T) {
 	t.Run("Shift Bit 0", func(t *testing.T) {
 		c := test.MustRunCluster(t, 1)

roaring/roaring.go

@@ -1534,6 +1534,9 @@ func (b *Bitmap) Xor(other *Bitmap) *Bitmap {
 }
 
 // Shift shifts the contents of b by 1.
+//
+// NOTE: This method is unsupported. See the `Shift()`
+// method on `Row` in `row.go`.
 func (b *Bitmap) Shift(n int) (*Bitmap, error) {
 	if n != 1 {
 		return nil, errors.New("cannot shift by a value other than 1")

row.go

@@ -387,6 +387,23 @@ func (r *Row) GenericUnaryOp(op ext.GenericBitmapOpBitmap, args map[string]inter
 
 // Shift returns the bitwise shift of r by n bits.
 // Currently only positive shift values are supported.
+//
+// NOTE: the Shift method is currently unsupported, and
+// is considerred to be incorrect. Please DO NOT use it.
+// We are leaving it here in case someone internally wants
+// to use it with the understanding that the results may
+// be incorrect.
+//
+// Why unsupported? For a full description, see:
+// https://github.com/molecula/pilosa/issues/403.
+// In short, the current implementation will shift a bit
+// at the edge of a shard out of the shard and into a
+// container which is assumed to be an invalid container
+// for the shard. So for example, shifting the last bit
+// of shard 0 (containers 0-15) will shift that bit out
+// to container 16. While this "sort of" works, it
+// breaks an assumption about containers, and might stop
+// working in the future if that assumption is enforced.
 func (r *Row) Shift(n int64) (*Row, error) {
 	if n < 0 {
 		return nil, errors.New("cannot shift by negative values")