From a093099a8c72955b5833835c2221f4c9cbbb95e0 Mon Sep 17 00:00:00 2001 From: Matthew Jaffee Date: Tue, 3 May 2022 16:46:12 -0500 Subject: [PATCH] dont default to standard view for some time range queries previously, we would use the standard view if the query seemed to cover all the views we had, or if we didn't seem to have any time views. This is unintuitive if some views have been deleted (which comes up a lot more often with TTL!). It's also unintuitive if you know you haven't set any data w/ a timestamp and your query that specifies a time range returns any data. --- executor_test.go | 62 +++++++++++++++++++++++++++++++++++++++--- field.go | 25 ++++++++--------- field_internal_test.go | 4 +-- 3 files changed, 71 insertions(+), 20 deletions(-) diff --git a/executor_test.go b/executor_test.go index bdcef2ee8..f81209757 100644 --- a/executor_test.go +++ b/executor_test.go @@ -1067,6 +1067,64 @@ func TestExecutor(t *testing.T) { }) } }) + + // This is a regression test which checks that queries over a time + // range that encompass all available time views do not default to + // using the standard view. This can be incorrect in the case + // where some time views have been deleted. + t.Run("TimeQueriesFullRange", func(t *testing.T) { + ts := func(t time.Time) int64 { + return t.Unix() * 1e+9 + } + indexName := "tq_range" + c.CreateField(t, indexName, pilosa.IndexOptions{Keys: true, TrackExistence: true}, "f1", pilosa.OptFieldKeys(), pilosa.OptFieldTypeTime(pilosa.TimeQuantum("D"), "0")) + c.ImportTimeQuantumKey(t, indexName, "f1", []test.TimeQuantumKey{ + // from edge cases + {ColKey: "C1", RowKey: "R1", Ts: ts(time.Date(2022, 1, 10, 0, 0, 0, 0, time.UTC))}, + {ColKey: "C2", RowKey: "R1", Ts: ts(time.Date(2022, 1, 11, 0, 0, 0, 0, time.UTC))}, + {ColKey: "C3", RowKey: "R1", Ts: ts(time.Date(2022, 1, 12, 0, 0, 0, 0, time.UTC))}, + }) + c.ImportKeyKey(t, indexName, "f1", [][2]string{ + {"R1", "C2"}, + {"R1", "C3"}, + {"R1", "C4"}, + {"R1", "C5"}, + {"R1", "C6"}, + }) + + tr := c.QueryGRPC(t, indexName, "Row(f1=R1, from='2022-01-10', to='2022-01-13')") + csvString, err := tableResponseToCSVString(tr) + if err != nil { + t.Fatalf("converting to CSV: %v", err) + } + expected := ` +_id +C1 +C3 +C2 +`[1:] + // check that an unqualified query does use the standard view + if csvString != expected { + t.Fatalf("expected:\n%s\ngot:\n%s\n", expected, csvString) + } + tr = c.QueryGRPC(t, indexName, "Row(f1=R1)") + csvString, err = tableResponseToCSVString(tr) + if err != nil { + t.Fatalf("converting to CSV: %v", err) + } + expected = ` +_id +C1 +C3 +C2 +C5 +C4 +C6 +`[1:] + if csvString != expected { + t.Fatalf("expected:\n%s\ngot:\n%s\n", expected, csvString) + } + }) } func runCallTest(c *test.Cluster, t *testing.T, writeQuery string, readQueries []string, indexOptions *pilosa.IndexOptions, fieldOption ...pilosa.FieldOption) []pilosa.QueryResponse { @@ -5076,10 +5134,6 @@ func TestExecutor_Execute_Rows(t *testing.T) { } } -func TestExecutor_Execute_RowsTime(t *testing.T) { - -} - // Ensure that an empty time field returns empty Rows(). func TestExecutor_Execute_RowsTimeEmpty(t *testing.T) { c := test.MustRunCluster(t, 1) diff --git a/field.go b/field.go index 988a79186..5eae96ca6 100644 --- a/field.go +++ b/field.go @@ -968,17 +968,18 @@ func (f *Field) TimeQuantum() TimeQuantum { return f.options.TimeQuantum } -// viewsByTimeRange is a wrapper on the non-method viewsByTimeRange, which -// computes views for a specific field for a given time range. The difference -// is that, as a Field operation, it can return "standard" for a view that -// covers the whole time range, if the field supports a standard view, and -// can automatically coerce from/to times to match the actual range present -// in the field. +// viewsByTimeRange is a wrapper on the non-method viewsByTimeRange, +// which computes views for a specific field for a given time +// range. The difference is that, it can return "standard" if from/to +// are not set and can automatically coerce from/to times to match the +// actual range present in the field. func (f *Field) viewsByTimeRange(from, to time.Time) (views []string, err error) { - // If we can't find time views at all, we'll yield "standard" regardless. - // It's the least-bad answer, I think. + // If we can't find time views at all, we'll yield "standard" + // regardless. It's the least-bad answer, I think. Also yield + // "standard" if from and to were both not set and there is a + // standard view. q := f.TimeQuantum() - if q == "" { + if q == "" || (from.IsZero() && to.IsZero() && !f.options.NoStandardView) { return []string{viewStandard}, nil } @@ -991,10 +992,9 @@ func (f *Field) viewsByTimeRange(from, to time.Time) (views []string, err error) // If min/max are empty, there were no time views. if min == "" || max == "" { - return []string{viewStandard}, nil + return []string{}, nil } - wasZero := from.IsZero() && to.IsZero() // Convert min/max from string to time.Time. minTime, err := timeOfView(min, false) if err != nil { @@ -1011,9 +1011,6 @@ func (f *Field) viewsByTimeRange(from, to time.Time) (views []string, err error) if to.IsZero() || to.After(maxTime) { to = maxTime } - if (wasZero || (from == minTime && to == maxTime)) && !f.Options().NoStandardView { - return []string{viewStandard}, nil - } return viewsByTimeRange(viewStandard, from, to, q), nil } diff --git a/field_internal_test.go b/field_internal_test.go index 268ca95d6..01f0bed0d 100644 --- a/field_internal_test.go +++ b/field_internal_test.go @@ -935,8 +935,8 @@ func TestFieldViewsByTimeRange(t *testing.T) { from, to string expected []string }{ - {"", "", []string{"standard"}}, - {"2020-12-31T00:00", "2023-01-03T00:00", []string{"standard"}}, + {"", "", []string{viewStandard}}, // this is interpreted as no time range being specified at all + {"2020-12-31T00:00", "2023-01-03T00:00", []string{"standard_2021", "standard_2022"}}, {"2021-01-01T00:00", "2022-01-01T00:00", []string{"standard_2021"}}, {"2021-01-01T00:00", "2022-01-02T00:00", []string{"standard_2021", "standard_20220101"}}, {"", "2022-01-02T00:00", []string{"standard_2021", "standard_20220101"}},