Skip to content

A random-random test for time-series data #132556

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

A random-random test for time-series data #132556

wants to merge 15 commits into from
+436 −11

Conversation

pabloem
Copy link
Contributor

@pabloem pabloem commented Aug 7, 2025 •
edited
Loading

Follow up items after this PR:

  • Test rate function and counters in general
  • Randomize window size
  • Increase test size
  • Hit some more corner cases (e.g. zero out some parameters)
  • Add a 'fixed schema' test case (as opposed to the current dynamic one)

@pabloem pabloem marked this pull request as ready for review August 11, 2025 18:06
@pabloem pabloem changed the title [wip][do not review] A random-random test for time-series data A random-random test for time-series data Aug 11, 2025
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Aug 11, 2025
@pabloem pabloem added >test Issues or PRs that are addressing/adding tests :StorageEngine/TSDB You know, for Metrics :StorageEngine/ES|QL Timeseries / metrics / logsdb capabilities in ES|QL and removed needs:triage Requires assignment of a team area label labels Aug 11, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

not-napoleon
not-napoleon reviewed Aug 11, 2025
View reviewed changes
Copy link
Member

@not-napoleon not-napoleon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to get some confirmation from @kkrik-es that this is doing what he wants, but I think it's pretty good. I left some feedback, none of which is critical but I'd like to get it addressed.

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java Outdated
private List<XContentBuilder> documents = null;
private DataGenerationHelper dataGenerationHelper;

private static final class DataGenerationHelper {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should be a top level class. Seems like we'll want to build multiple test classes using this framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved this class to its own file! TY.

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java Outdated
private static Object randomDimensionValue(String dimensionName) {
// We use dimensionName to determine the type of the value.
var isNumeric = dimensionName.hashCode() % 5 == 0;
if (isNumeric) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about IP dimensions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added 20% of dimensions as IP-like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as follow up ill add dynamic mapping to parse as ip. thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not rely on the existing one:
https://github.com/elastic/elasticsearch/blob/main/test/framework/src/main/java/org/elasticsearch/datageneration/fields/leaf/IpFieldDataGenerator.java

pabloem
pabloem commented Aug 12, 2025
View reviewed changes
Copy link
Contributor Author

@pabloem pabloem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TY @not-napoleon - ptal!

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java Outdated
private List<XContentBuilder> documents = null;
private DataGenerationHelper dataGenerationHelper;

private static final class DataGenerationHelper {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved this class to its own file! TY.

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java Outdated
private static Object randomDimensionValue(String dimensionName) {
// We use dimensionName to determine the type of the value.
var isNumeric = dimensionName.hashCode() % 5 == 0;
if (isNumeric) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added 20% of dimensions as IP-like.

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java Outdated
private static Object randomDimensionValue(String dimensionName) {
// We use dimensionName to determine the type of the value.
var isNumeric = dimensionName.hashCode() % 5 == 0;
if (isNumeric) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as follow up ill add dynamic mapping to parse as ip. thoughts?

kkrik-es
kkrik-es reviewed Aug 12, 2025
View reviewed changes
...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java Outdated
// Verify that the second column is the avg value (thus why row.get(2))
docValues.stream().mapToDouble(Integer::doubleValue).average().ifPresentOrElse(avgValue -> {
var res = (Double) row.get(2);
assertThat(res, closeTo(avgValue, res * 0.5));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the 0.5 factor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that was a mistake (I meant to do 5% not 50%).
However, average calculation does seem to have up to 20-25% error between ES and test-framework numbers. Should I check if that's a bug and how to deal with it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be no error here (Double.compare() should return 0), so there's a bug somewhere. Let's investigate separately.

kkrik-es
kkrik-es reviewed Aug 12, 2025
View reviewed changes
Copy link
Contributor

@kkrik-es kkrik-es left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Pablo, this ia a good step. It's nice that you tried to include the pass-through field on the first take, though that complicates things somewhat. I'd start with statically defined dimension and metric fields to get the validation logic in place first, then introduce dynamic fields on top of that.

Let's try to refactor the logic slightly so that it can be further extended in follow-up PRs.

kkrik-es
kkrik-es approved these changes Aug 13, 2025
View reviewed changes
Copy link
Contributor

@kkrik-es kkrik-es left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for addressing the comments. Let's keep iterating.

@pabloem pabloem enabled auto-merge (squash) August 15, 2025 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@not-napoleon not-napoleon not-napoleon left review comments

@kkrik-es kkrik-es kkrik-es approved these changes

Assignees
No one assigned
Labels
:StorageEngine/ES|QL Timeseries / metrics / logsdb capabilities in ES|QL :StorageEngine/TSDB You know, for Metrics Team:StorageEngine >test Issues or PRs that are addressing/adding tests v9.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pabloem @elasticsearchmachine @not-napoleon @kkrik-es