From 195c6f7704677f4702a53fb47facd5915fdc18e2 Mon Sep 17 00:00:00 2001 From: Nia Weiss Date: Wed, 7 Oct 2020 12:14:44 -0400 Subject: [PATCH 1/2] remove "fix" for inspecting garbage keys Inspect was changed to discard garbage keys. Unfortunately, this change was incorrect, and users of inspect do not expect this. --- server/grpc.go | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/server/grpc.go b/server/grpc.go index b7b79efa3..78e630e1f 100644 --- a/server/grpc.go +++ b/server/grpc.go @@ -584,11 +584,13 @@ func (h *GRPCHandler) Inspect(req *pb.InspectRequest, stream pb.Pilosa_InspectSe // // A query which will work here is 'SELECT *' or any query with more columns // than just _id. - if colAdded > 0 { - if err := stream.Send(rowResp); err != nil { - return errors.Wrap(err, "sending response to stream") - } + //if colAdded > 0 { + // This has been reverted, as it violates the expectations of users of inspect. + // It is expected that inspect will return garbage. + if err := stream.Send(rowResp); err != nil { + return errors.Wrap(err, "sending response to stream") } + //} } } else { @@ -607,7 +609,7 @@ func (h *GRPCHandler) Inspect(req *pb.InspectRequest, stream pb.Pilosa_InspectSe } } - forceSend := false + //forceSend := false ci := []*pb.ColumnInfo{ {Name: "_id", Datatype: "string"}, } @@ -627,11 +629,11 @@ func (h *GRPCHandler) Inspect(req *pb.InspectRequest, stream pb.Pilosa_InspectSe end = uint64(len(cols)) } cols = cols[offset:end] - if len(cols) == 1 { + /*if len(cols) == 1 { if id, err := h.api.TranslateIndexKey(stream.Context(), index.Name(), cols[0], false); id != 0 && err == nil { forceSend = true } - } + }*/ } else { // Prevent getting too many records by forcing a limit. pql := fmt.Sprintf("All(limit=%d, offset=%d)", limit, offset) @@ -909,11 +911,13 @@ func (h *GRPCHandler) Inspect(req *pb.InspectRequest, stream pb.Pilosa_InspectSe // ------- // // (1 row) - if colAdded > 0 || forceSend { - if err := stream.Send(rowResp); err != nil { - return errors.Wrap(err, "sending response to stream") - } + //if colAdded > 0 || forceSend { + // This has been reverted, as it violates the expectations of users of inspect. + // It is expected that inspect will return garbage. + if err := stream.Send(rowResp); err != nil { + return errors.Wrap(err, "sending response to stream") } + //} } } From 9f7225b14925e5f4e6e8363a9e53330a0a23c9dd Mon Sep 17 00:00:00 2001 From: Nia Weiss Date: Thu, 8 Oct 2020 13:21:30 -0400 Subject: [PATCH 2/2] delete commented-out broken code in inspect --- server/grpc.go | 37 ++----------------------------------- 1 file changed, 2 insertions(+), 35 deletions(-) diff --git a/server/grpc.go b/server/grpc.go index 78e630e1f..cac1d0853 100644 --- a/server/grpc.go +++ b/server/grpc.go @@ -569,28 +569,12 @@ func (h *GRPCHandler) Inspect(req *pb.InspectRequest, stream pb.Pilosa_InspectSe } } - // For SQL queries like: - // SELECT * FROM t WHERE _id=garbageID; - // we don't want to return any rows. - // So, check here if we added any columns. - // - // Because we don't have keys to translate - // and _id is an artificial field that's why for query: - // SELECT _id FROM t WHERE _id=existing-id; - // we return an empty result. - // - // TODO(kuba--): We need to find a way to check here if - // existing-id is not a garbage. - // - // A query which will work here is 'SELECT *' or any query with more columns - // than just _id. - //if colAdded > 0 { + // Previously, we discarded rows that did not exist. // This has been reverted, as it violates the expectations of users of inspect. // It is expected that inspect will return garbage. if err := stream.Send(rowResp); err != nil { return errors.Wrap(err, "sending response to stream") } - //} } } else { @@ -609,7 +593,6 @@ func (h *GRPCHandler) Inspect(req *pb.InspectRequest, stream pb.Pilosa_InspectSe } } - //forceSend := false ci := []*pb.ColumnInfo{ {Name: "_id", Datatype: "string"}, } @@ -629,11 +612,6 @@ func (h *GRPCHandler) Inspect(req *pb.InspectRequest, stream pb.Pilosa_InspectSe end = uint64(len(cols)) } cols = cols[offset:end] - /*if len(cols) == 1 { - if id, err := h.api.TranslateIndexKey(stream.Context(), index.Name(), cols[0], false); id != 0 && err == nil { - forceSend = true - } - }*/ } else { // Prevent getting too many records by forcing a limit. pql := fmt.Sprintf("All(limit=%d, offset=%d)", limit, offset) @@ -901,23 +879,12 @@ func (h *GRPCHandler) Inspect(req *pb.InspectRequest, stream pb.Pilosa_InspectSe } } - // For SQL queries like: - // SELECT _id FROM parent WHERE _id="garbage"; - // we get here without any real columns and fields, and we did not - // translate any keys. That's why we don't want to send anything back - // and return fake response like: - // - // _id - // ------- - // - // (1 row) - //if colAdded > 0 || forceSend { + // Previously, we discarded rows that did not exist. // This has been reverted, as it violates the expectations of users of inspect. // It is expected that inspect will return garbage. if err := stream.Send(rowResp); err != nil { return errors.Wrap(err, "sending response to stream") } - //} } }