Esql Fix bug with loading TS metadata #134648
Conversation
not-napoleon
added
>test
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| * Tests for the {@link org.elasticsearch.xpack.esql.optimizer.rules.logical.local.IgnoreNullMetrics} planner rule, to | ||
| * verify that the filters are being pushed to Lucene. | ||
| */ | ||
| public class IgnoreNullMetricsPhysicalPlannerTests extends LocalPhysicalPlanOptimizerTests { |
There was a problem hiding this comment.
Inheriting from LocalPhysicalPlanOptimizerTests seems wrong, but there's no base class between that and MapperTestCase, and this is the strategy ReplaceRoundToWithQueryAndTagsTests uses. This does mean that all the (many) tests in LocalPhysicalPlanOptimizerTests will be re-run in this test class. I recommend refactoring the vaious plannerOptimizer objects into an abstract base class that all three of these classes can inherit their scaffolding from, but such a refactoring is out of scope for this PR, and I will defer to the people who frequently look at this code.
|
Note to future self: Remember to run |
not-napoleon
changed the title
| @@ -225,7 +225,7 @@ private static EsField createField( | |||
| List<IndexFieldCapabilities> rest = fcs.subList(1, fcs.size()); | |||
| DataType type = EsqlDataTypeRegistry.INSTANCE.fromEs(first.type(), first.metricType()); | |||
| boolean aggregatable = first.isAggregatable(); | |||
| EsField.TimeSeriesFieldType timeSeriesFieldType = EsField.TimeSeriesFieldType.UNKNOWN; | |||
| EsField.TimeSeriesFieldType timeSeriesFieldType = EsField.TimeSeriesFieldType.fromIndexFieldCapabilities(first); | |||
There was a problem hiding this comment.
This is the entire bug fix. Everything else in this PR is tests and test scaffolding.
There was a problem hiding this comment.
These changes are necessary to enable running planner tests that load dimension and metric information from our json schema files. My earlier tests for this feature hard coded a test schema rather than load from json, so I didn't need this before, but we'll want it for many tests going forward.
| .get(); | ||
| List<Doc> sparseDocs = new ArrayList<>(); | ||
| // generate 100 docs, 50 will have a null metric | ||
| // TODO: this is all copied from populateIndex(), refactor it sensibly. |
There was a problem hiding this comment.
I really don't want to refactor this test today, and copying a few dozen lines of test code isn't that big a sin. I'm going to leave this TODO for someone to take on a slow Friday.
| @@ -119,14 +119,15 @@ public void testSimple() { | |||
| assumeTrue("requires metrics command", EsqlCapabilities.Cap.METRICS_COMMAND.isEnabled()); | |||
| LogicalPlan actual = localPlan(""" | |||
| TS test | |||
| | STATS max(max_over_time(metric_1)) | |||
| | STATS max(max_over_time(metric_1)) BY BUCKET(@timestamp, 1 min) | |||
There was a problem hiding this comment.
Makes sure we're testing this rule and not InferNonNullAggConstraint
| client().admin().indices().prepareRefresh("sparse-hosts").get(); | ||
| // Control test | ||
| /* | ||
| try (EsqlQueryResponse resp = run(""" |
There was a problem hiding this comment.
probably worth keeping, I just forgot to uncomment it after running the debugger.
| boolean isDimension = boolSetting(content.get("time_series_dimension"), false); | ||
| boolean isMetric = content.containsKey("time_series_metric"); | ||
| if (isDimension && isMetric) { | ||
| throw new IllegalArgumentException("Field configured as both dimension and metric:" + value); |
There was a problem hiding this comment.
This should never happen here, probably IllegalStateException?
There was a problem hiding this comment.
I don't have a strong opinion, but I chose IllegalArgumentException because this method already throws that on line 133 for other invalid mapping states.
|
I feel confidant that the release tests failures are unrelated to this PR, so I'm taking the tag off. I continue to wish we had a better way to do this. |
Relates to #133087
@dnhatn reported that we weren't seeing the performance improvement we expected from the null filters. I added tests and investigated, and it turns out I had forgotten to load the TS metadata from a single filed caps response. This PR includes the tests that help find the problem and the fix.