From 9f2b888c4f7d83cfbf8242fa3daa3ff7833245e6 Mon Sep 17 00:00:00 2001 From: Seebs Date: Thu, 7 Apr 2022 13:20:00 -0500 Subject: [PATCH] standardize and correct time range handling If you're wondering how something that simple gets a commit message this long, sit down, because you are in for a ride. The Row, Rows, TopK, and GroupBy(Rows...) commands had three different sets of semantics for from/to ranges. We unify these. Sounds easy, right? The original purpose of this was to address a bug in GroupBy where, if you had multiple queries only one of which used time, we could end up silently returning no results because we tried to do a time query against a non-time field. This was easy to fix; just move a boolean flag from outside a loop to inside the loop so it resets to false on each pass. In the process of trying to test that, I discovered that specifying `from=...` without `to=...` in a Rows in a GroupBy didn't work. Searching around, I discovered that we had three different answers: GroupBy, TopK: unspecified 'to=' is 0 Row: unspecified to is tomorrow Rows: unspecified to is the max time quantum in the field (A time value of 0 is apparently interpreted as January 1st, 0001.) Note that "GroupBy" is really referring to a Rows() command in a GroupBy, it's just that this uses completely different code (because it has to be computing rows potentially matching or restricted to a filter, or provide the rows it generated so they can be used to filter something else). So we fixed that, and made a field method for finding the min/max values (as done in a Rows command that *isn't* in a GroupBy), and tried to use that with viewsByTimeRange. Then I tried to write documentation for this, but the documentation was unclear, and I tried to clear it up. Which caused me to discover that these four different places ALSO differed in when or whether they'd replace a broad query with "just the standard view". So. Round two of the fix: We create a `field.viewsByTimeRange`, which tries to fall back to a standard view when one exists and the specified range covers everything, and treats zero values as non-restrictive, but also picks a narrow range that is actually related to the range of dates in the field. This matters because viewsByTimeRange generates the entire set of views it would need *even if those views don't exist*. We drop one test that was testing Rows specifically to verify that, if you omitted To, we acted as though you'd specified a date two days in the future. That behavior is not now intended, so we drop the test that tries to verify it. Thing that might make this better: Figuring out a way to generate the list of views more cheaply. Right now, we're redoing all the view computation, including producing a sorted list of view names, for every shard. This is excessive, but hard to fix. In particular, there is no trivial way to generate a sorting such that you can take slices of it and have them be the right slices, because we want to skip smaller time quanta when an entire larger parent quantum is included. e.g., if we're including all of April 2022, we don't want to include any of the days for April of 2022, but if we're doing up through April 15th, we want to include the first 15 days of April, but NOT include the whole-month quantum. And so on. Fixing this cleanly is hard and would require a significant design effort. --- executor.go | 87 ++++++++++-------------------------------- executor_test.go | 42 +++++++++++++++++--- field.go | 49 ++++++++++++++++++++++++ field_internal_test.go | 67 ++++++++++++++++++++++++++++++++ 4 files changed, 173 insertions(+), 72 deletions(-) diff --git a/executor.go b/executor.go index 0f52e09d8..ade3b9001 100644 --- a/executor.go +++ b/executor.go @@ -2182,16 +2182,14 @@ func (e *executor) executeTopKShardTime(ctx context.Context, tx Tx, filter *Row, return nil, newNotFoundError(ErrFieldNotFound, field) } - // Check the time quantum. - quantum := f.TimeQuantum() - if quantum == "" { - // ???????? - return nil, nil + views, err := f.viewsByTimeRange(from, to) + if err != nil { + return nil, err } // Fetch fragments. var fragments []*fragment - for _, view := range viewsByTimeRange(viewStandard, from, to, quantum) { + for _, view := range views { f := e.Holder.fragment(index, field, view, shard) if f == nil { continue @@ -3746,48 +3744,9 @@ func (e *executor) executeRowsShard(ctx context.Context, qcx *Qcx, index string, return nil, errors.Wrap(err, "parsing to time") } } - - // Calculate the views for a range as long as some piece of the range - // (from/to) are specified, or if there's no standard view to represent - // all dates. - if !fromTime.IsZero() || !toTime.IsZero() || f.options.NoStandardView { - // If no quantum exists then return an empty result set. - q := f.TimeQuantum() - if q == "" { - return rowIDs, nil - } - - // Get min/max based on existing views. - var vs []string - for _, v := range f.views() { - vs = append(vs, v.name) - } - min, max := minMaxViews(vs, q) - - // If min/max are empty, there were no time views. - if min == "" || max == "" { - return rowIDs, nil - } - - // Convert min/max from string to time.Time. - minTime, err := timeOfView(min, false) - if err != nil { - return rowIDs, errors.Wrapf(err, "getting min time from view: %s", min) - } - if fromTime.IsZero() || fromTime.Before(minTime) { - fromTime = minTime - } - - maxTime, err := timeOfView(max, true) - if err != nil { - return rowIDs, errors.Wrapf(err, "getting max time from view: %s", max) - } - if toTime.IsZero() || toTime.After(maxTime) { - toTime = maxTime - } - - // Determine the views based on the specified time range. - views = viewsByTimeRange(viewStandard, fromTime, toTime, q) + views, err = f.viewsByTimeRange(fromTime, toTime) + if err != nil { + return nil, err } default: return nil, errors.Errorf("%s fields not supported by Rows() query", f.Type()) @@ -4585,21 +4544,12 @@ func (e *executor) executeRowShard(ctx context.Context, qcx *Qcx, index string, return row, err } - // If no quantum exists then return an empty bitmap. - q := f.TimeQuantum() - if q == "" { - return &Row{}, nil - } - - // Set maximum "to" value if only "from" is set. We don't need to worry - // about setting the minimum "from" since it is the zero value if omitted. - if toTime.IsZero() { - // Set the end timestamp to current time + 1 day, in order to account for timezone differences. - toTime = time.Now().AddDate(0, 0, 1) + views, err := f.viewsByTimeRange(fromTime, toTime) + if err != nil { + return nil, err } // Union bitmaps across all time-based views. - views := viewsByTimeRange(viewStandard, fromTime, toTime, q) rows := make([]*Row, 0, len(views)) tx, finisher, err := qcx.GetTx(Txo{Write: !writable, Index: idx, Shard: shard}) defer finisher(&err0) @@ -7856,14 +7806,14 @@ func newGroupByIterator(executor *executor, qcx *Qcx, rowIDs []RowIDs, children idx := holder.Index(index) var ( - fieldName string - viewName string - ok bool - views []string - isTimeField bool + fieldName string + viewName string + ok bool + views []string ) ignorePrev := false for i, call := range children { + var isTimeField bool if fieldName, ok = call.Args["_field"].(string); !ok { return nil, errors.Errorf("%s call must have field with valid (string) field name. Got %v of type %[2]T", call.Name, call.Args["_field"]) } @@ -7907,7 +7857,12 @@ func newGroupByIterator(executor *executor, qcx *Qcx, rowIDs []RowIDs, children } if hasTo || hasFrom { - views = viewsByTimeRange(viewStandard, fromTime, toTime, field.TimeQuantum()) + // Determine the views based on the specified time range. + var err error + views, err = field.viewsByTimeRange(fromTime, toTime) + if err != nil { + return nil, err + } isTimeField = true } else { viewName = viewStandard diff --git a/executor_test.go b/executor_test.go index 6f0e8839d..069f27b7b 100644 --- a/executor_test.go +++ b/executor_test.go @@ -469,21 +469,17 @@ func TestExecutor(t *testing.T) { t.Run("Range", func(t *testing.T) { t.Run("RowIDColumnID", func(t *testing.T) { - // Create a timestamp just out of the current date + 1 day timestamp (default end timestamp). - nextDayExclusive := time.Now().AddDate(0, 0, 2) - - writeQuery := fmt.Sprintf(` + writeQuery := ` Set(2, f=1, 1999-12-31T00:00) Set(3, f=1, 2000-01-01T00:00) Set(4, f=1, 2000-01-02T00:00) Set(5, f=1, 2000-02-01T00:00) Set(6, f=1, 2001-01-01T00:00) Set(7, f=1, 2002-01-01T02:00) - Set(8, f=1, %s) Set(2, f=1, 1999-12-30T00:00) Set(2, f=1, 2002-02-01T00:00) - Set(2, f=10, 2001-01-01T00:00)`, nextDayExclusive.Format("2006-01-02T15:04")) + Set(2, f=10, 2001-01-01T00:00)` readQueries := []string{ `Row(f=1, from=1999-12-31T00:00, to=2002-01-01T03:00)`, `Row(f=1, from=1999-12-31T00:00)`, @@ -5766,6 +5762,7 @@ func TestExecutor_Execute_GroupBy(t *testing.T) { defer c.Close() c.CreateField(t, "i", pilosa.IndexOptions{}, "general") c.CreateField(t, "i", pilosa.IndexOptions{}, "sub") + c.CreateField(t, "i", pilosa.IndexOptions{}, "tq", pilosa.OptFieldTypeTime("YMDH", "0")) c.CreateField(t, "i", pilosa.IndexOptions{}, "v", pilosa.OptFieldTypeInt(0, 1000)) c.ImportBits(t, "i", "general", [][2]uint64{ {10, 0}, @@ -5786,6 +5783,7 @@ func TestExecutor_Execute_GroupBy(t *testing.T) { {110, 2}, {110, 0}, }) + if _, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(0, v=10)`}); err != nil { t.Fatal(err) } else if _, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(1, v=100)`}); err != nil { @@ -6198,6 +6196,38 @@ func TestExecutor_Execute_GroupBy(t *testing.T) { ) }) + // Create some time-quantum data: + c.Query(t, "i", "Set(0, tq=1, 2022-01-01T01:01)") + c.Query(t, "i", "Set(1, tq=1, 2021-01-01T01:01)") + t.Run("GroupByWithTime", func(t *testing.T) { + expected := map[string][]pilosa.GroupCount{ + // no time specified + "GroupBy(Rows(tq), Rows(general))": { + {Group: []pilosa.FieldRow{{Field: "tq", RowID: 1}, {Field: "general", RowID: 10}}, Count: 2}, + }, + // time specified but includes all data + "GroupBy(Rows(tq, from=2020-01-01T01:01), Rows(general))": { + {Group: []pilosa.FieldRow{{Field: "tq", RowID: 1}, {Field: "general", RowID: 10}}, Count: 2}, + }, + // same but in a different order + "GroupBy(Rows(general), Rows(tq, from=2020-01-01T01:01))": { + {Group: []pilosa.FieldRow{{Field: "general", RowID: 10}, {Field: "tq", RowID: 1}}, Count: 2}, + }, + // time excludes any data + "GroupBy(Rows(general), Rows(tq, from=2022-01-01T01:01))": { + {Group: []pilosa.FieldRow{{Field: "general", RowID: 10}, {Field: "tq", RowID: 1}}, Count: 1}, + }, + // limit excludes all data + "GroupBy(Rows(general), Rows(tq, from=2023-01-01T01:01))": {}, + } + + for query, want := range expected { + results := c.Query(t, "i", query).Results[0].(*pilosa.GroupCounts).Groups() + t.Logf("query %q", query) + test.CheckGroupBy(t, want, results) + } + + }) } for _, size := range []int{1, 3} { t.Run(fmt.Sprintf("%d_nodes", size), func(t *testing.T) { diff --git a/field.go b/field.go index 8c404068c..df87eea91 100644 --- a/field.go +++ b/field.go @@ -968,6 +968,55 @@ 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. +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. + q := f.TimeQuantum() + if q == "" { + return []string{viewStandard}, nil + } + + // Get min/max based on existing views. + var vs []string + for _, v := range f.views() { + vs = append(vs, v.name) + } + min, max := minMaxViews(vs, q) + + // If min/max are empty, there were no time views. + if min == "" || max == "" { + return []string{viewStandard}, nil + } + + wasZero := from.IsZero() && to.IsZero() + // Convert min/max from string to time.Time. + minTime, err := timeOfView(min, false) + if err != nil { + return nil, errors.Wrapf(err, "getting min time from view: %s", min) + } + if from.IsZero() || from.Before(minTime) { + from = minTime + } + + maxTime, err := timeOfView(max, true) + if err != nil { + return nil, errors.Wrapf(err, "getting max time from view: %s", max) + } + 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 +} + // RowTime gets the row at the particular time with the granularity specified by // the quantum. func (f *Field) RowTime(tx Tx, rowID uint64, time time.Time, quantum string) (*Row, error) { diff --git a/field_internal_test.go b/field_internal_test.go index fb2f01060..268ca95d6 100644 --- a/field_internal_test.go +++ b/field_internal_test.go @@ -911,3 +911,70 @@ func TestField_SaveMeta(t *testing.T) { t.Fatalf("expected value after reopen to be: %d, got: %d", val, rslt) } } + +func TestFieldViewsByTimeRange(t *testing.T) { + f := OpenField(t, OptFieldTypeTime("YMD", "0", false)) + for _, date := range []string{ + // a handful of YMD parameters describing dates that we could have data for + "2021", + "202112", + "20211229", + "20211230", + "20211231", + "2022", + "202201", + "20220101", + "20220102", + } { + _, err := f.createViewIfNotExists(viewStandard + "_" + date) + if err != nil { + t.Fatalf("creating view for %s: %v", date, err) + } + } + var testCases = []struct { + from, to string + expected []string + }{ + {"", "", []string{"standard"}}, + {"2020-12-31T00:00", "2023-01-03T00:00", []string{"standard"}}, + {"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"}}, + {"2021-12-01T00:00", "", []string{"standard_202112", "standard_2022"}}, + {"2021-12-30T00:00", "2022-02-01T00:00", []string{"standard_20211230", "standard_20211231", "standard_202201"}}, + } + for _, tc := range testCases { + t.Logf("checking %q to %q", tc.from, tc.to) + var fromTime, toTime time.Time + var err error + if tc.from != "" { + fromTime, err = time.Parse("2006-01-02T15:04", tc.from) + if err != nil { + t.Fatalf("invalid time %q: %v", tc.from, err) + } + } + if tc.to != "" { + toTime, err = time.Parse("2006-01-02T15:04", tc.to) + if err != nil { + t.Fatalf("invalid time %q: %v", tc.to, err) + } + } + views, err := f.viewsByTimeRange(fromTime, toTime) + if err != nil { + t.Fatalf("unexpected error getting views for %s-%s: %v", tc.from, tc.to, err) + } + for i, v := range tc.expected { + if len(views) <= i { + t.Fatalf("expected view %q, didn't get it", v) + } else { + if views[i] != v { + t.Fatalf("expected view %q, got %q", v, views[i]) + } + } + } + if len(views) > len(tc.expected) { + t.Fatalf("unexpected view %q", views[len(tc.expected)]) + } + t.Logf("views: %v", views) + } +}