Skip to content
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.

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

[ML] Apply source query on data frame analytics memory estimation #49517

Conversation

dimitris-athanasiou
Copy link
Contributor

Closes #49454

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

Copy link
Contributor

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

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

LGTM

Two minor comments inline.


ExplainDataFrameAnalyticsAction.Response explainResponse = explainDataFrame(config);

assertThat(explainResponse.getMemoryEstimation().getExpectedMemoryWithoutDisk().getKb(), lessThanOrEqualTo(500L));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if

assertThat(explainResponse.getMemoryEstimation().getExpectedMemoryWithoutDisk(), lessThanOrEqualTo(new ByteSizeValue(500, KB)));

would work as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does but it makes the line longer and I don't think it significantly improves readability.

@jpountz
Copy link
Contributor

jpountz commented Dec 17, 2019

@dimitris-athanasiou FYI I'm removing the v7.5.1 label as this doesn't seem to have been backported to the 7.5 branch.

@jpountz jpountz removed the v7.5.1 label Dec 17, 2019
@droberts195
Copy link
Contributor

Looks like a logically equivalent fix is in 7.5.1, which is #49527. Both PRs have the same title and #49527 is in the release notes. Sorry for the extra work though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data Frame Analytics memory estimation ignores Query
6 participants